Re: [PATCH v2 1/2] btrfs: fix silent failure when deleting root reference

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2022/8/22 22:47, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> At btrfs_del_root_ref(), if btrfs_search_slot() returns an error, we end
> up returning from the function with a value of 0 (success). This happens
> because the function returns the value stored in the variable 'err', which
> is 0, while the error value we got from btrfs_search_slot() is stored in
> the 'ret' variable.
>
> So fix it by setting 'err' with the error value.
>
> Fixes: 8289ed9f93bef2 ("btrfs: replace the BUG_ON in btrfs_del_root_ref with proper error handling")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks for catching this,
Qu
> ---
>   fs/btrfs/root-tree.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> index a64b26b16904..d647cb2938c0 100644
> --- a/fs/btrfs/root-tree.c
> +++ b/fs/btrfs/root-tree.c
> @@ -349,9 +349,10 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id,
>   	key.offset = ref_id;
>   again:
>   	ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		err = ret;
>   		goto out;
> -	if (ret == 0) {

Just a small nitpick here, since above if (ret < 0) branch will call
"goto out", we don't need the "else" branch.
The old "if (ret == 0) {" should be good enough.

Thanks,
Qu

> +	} else if (ret == 0) {
>   		leaf = path->nodes[0];
>   		ref = btrfs_item_ptr(leaf, path->slots[0],
>   				     struct btrfs_root_ref);