[PATCH] Ext4: wait for log to commit when putting super

[PATCH] Ext4: wait for log to commit when putting super

[Josef Bacik]

There is a problem where a transaction will be committing while we're unmounting
the filesystem and you will get a panic because EXT4_SB(sb)->s_group_info has
been kfree'ed in ext4_put_super.  The commit code does the callback for the
mballoc stuff to release free'ed blocks in the transaction and panic's trying to
access s_group_info.  The fix is to wait for the transaction to finish
committing before we start cleaning up the mballoc stuff.  This patch hasn't
been tested yet, but its an obvious fix.

Signed-off-by: Josef Bacik <josef@redhat.com>
---
 fs/ext4/super.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index d4ca92a..ddd115f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -593,6 +593,7 @@ static void ext4_put_super(struct super_block *sb)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct ext4_super_block *es = sbi->s_es;
+	tid_t target;
 	int i, err;
 
 	flush_workqueue(sbi->dio_unwritten_wq);
@@ -603,6 +604,9 @@ static void ext4_put_super(struct super_block *sb)
 	if (sb->s_dirt)
 		ext4_commit_super(sb, 1);
 
+	if (jbd2_journal_start_commit(EXT4_SB(sb)->s_journal, &target))
+		jbd2_log_wait_commit(EXT4_SB(sb)->s_journal, target);
+
 	ext4_release_system_zone(sb);
 	ext4_mb_release(sb);
 	ext4_ext_release(sb);
-- 
1.6.2.5

Re: [PATCH] Ext4: wait for log to commit when putting super

[Eric Sandeen]

Josef Bacik wrote:
> There is a problem where a transaction will be committing while we're unmounting
> the filesystem and you will get a panic because EXT4_SB(sb)->s_group_info has
> been kfree'ed in ext4_put_super.  The commit code does the callback for the
> mballoc stuff to release free'ed blocks in the transaction and panic's trying to
> access s_group_info.  The fix is to wait for the transaction to finish
> committing before we start cleaning up the mballoc stuff.  This patch hasn't
> been tested yet, but its an obvious fix.

Hm, doesn't jbd2_journal_destroy already do that, but we just
call it -after- we've done ext4_mb_release which leads to freeing
EXT4_SB(sb)->s_group_info ?

Can we just do jbd2_journal_destroy() earlier before we tear
down things it may depend on?

-Eric

> Signed-off-by: Josef Bacik <josef@redhat.com>
> ---
>  fs/ext4/super.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index d4ca92a..ddd115f 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -593,6 +593,7 @@ static void ext4_put_super(struct super_block *sb)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
>  	struct ext4_super_block *es = sbi->s_es;
> +	tid_t target;
>  	int i, err;
>  
>  	flush_workqueue(sbi->dio_unwritten_wq);
> @@ -603,6 +604,9 @@ static void ext4_put_super(struct super_block *sb)
>  	if (sb->s_dirt)
>  		ext4_commit_super(sb, 1);
>  
> +	if (jbd2_journal_start_commit(EXT4_SB(sb)->s_journal, &target))
> +		jbd2_log_wait_commit(EXT4_SB(sb)->s_journal, target);
> +
>  	ext4_release_system_zone(sb);
>  	ext4_mb_release(sb);
>  	ext4_ext_release(sb);

Re: [PATCH] Ext4: wait for log to commit when putting super

[Josef Bacik]

On Mon, Dec 07, 2009 at 02:26:21PM -0600, Eric Sandeen wrote:
> Josef Bacik wrote:
> > There is a problem where a transaction will be committing while we're unmounting
> > the filesystem and you will get a panic because EXT4_SB(sb)->s_group_info has
> > been kfree'ed in ext4_put_super.  The commit code does the callback for the
> > mballoc stuff to release free'ed blocks in the transaction and panic's trying to
> > access s_group_info.  The fix is to wait for the transaction to finish
> > committing before we start cleaning up the mballoc stuff.  This patch hasn't
> > been tested yet, but its an obvious fix.
> 
> Hm, doesn't jbd2_journal_destroy already do that, but we just
> call it -after- we've done ext4_mb_release which leads to freeing
> EXT4_SB(sb)->s_group_info ?
> 
> Can we just do jbd2_journal_destroy() earlier before we tear
> down things it may depend on?
> 

That seems reasonable.  We do

        ext4_release_system_zone(sb);
        ext4_mb_release(sb);
        ext4_ext_release(sb);
        ext4_xattr_put_super(sb);

before the journal_destroy, and all thats doing is cleaning up our internal
stuff.  If we want to keep it close to what ext3 does we can just move

        ext4_release_system_zone(sb);
        ext4_mb_release(sb);
        ext4_ext_release(sb);

under the journal_destroy or just move the journal destroy above all of that
stuff.  Whichever you prefer.  Thanks,

Josef

Re: [PATCH] Ext4: wait for log to commit when putting super

[Eric Sandeen]

Josef Bacik wrote:
> On Mon, Dec 07, 2009 at 02:26:21PM -0600, Eric Sandeen wrote:
>> Josef Bacik wrote:
>>> There is a problem where a transaction will be committing while we're unmounting
>>> the filesystem and you will get a panic because EXT4_SB(sb)->s_group_info has
>>> been kfree'ed in ext4_put_super.  The commit code does the callback for the
>>> mballoc stuff to release free'ed blocks in the transaction and panic's trying to
>>> access s_group_info.  The fix is to wait for the transaction to finish
>>> committing before we start cleaning up the mballoc stuff.  This patch hasn't
>>> been tested yet, but its an obvious fix.
>> Hm, doesn't jbd2_journal_destroy already do that, but we just
>> call it -after- we've done ext4_mb_release which leads to freeing
>> EXT4_SB(sb)->s_group_info ?
>>
>> Can we just do jbd2_journal_destroy() earlier before we tear
>> down things it may depend on?
>>
> 
> That seems reasonable.  We do
> 
>         ext4_release_system_zone(sb);
>         ext4_mb_release(sb);
>         ext4_ext_release(sb);
>         ext4_xattr_put_super(sb);
> 
> before the journal_destroy, and all thats doing is cleaning up our internal
> stuff.  If we want to keep it close to what ext3 does we can just move
> 
>         ext4_release_system_zone(sb);
>         ext4_mb_release(sb);
>         ext4_ext_release(sb);
> 
> under the journal_destroy or just move the journal destroy above all of that
> stuff.  Whichever you prefer.  Thanks,

I guess it doesn't matter much to me, I just preferred rearranging
teardown order to adding more logic that I think is already taken care
of in the jbd2_journal_destroy call ..

Thanks for finding this! :)

-Eric

> Josef

Re: [PATCH] Ext4: wait for log to commit when putting super

[tytso]

On Mon, Dec 07, 2009 at 02:39:44PM -0600, Eric Sandeen wrote:
> 
> I guess it doesn't matter much to me, I just preferred rearranging
> teardown order to adding more logic that I think is already taken care
> of in the jbd2_journal_destroy call ..

Agreed.

> Thanks for finding this! :)

... and seconded!  :-)

					- Ted