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@xxxxxxxxx>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
