[PATCH 0/2] btrfs: fix lost error value deleting root reference

[PATCH 0/2] btrfs: fix lost error value deleting root reference

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Fix a silent failure in case deleting a root reference fails when searching
for an item. Then make btrfs_del_root_ref() less likely to run into such
type of bug in the future. Details in the changelogs.

Filipe Manana (2):
  btrfs: fix silent failure when deleting root reference
  btrfs: simplify error handling at btrfs_del_root_ref()

 fs/btrfs/root-tree.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

-- 
2.35.1

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

[fdmanana]

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>
---
 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) {
+	} else if (ret == 0) {
 		leaf = path->nodes[0];
 		ref = btrfs_item_ptr(leaf, path->slots[0],
 				     struct btrfs_root_ref);
-- 
2.35.1

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

[fdmanana]

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>
---
 fs/btrfs/root-tree.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index d647cb2938c0..3975a24c3fdc 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,28 @@ 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 {
+		if (key.type == BTRFS_ROOT_REF_KEY) {
+			/*
+			 * We've searched for a BTRFS_ROOT_BACKREF_KEY item and
+			 * for a BTRFS_ROOT_REF_KEY item, so error out.
+			 */
+			ret = -ENOENT;
 			goto out;
 		}
-	} else
-		err = -ENOENT;
+		/*
+		 * We have only searched for a BTRFS_ROOT_BACKREF_KEY item, so
+		 * fallback below to search for BTRFS_ROOT_REF_KEY item.
+		 */
+	}
 
 	if (key.type == BTRFS_ROOT_BACKREF_KEY) {
 		btrfs_release_path(path);
@@ -383,7 +391,7 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id,
 
 out:
 	btrfs_free_path(path);
-	return err;
+	return ret;
 }
 
 /*
-- 
2.35.1

[PATCH v2 0/2] btrfs: fix lost error value deleting root reference

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Fix a silent failure in case deleting a root reference fails when searching
for an item. Then make btrfs_del_root_ref() less likely to run into such
type of bug in the future. Details in the changelogs.

V2: Fix patch 2/2. If the BTRFS_ROOT_BACKREF_KEY item is not found, but
    the BTRFS_ROOT_REF_KEY is found, then before we would return -ENOENT
    and now we were returning 0 (unless an error happened when deleting
    the BTRFS_ROOT_REF_KEY item). Just return -ENOENT whenever one of
    the items is not found, all the callers abort the transaction if
    btrfs_del_root_ref() returns an error.

Filipe Manana (2):
  btrfs: fix silent failure when deleting root reference
  btrfs: simplify error handling at btrfs_del_root_ref()

 fs/btrfs/root-tree.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

-- 
2.35.1

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

[fdmanana]

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>
---
 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) {
+	} else if (ret == 0) {
 		leaf = path->nodes[0];
 		ref = btrfs_item_ptr(leaf, path->slots[0],
 				     struct btrfs_root_ref);
-- 
2.35.1

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

[fdmanana]

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>
---
 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);
@@ -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;
 }
 
 /*
-- 
2.35.1

Re: [PATCH v2 0/2] btrfs: fix lost error value deleting root reference

[David Sterba]

On Mon, Aug 22, 2022 at 03:47:08PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Fix a silent failure in case deleting a root reference fails when searching
> for an item. Then make btrfs_del_root_ref() less likely to run into such
> type of bug in the future. Details in the changelogs.
> 
> V2: Fix patch 2/2. If the BTRFS_ROOT_BACKREF_KEY item is not found, but
>     the BTRFS_ROOT_REF_KEY is found, then before we would return -ENOENT
>     and now we were returning 0 (unless an error happened when deleting
>     the BTRFS_ROOT_REF_KEY item). Just return -ENOENT whenever one of
>     the items is not found, all the callers abort the transaction if
>     btrfs_del_root_ref() returns an error.
> 
> Filipe Manana (2):
>   btrfs: fix silent failure when deleting root reference
>   btrfs: simplify error handling at btrfs_del_root_ref()

Added to misc-next, thanks.

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

[Qu Wenruo]

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);

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

[Qu Wenruo]

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?

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;
>   }
>
>   /*

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

[Filipe Manana]

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;
> >   }
> > 
> >   /*

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

[Filipe Manana]

On Tue, Aug 23, 2022 at 12:47 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> 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.

Yes, it's not about correctness. It's just my preferred style.
I find it more intuitive to have a single if-else that tests the same
variable instead
of having it tested by two distinct if statements.

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

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

[David Sterba]

On Tue, Aug 23, 2022 at 09:11:55AM +0100, Filipe Manana wrote:
> On Tue, Aug 23, 2022 at 12:47 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> > > ---
> > >   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.
> 
> Yes, it's not about correctness. It's just my preferred style.
> I find it more intuitive to have a single if-else that tests the same
> variable instead
> of having it tested by two distinct if statements.

Agreed with that style.