Re: [PATCH v2 2/2] btrfs: simplify error handling at btrfs_del_root_ref()

[Filipe Manana <fdmanana@kernel.org>]

On Tue, Aug 23, 2022 at 07:54:12AM +0800, Qu Wenruo wrote:
> 
> 
> On 2022/8/22 22:47, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> > 
> > At btrfs_del_root_ref() we are using two return variables, named 'ret' and
> > 'err'. This makes it harder to follow and easier to return the wrong value
> > in case an error happens - the previous patch in the series, which has the
> > subject "btrfs: fix silent failure when deleting root reference", fixed a
> > bug due to confusion created by these two variables.
> > 
> > So change the function to use a single variable for tracking the return
> > value of the function, using only 'ret', which is consistent with most of
> > the codebase.
> > 
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> Although one small nitpick inlined below.
> 
> > ---
> >   fs/btrfs/root-tree.c | 16 +++++++---------
> >   1 file changed, 7 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> > index d647cb2938c0..e1f599d7a916 100644
> > --- a/fs/btrfs/root-tree.c
> > +++ b/fs/btrfs/root-tree.c
> > @@ -337,7 +337,6 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id,
> >   	struct extent_buffer *leaf;
> >   	struct btrfs_key key;
> >   	unsigned long ptr;
> > -	int err = 0;
> >   	int ret;
> > 
> >   	path = btrfs_alloc_path();
> > @@ -350,7 +349,6 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id,
> >   again:
> >   	ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1);
> >   	if (ret < 0) {
> > -		err = ret;
> >   		goto out;
> >   	} else if (ret == 0) {
> >   		leaf = path->nodes[0];
> > @@ -360,18 +358,18 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id,
> >   		if ((btrfs_root_ref_dirid(leaf, ref) != dirid) ||
> >   		    (btrfs_root_ref_name_len(leaf, ref) != name_len) ||
> >   		    memcmp_extent_buffer(leaf, name, ptr, name_len)) {
> > -			err = -ENOENT;
> > +			ret = -ENOENT;
> >   			goto out;
> >   		}
> >   		*sequence = btrfs_root_ref_sequence(leaf, ref);
> > 
> >   		ret = btrfs_del_item(trans, tree_root, path);
> > -		if (ret) {
> > -			err = ret;
> > +		if (ret)
> >   			goto out;
> > -		}
> > -	} else
> > -		err = -ENOENT;
> > +	} else {
> > +		ret = -ENOENT;
> > +		goto out;
> > +	}
> > 
> >   	if (key.type == BTRFS_ROOT_BACKREF_KEY) {
> >   		btrfs_release_path(path);
> 
> To the if () check here can also be a cause of confusion.
> 
> Can we split it into two dedicated btrfs_search_slot() calls (instead of
> current goto again with different keys) in a separate patch?
> 
> I guess that's why the v1 version had some error got overriden, right?

The problem in v1 was because I didn't think properly.
I was under the wrong idea that we could have either one key type or
the other, that it was to deal with some old format - like the v0
backref thing.

Then I realized we got all v0 compatibility stuff removed some years ago,
and that this is something different - both keys must always exist.
In other words, it was not because of the if + goto or because of having
two variables for the return value.

I'm not sure getting rid of the if + goto logic and duplicating the deletion
is better. It would duplicate a lot of code. Either way, my intention of
this patch is really just to have a single variable for the return value
instead of two.

> 
> Thanks,
> Qu
> > @@ -383,7 +381,7 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id,
> > 
> >   out:
> >   	btrfs_free_path(path);
> > -	return err;
> > +	return ret;
> >   }
> > 
> >   /*