Re: [PATCH 2/7] btrfs: unset reloc control if we fail to recover

[David Sterba <dsterba@suse.cz>]

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@toxicpanda.com>
> > ---
> >  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.