On Tue, Mar 03, 2020 at 08:58:22AM +0800, Qu Wenruo wrote:
> On 2020/3/3 上午2:47, Josef Bacik wrote:
> > If we fail to load an fs root, or fail to start a transaction we can
> > bail without unsetting the reloc control, which leads to problems later
> > when we free the reloc control but still have it attached to the file
> > system.
> >
> > Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> > ---
> > fs/btrfs/relocation.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> > index 507361e99316..173fc7628235 100644
> > --- a/fs/btrfs/relocation.c
> > +++ b/fs/btrfs/relocation.c
> > @@ -4678,6 +4678,7 @@ int btrfs_recover_relocation(struct btrfs_root *root)
> > fs_root = read_fs_root(fs_info, reloc_root->root_key.offset);
> > if (IS_ERR(fs_root)) {
> > err = PTR_ERR(fs_root);
> > + unset_reloc_control(rc);
> > list_add_tail(&reloc_root->root_list, &reloc_roots);
> > goto out_free;
> > }
>
>
> Shouldn't the unset_reloc_control() also get called for all related
> errors after set_reloc_control()?
It should and the patch does that but I think it could be merged into
one label so it follows the nesting. The only problem is that the reloc
control is unset between 2 calls of transaction join/commit, so the
unset would have to be called twice:
set_reloc_control()
...
join_transaction()
if (error)
goto out_unset;
...
read_fs_root()
if (error)
goto out_unset; // added by patch
...
commit_transaction()
if (error)
goto out_unset; // added by patch
...
merge_reloc_roots();
unset_reloc_control(); // unconditional
join_transaction();
if (error)
goto out_free;
commit_transaction();
clean_dirty_subvols()
out_free_unset:
unset_reloc_control(); // would be new, duplicated
out_free:
kfree(rc);
It's fine to call the function twice as it only resets the reloc_ctl
pointer, no other relocation can run at this point, but it does not look
all great. I still think that having the label-based cleanup is better
than to add the cleanup before each goto.
