Re: [PATCH 1/2] Btrfs: restore restriper state on all mounts

[Ilya Dryomov <idryomov@gmail.com>]

First of all, thanks for reviewing!

On Tue, Jun 26, 2012 at 06:17:39PM +0200, David Sterba wrote:
> On Fri, Jun 22, 2012 at 09:24:12PM +0300, Ilya Dryomov wrote:
> > Fix a bug that triggered asserts in btrfs_balance() in both normal and
> > resume modes -- restriper state was not properly restored on read-only
> > mounts.  This factors out resuming code from btrfs_restore_balance(),
> > which is now also called earlier in the mount sequence to avoid the
> > problem of some early writes getting the old profile.
> > 
> > Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> > ---
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index 77872da..dae7cd6 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -2492,9 +2497,6 @@ retry_root_backup:
> >  			err = btrfs_orphan_cleanup(fs_info->tree_root);
> >  		up_read(&fs_info->cleanup_work_sem);
> >  
> > -		if (!err)
> > -			err = btrfs_recover_balance(fs_info->tree_root);
> > -
> >  		if (err) {
> >  			close_ctree(tree_root);
> >  			return err;
> > @@ -2518,6 +2520,9 @@ fail_cleaner:
> >  fail_block_groups:
> >  	btrfs_free_block_groups(fs_info);
> >  
> > +fail_balance_ctl:
> > +	kfree(fs_info->balance_ctl);
> 
> I think you need to set fs_info->balance_ctl to NULL, otherwise this
> could lead to double free from free_fs_info. I was looking along the

Yes, I do.  I meant to call unset_balance_control(fs_info) there, but
changed it to kfree(), because of the BUG_ON() in the former.

unset_balance_control(), of course, sets ->balance_ctl to NULL ;)

> call paths and didn't see free_fs_info called on the mount failure path:
> 
> vfs->mount
>   btrfs_mount
>     btrfs_fill_super
>       open_ctree
>         (recover balance fails, frees ctl)
> 
> error is propagated back to vfs, no other fs callback is done (like
> kill_super which does call free_fs_info).
> 
> The only exit path that is not going through free_fs_info is after error
> from btrfs_fill_super, and this can fail from various reasons.
> 
> Either I'm missing something, or we leak a btrfs_fs_info every time a
> mount fails ...

No, we don't, you just missed it.  It's freed from btrfs_kill_super(),
which is called from deactivate_locked_super() after btrfs_fill_super()
errors out.

> 
> 
> Back to your patch, apart from the balance_ctl pointer reset, both are
> ok and given the number of bug reports [useless padding text here]
> 
>   this should go to 3.5-rc.

Thanks,

		Ilya