diff mbox

fs/jbd2: t_updates should increase when start_this_handle() failed in jbd2__journal_restart()

Message ID 51C1381A.2@huawei.com (mailing list archive)
State New, archived
Headers show

Commit Message

Younger Liu June 19, 2013, 4:48 a.m. UTC
jbd2_journal_restart() would restart a handle. In this function, it
calls start_this_handle(). Before calling start_this_handle()?subtract
1 from transaction->t_updates.
If start_this_handle() succeeds, transaction->t_updates increases by 1
in it. But if start_this_handle() fails, transaction->t_updates does
not increase.
So, when commit the handle's transaction in jbd2_journal_stop(), the
assertion is false, and then trigger a bug.
The assertion is as follows:
J_ASSERT(atomic_read(&transaction->t_updates) > 0) 

Signed-off-by: Younger Liu <younger.liu@huawei.com>
---
 fs/jbd2/transaction.c |    2 ++
 1 file changed, 2 insertions(+)

Comments

Theodore Ts'o June 20, 2013, 3:55 p.m. UTC | #1
[ LKML and linux-fsdevel BCC'ed ]

On Wed, Jun 19, 2013 at 12:48:26PM +0800, Younger Liu wrote:
> jbd2_journal_restart() would restart a handle. In this function, it
> calls start_this_handle(). Before calling start_this_handle()?subtract
> 1 from transaction->t_updates.
> If start_this_handle() succeeds, transaction->t_updates increases by 1
> in it. But if start_this_handle() fails, transaction->t_updates does
> not increase.
> So, when commit the handle's transaction in jbd2_journal_stop(), the
> assertion is false, and then trigger a bug.
> The assertion is as follows:
> J_ASSERT(atomic_read(&transaction->t_updates) > 0) 
> 
> Signed-off-by: Younger Liu <younger.liu@huawei.com>

Thanks for pointing out this potential problem.  Your fix isn't quite
the right one, however.

The problem is once we get to this point, the transaction pointer may
no longer be valid, since once we decrement t_updates, the transaction
could start commiting, and so we should not actually dereference the
transaction pointer after we unlock transaction->t_handle_lock.  (We
are referencing t_tid two lines later, and technically that's a bug.
We've just been getting lucky.)

The real issue is that by the time we call start_this_handle() in
jbd2__journal_restart, the handle is not attached to any transaction.
So if jbd2_journal_restart() fails, the handle has to be considered
invalid, and the calling code should not try to use the handle at all,
including calling jbd2_journal_stop().

Jan Kara is I believe currently on vacation but I'd really like him to
chime in with his opinion about the best way to fix this, since he's
also quite familiar with the jbd2 code.

Also, Jan has recently submitted changes to implement reserved handles
(to be submitted in the next merge window), and in these new
functions, if start_this_handle() fails when called from
jbd2_journal_start_reserved(), the handle is left invalidated, and the
caller of jbd2_journal_start_reserved() must not touch the handle
again, including calling jbd2_journal_stop() --- in fact, because
jbd2_journal_start_reserved() clears current->journal_info on failure,
an attempt to call jbd2_journal_stop() will result in the kernel oops
due to an assertion failure.

My inclination is to fix this in the same way, but it will require
changing the current code paths that use jbd2_journal_restart(), and
in some cases passing back the state that the handle is now invalid
and should not be released via jbd2_journal_stop() is going to be
tricky indeed.


Another possible fix is to set the handle to be aborted, via
jbd2_journal_abort_handle().  This function isn't used at all at the
moment, but from what I can tell this should do the right thing.  The
one unfortunate thing about this is that when jbd2_journal_stop() gets
called, it will return EROFS, which is a misleading error code.  I'm
guessing you're seeing this because start_this_handle() returned
ENOMEM, correct?  We could hack around this by stashing the real error
in the handle, and then change jbd2_journal_stop() to return that
error instead of EROFS if it is set.

This second solution is hacky all-around, and it's also inconsistent
with how we are doing things with jbd2_journal_start_reserved().  So
I'm not so happy with this solution.  But it would require a lot less
work because the fix would be isolated in the jbd2 layer.  OTOH, right
now if the code calls jbd2_journal_stop() on the handle after a
failure in jbd2_journal_start_reserved(), they are crashing anyway, so
changing the code so it changes with an assertion failure doesn't make
things any worse, and then we fix things in ext4 and ocfs2 without any
patch interdependencies --- and this is a problem which appears to
happen very rarely in practice.

(How did you manage to trigger this, BTW?  Was this something you
noticed by manual code inspection?  Or are you instrumenting the
kernel's memory allocators to occasionally fail to test our error
paths?  Or were you running in a system with very heavy memory
pressure?)

						- Ted



> ---
>  fs/jbd2/transaction.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 325bc01..9ddb444 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -530,6 +530,8 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, gfp_t gfp_mask)
>  	lock_map_release(&handle->h_lockdep_map);
>  	handle->h_buffer_credits = nblocks;
>  	ret = start_this_handle(journal, handle, gfp_mask);
> +	if (ret < 0)
> +		atomic_inc(&transaction->t_updates);
>  	return ret;
>  }
>  EXPORT_SYMBOL(jbd2__journal_restart);
> -- 
> 1.7.9.7
>
Josef Bacik June 20, 2013, 5:26 p.m. UTC | #2
On Thu, Jun 20, 2013 at 11:55:55AM -0400, Theodore Ts'o wrote:
> [ LKML and linux-fsdevel BCC'ed ]
> 
> On Wed, Jun 19, 2013 at 12:48:26PM +0800, Younger Liu wrote:
> > jbd2_journal_restart() would restart a handle. In this function, it
> > calls start_this_handle(). Before calling start_this_handle()?subtract
> > 1 from transaction->t_updates.
> > If start_this_handle() succeeds, transaction->t_updates increases by 1
> > in it. But if start_this_handle() fails, transaction->t_updates does
> > not increase.
> > So, when commit the handle's transaction in jbd2_journal_stop(), the
> > assertion is false, and then trigger a bug.
> > The assertion is as follows:
> > J_ASSERT(atomic_read(&transaction->t_updates) > 0) 
> > 
> > Signed-off-by: Younger Liu <younger.liu@huawei.com>
> 
> Thanks for pointing out this potential problem.  Your fix isn't quite
> the right one, however.
> 
> The problem is once we get to this point, the transaction pointer may
> no longer be valid, since once we decrement t_updates, the transaction
> could start commiting, and so we should not actually dereference the
> transaction pointer after we unlock transaction->t_handle_lock.  (We
> are referencing t_tid two lines later, and technically that's a bug.
> We've just been getting lucky.)
> 
> The real issue is that by the time we call start_this_handle() in
> jbd2__journal_restart, the handle is not attached to any transaction.
> So if jbd2_journal_restart() fails, the handle has to be considered
> invalid, and the calling code should not try to use the handle at all,
> including calling jbd2_journal_stop().
> 
> Jan Kara is I believe currently on vacation but I'd really like him to
> chime in with his opinion about the best way to fix this, since he's
> also quite familiar with the jbd2 code.
> 
> Also, Jan has recently submitted changes to implement reserved handles
> (to be submitted in the next merge window), and in these new
> functions, if start_this_handle() fails when called from
> jbd2_journal_start_reserved(), the handle is left invalidated, and the
> caller of jbd2_journal_start_reserved() must not touch the handle
> again, including calling jbd2_journal_stop() --- in fact, because
> jbd2_journal_start_reserved() clears current->journal_info on failure,
> an attempt to call jbd2_journal_stop() will result in the kernel oops
> due to an assertion failure.
> 
> My inclination is to fix this in the same way, but it will require
> changing the current code paths that use jbd2_journal_restart(), and
> in some cases passing back the state that the handle is now invalid
> and should not be released via jbd2_journal_stop() is going to be
> tricky indeed.
> 
> 
> Another possible fix is to set the handle to be aborted, via
> jbd2_journal_abort_handle().  This function isn't used at all at the
> moment, but from what I can tell this should do the right thing.  The
> one unfortunate thing about this is that when jbd2_journal_stop() gets
> called, it will return EROFS, which is a misleading error code.  I'm
> guessing you're seeing this because start_this_handle() returned
> ENOMEM, correct?  We could hack around this by stashing the real error
> in the handle, and then change jbd2_journal_stop() to return that
> error instead of EROFS if it is set.
> 
> This second solution is hacky all-around, and it's also inconsistent
> with how we are doing things with jbd2_journal_start_reserved().  So
> I'm not so happy with this solution.  But it would require a lot less
> work because the fix would be isolated in the jbd2 layer.  OTOH, right
> now if the code calls jbd2_journal_stop() on the handle after a
> failure in jbd2_journal_start_reserved(), they are crashing anyway, so
> changing the code so it changes with an assertion failure doesn't make
> things any worse, and then we fix things in ext4 and ocfs2 without any
> patch interdependencies --- and this is a problem which appears to
> happen very rarely in practice.
> 

I realize it's been a little bit since I've looked at jbd but I'll offer my
opinion.  Callers of jbd2_journal_restart() may not be the ones who originated
the handle, so doing what Jan has done with jbd2_journal_start_reserved() isn't
going to work because all the guy at the top is going to see is an error and
have no way to tell if his handle is invalid or not.

What I would suggest is getting a unified way to mark that the handle has
already been cleaned up and can just be free'd.  Then fix
jbd2_journal_start_reserved() and jbd2_journal_restart() to set that in the
handle and make jbd2_journal_stop() just free up the handle and reset
current->journal_info but not return an error.  It's important to not return an
error from jbd2_journal_stop() so that it doesn't invoke the ext4 error handling
stuff and you get a read only file system when the error may not be read only
file system worthy.

This way you have a nice clean way of dealing with handle errors that allow you
to pass back a real error to the caller and the caller can just do its normal
jbd2_journal_stop() and cleanup and do its own error handling the way it feels.
This keeps the yucky details of no longer valid handles all internal to jbd2 and
ext4/ocfs2 don't have to worry about it.  Thanks,

Josef
Theodore Ts'o June 20, 2013, 6:12 p.m. UTC | #3
On Thu, Jun 20, 2013 at 01:26:09PM -0400, Josef Bacik wrote:
> I realize it's been a little bit since I've looked at jbd but I'll offer my
> opinion.  Callers of jbd2_journal_restart() may not be the ones who originated
> the handle, so doing what Jan has done with jbd2_journal_start_reserved() isn't
> going to work because all the guy at the top is going to see is an error and
> have no way to tell if his handle is invalid or not.

Yeah, that's what I meant by "it would require changing all of the
callers".

> What I would suggest is getting a unified way to mark that the handle has
> already been cleaned up and can just be free'd.

The problem though is we need to make sure none of the callers don't
try to do anything else with handle besides calling
jbd2_journal_stop().  In particular, we can't allow a call to
jbd2_journal_get_write_access(), jbd2_journal_revoke() to operate on
the handle, because its transaction pointer is (potentially) invalid.

> Then fix jbd2_journal_start_reserved() and jbd2_journal_restart() to
> set that in the handle and make jbd2_journal_stop() just free up the
> handle and reset current->journal_info but not return an error.
> It's important to not return an error from jbd2_journal_stop() so
> that it doesn't invoke the ext4 error handling stuff and you get a
> read only file system when the error may not be read only file
> system worthy.

The handle->h_aborted bit, which is currently not used, does most of
the right thing, modulo the question of the fact that
jbd2_journal_stop() will return an error.  What's important from my
perspective is that the various callers that operate on a handle check
is_handle_aborted() before trying to use the it.  We'll still need to
audit the callers to make sure there isn't some uncommon-taken code
path where ext4_handle_dirty_metadata() gets called after
ext4_journal_restart() has returned an error.

As a FAST paper once opined, "EIO: Error Handling Is Occasionally correct".  :-)

> This way you have a nice clean way of dealing with handle errors that allow you
> to pass back a real error to the caller and the caller can just do its normal
> jbd2_journal_stop() and cleanup and do its own error handling the way it feels.
> This keeps the yucky details of no longer valid handles all internal to jbd2 and
> ext4/ocfs2 don't have to worry about it.  Thanks,

Yes, that could work, although we'll need to check to make sure all of
the code paths that invoke jbd2_journal_restart() handle errors
appropriately, and don't rely on jbd2_journal_stop() returning an
error.  Thanks for your thoughts!

Regards,

	       	   	      	 		     - Ted
Younger Liu June 21, 2013, 1:29 p.m. UTC | #4
Hi Ted,

On 2013/6/20 23:55, Theodore Ts'o wrote:
> [ LKML and linux-fsdevel BCC'ed ]
> 
> On Wed, Jun 19, 2013 at 12:48:26PM +0800, Younger Liu wrote:
>> jbd2_journal_restart() would restart a handle. In this function, it
>> calls start_this_handle(). Before calling start_this_handle()?subtract
>> 1 from transaction->t_updates.
>> If start_this_handle() succeeds, transaction->t_updates increases by 1
>> in it. But if start_this_handle() fails, transaction->t_updates does
>> not increase.
>> So, when commit the handle's transaction in jbd2_journal_stop(), the
>> assertion is false, and then trigger a bug.
>> The assertion is as follows:
>> J_ASSERT(atomic_read(&transaction->t_updates) > 0) 
>>
>> Signed-off-by: Younger Liu <younger.liu@huawei.com>
> 
> Thanks for pointing out this potential problem.  Your fix isn't quite
> the right one, however.
> 
> The problem is once we get to this point, the transaction pointer may
> no longer be valid, since once we decrement t_updates, the transaction
> could start commiting, and so we should not actually dereference the
> transaction pointer after we unlock transaction->t_handle_lock.  (We
> are referencing t_tid two lines later, and technically that's a bug.
> We've just been getting lucky.)
> 
> The real issue is that by the time we call start_this_handle() in
> jbd2__journal_restart, the handle is not attached to any transaction.
> So if jbd2_journal_restart() fails, the handle has to be considered
> invalid, and the calling code should not try to use the handle at all,
> including calling jbd2_journal_stop().
> 
> Jan Kara is I believe currently on vacation but I'd really like him to
> chime in with his opinion about the best way to fix this, since he's
> also quite familiar with the jbd2 code.
> 
> Also, Jan has recently submitted changes to implement reserved handles
> (to be submitted in the next merge window), and in these new
> functions, if start_this_handle() fails when called from
> jbd2_journal_start_reserved(), the handle is left invalidated, and the
> caller of jbd2_journal_start_reserved() must not touch the handle
> again, including calling jbd2_journal_stop() --- in fact, because
> jbd2_journal_start_reserved() clears current->journal_info on failure,
> an attempt to call jbd2_journal_stop() will result in the kernel oops
> due to an assertion failure.
> 
> My inclination is to fix this in the same way, but it will require
> changing the current code paths that use jbd2_journal_restart(), and
> in some cases passing back the state that the handle is now invalid
> and should not be released via jbd2_journal_stop() is going to be
> tricky indeed.
> 
> 
> Another possible fix is to set the handle to be aborted, via
> jbd2_journal_abort_handle().  This function isn't used at all at the
> moment, but from what I can tell this should do the right thing.  The
> one unfortunate thing about this is that when jbd2_journal_stop() gets
> called, it will return EROFS, which is a misleading error code.  I'm
> guessing you're seeing this because start_this_handle() returned
> ENOMEM, correct?  We could hack around this by stashing the real error
> in the handle, and then change jbd2_journal_stop() to return that
> error instead of EROFS if it is set.
> 
> This second solution is hacky all-around, and it's also inconsistent
> with how we are doing things with jbd2_journal_start_reserved().  So
> I'm not so happy with this solution.  But it would require a lot less
> work because the fix would be isolated in the jbd2 layer.  OTOH, right
> now if the code calls jbd2_journal_stop() on the handle after a
> failure in jbd2_journal_start_reserved(), they are crashing anyway, so
> changing the code so it changes with an assertion failure doesn't make
> things any worse, and then we fix things in ext4 and ocfs2 without any
> patch interdependencies --- and this is a problem which appears to
> happen very rarely in practice.
> 
> (How did you manage to trigger this, BTW?  Was this something you
> noticed by manual code inspection?  Or are you instrumenting the
> kernel's memory allocators to occasionally fail to test our error
> paths?  Or were you running in a system with very heavy memory
> pressure?)
> 
> 						- Ted
> 

This bug was triggered by the following scenario:
In ocfs2 file system, allocate a very large disk space for a small file
with ocfs2_fallocate(), while the journal file size is 32M. 

Because there are much many journal blocks needed by jbd2_journal_restart(), 
so that nblocks is greater than journal->j_max_transaction_buffers 
in start_this_handle(), and then return -ENOSPC.

In start_this_handle():
	if (nblocks > journal->j_max_transaction_buffers) {
		printk(KERN_ERR "JBD: %s wants too many credits (%d > %d)\n",
		       current->comm, nblocks,
		       journal->j_max_transaction_buffers);
		return -ENOSPC;
	}

Dump stack:
       <ffffffffa0b04620>{jbd2:jbd2_journal_stop+0x50}
       <ffffffffa0b671a3>{ocfs2:ocfs2_commit_trans+0x23}
       <ffffffffa0b5c8ce>{ocfs2:__ocfs2_extend_allocation+0x66e}
       <ffffffffa0b5cc55>{ocfs2:ocfs2_allocate_unwritten_extents+0xc5}
       <ffffffffa0b5d315>{ocfs2:__ocfs2_change_file_space+0x3f5}
       <ffffffffa0b5d67a>{ocfs2:ocfs2_fallocate+0x7a}
       <ffffffff80127689>{do_fallocate+0x129}
       <ffffffff801276d6>{sys_fallocate+0x46}
       <ffffffff803fd423>{system_call_fastpath+0x16}
       [<00007f7283b25030>]

This problem may be because jbd2_journal_restart() was called incorrectly 
in __ocfs2_extend_allocation() which was called by ocfs2_fallocate(). 

Although I solved this question by modifing __ocfs2_extend_allocation(), 
there is also a risk in jbd2_journal_restart() for jbd2 system.
So I put this potential risk to see if there is a better ideas.

> 
> 
>> ---
>>  fs/jbd2/transaction.c |    2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
>> index 325bc01..9ddb444 100644
>> --- a/fs/jbd2/transaction.c
>> +++ b/fs/jbd2/transaction.c
>> @@ -530,6 +530,8 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, gfp_t gfp_mask)
>>  	lock_map_release(&handle->h_lockdep_map);
>>  	handle->h_buffer_credits = nblocks;
>>  	ret = start_this_handle(journal, handle, gfp_mask);
>> +	if (ret < 0)
>> +		atomic_inc(&transaction->t_updates);
>>  	return ret;
>>  }
>>  EXPORT_SYMBOL(jbd2__journal_restart);
>> -- 
>> 1.7.9.7
>>
> 
> .
>
Jan Kara June 21, 2013, 11:26 p.m. UTC | #5
On Thu 20-06-13 14:12:15, Ted Tso wrote:
> On Thu, Jun 20, 2013 at 01:26:09PM -0400, Josef Bacik wrote:
> > I realize it's been a little bit since I've looked at jbd but I'll offer my
> > opinion.  Callers of jbd2_journal_restart() may not be the ones who originated
> > the handle, so doing what Jan has done with jbd2_journal_start_reserved() isn't
> > going to work because all the guy at the top is going to see is an error and
> > have no way to tell if his handle is invalid or not.
> 
> Yeah, that's what I meant by "it would require changing all of the
> callers".
> 
> > What I would suggest is getting a unified way to mark that the handle has
> > already been cleaned up and can just be free'd.
> 
> The problem though is we need to make sure none of the callers don't
> try to do anything else with handle besides calling
> jbd2_journal_stop().  In particular, we can't allow a call to
> jbd2_journal_get_write_access(), jbd2_journal_revoke() to operate on
> the handle, because its transaction pointer is (potentially) invalid.
  I think this is the cleanest solution going forward as well.

> > Then fix jbd2_journal_start_reserved() and jbd2_journal_restart() to
> > set that in the handle and make jbd2_journal_stop() just free up the
> > handle and reset current->journal_info but not return an error.
> > It's important to not return an error from jbd2_journal_stop() so
> > that it doesn't invoke the ext4 error handling stuff and you get a
> > read only file system when the error may not be read only file
> > system worthy.
> 
> The handle->h_aborted bit, which is currently not used, does most of
> the right thing, modulo the question of the fact that
> jbd2_journal_stop() will return an error.  What's important from my
> perspective is that the various callers that operate on a handle check
> is_handle_aborted() before trying to use the it.  We'll still need to
> audit the callers to make sure there isn't some uncommon-taken code
> path where ext4_handle_dirty_metadata() gets called after
> ext4_journal_restart() has returned an error.
  Yeah, I don't see a solution where we could avoid the audit... Somewhat
comforting is that if the change to error handling will break something it
has been broken previously as well, only the bug was better hidden :)

> As a FAST paper once opined, "EIO: Error Handling Is Occasionally correct".  :-)
> 
> > This way you have a nice clean way of dealing with handle errors that allow you
> > to pass back a real error to the caller and the caller can just do its normal
> > jbd2_journal_stop() and cleanup and do its own error handling the way it feels.
> > This keeps the yucky details of no longer valid handles all internal to jbd2 and
> > ext4/ocfs2 don't have to worry about it.  Thanks,
> 
> Yes, that could work, although we'll need to check to make sure all of
> the code paths that invoke jbd2_journal_restart() handle errors
> appropriately, and don't rely on jbd2_journal_stop() returning an
> error.  Thanks for your thoughts!

								Honza
Theodore Ts'o June 23, 2013, 5:36 p.m. UTC | #6
On Fri, Jun 21, 2013 at 09:29:31PM +0800, Younger Liu wrote:
> 
> This bug was triggered by the following scenario:
> In ocfs2 file system, allocate a very large disk space for a small file
> with ocfs2_fallocate(), while the journal file size is 32M. 
> 
> Because there are much many journal blocks needed by jbd2_journal_restart(), 
> so that nblocks is greater than journal->j_max_transaction_buffers 
> in start_this_handle(), and then return -ENOSPC.

Ah, I see.  I have a patch that should prevent the kernel from
crashing in this situation, and which adds some additional checks to
make sure no one tries to use the handle after jbd2_journal_restart()
fails in this circumstance.

However, you may want to further pursue a fix in ocfs2 so you don't
actually return ENOSPC to userspace, since it is a very misleading
error message --- it's not that the file system is out of space, but
that the journal is too small for the amount of space that you are
trying to allocate using fallocate().

I would think a better way of handling this situation would be to log
a warning message that the journal is probably too small, and then to
break up the fallocate into smaller chunks, so that it can
successfully complete despite the fact that the journal was
unfortunately missized.

I'll be sending the proposed fix in a moment; could you check and see
if the patch prevents ocfs2/jbd2 from tripping over the assertion
given your test case?

Thanks,

					- Ted
Younger Liu June 25, 2013, 8:30 a.m. UTC | #7
I will check and test the pacth.

I only merge the patch about " jbd2: invalidate handle if 
jbd2_journal_restart() fails" int my source. But I do not 
merge the patch about "jbd2: Transaction reservation support...".

Does it affect the test?

On 2013/6/24 1:36, Theodore Ts'o wrote:
> On Fri, Jun 21, 2013 at 09:29:31PM +0800, Younger Liu wrote:
>>
>> This bug was triggered by the following scenario:
>> In ocfs2 file system, allocate a very large disk space for a small file
>> with ocfs2_fallocate(), while the journal file size is 32M. 
>>
>> Because there are much many journal blocks needed by jbd2_journal_restart(), 
>> so that nblocks is greater than journal->j_max_transaction_buffers 
>> in start_this_handle(), and then return -ENOSPC.
> 
> Ah, I see.  I have a patch that should prevent the kernel from
> crashing in this situation, and which adds some additional checks to
> make sure no one tries to use the handle after jbd2_journal_restart()
> fails in this circumstance.
> 
> However, you may want to further pursue a fix in ocfs2 so you don't
> actually return ENOSPC to userspace, since it is a very misleading
> error message --- it's not that the file system is out of space, but
> that the journal is too small for the amount of space that you are
> trying to allocate using fallocate().
> 
> I would think a better way of handling this situation would be to log
> a warning message that the journal is probably too small, and then to
> break up the fallocate into smaller chunks, so that it can
> successfully complete despite the fact that the journal was
> unfortunately missized.
> 
> I'll be sending the proposed fix in a moment; could you check and see
> if the patch prevents ocfs2/jbd2 from tripping over the assertion
> given your test case?
> 
> Thanks,
> 
> 					- Ted
> 
> .
>
Joel Becker June 29, 2013, 1:22 p.m. UTC | #8
On Sun, Jun 23, 2013 at 01:36:28PM -0400, Theodore Ts'o wrote:
> On Fri, Jun 21, 2013 at 09:29:31PM +0800, Younger Liu wrote:
> > 
> > This bug was triggered by the following scenario:
> > In ocfs2 file system, allocate a very large disk space for a small file
> > with ocfs2_fallocate(), while the journal file size is 32M. 
> > 
> > Because there are much many journal blocks needed by jbd2_journal_restart(), 
> > so that nblocks is greater than journal->j_max_transaction_buffers 
> > in start_this_handle(), and then return -ENOSPC.
> 
> Ah, I see.  I have a patch that should prevent the kernel from
> crashing in this situation, and which adds some additional checks to
> make sure no one tries to use the handle after jbd2_journal_restart()
> fails in this circumstance.
> 
> However, you may want to further pursue a fix in ocfs2 so you don't
> actually return ENOSPC to userspace, since it is a very misleading
> error message --- it's not that the file system is out of space, but
> that the journal is too small for the amount of space that you are
> trying to allocate using fallocate().
> 
> I would think a better way of handling this situation would be to log
> a warning message that the journal is probably too small, and then to
> break up the fallocate into smaller chunks, so that it can
> successfully complete despite the fact that the journal was
> unfortunately missized.

	Yes, this solution is a good one.

Joel

> 
> I'll be sending the proposed fix in a moment; could you check and see
> if the patch prevents ocfs2/jbd2 from tripping over the assertion
> given your test case?
> 
> Thanks,
> 
> 					- Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 325bc01..9ddb444 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -530,6 +530,8 @@  int jbd2__journal_restart(handle_t *handle, int nblocks, gfp_t gfp_mask)
 	lock_map_release(&handle->h_lockdep_map);
 	handle->h_buffer_credits = nblocks;
 	ret = start_this_handle(journal, handle, gfp_mask);
+	if (ret < 0)
+		atomic_inc(&transaction->t_updates);
 	return ret;
 }
 EXPORT_SYMBOL(jbd2__journal_restart);