[PATCH] btrfs: extent-tree.c: Remove redundant variable from btrfs_cross_ref_exist()

[PATCH] btrfs: extent-tree.c: Remove redundant variable from btrfs_cross_ref_exist()

[Misono Tomohiro]

Since commit d7df2c796d7e ("Btrfs attach delayed ref updates to
delayed ref heads"), check_delaed_ref() won't return -ENOENT.

In btrfs_cross_ref_exist(), two variable 'ret' and 'ret2' is
originally used to handle -ENOENT error case.

Since the code is not needed anymore, let's just remove 'ret2'.

Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
---
 fs/btrfs/extent-tree.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 2d9074295d7f..0c87472d5719 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3139,7 +3139,6 @@ int btrfs_cross_ref_exist(struct btrfs_root *root, u64 objectid, u64 offset,
 {
 	struct btrfs_path *path;
 	int ret;
-	int ret2;
 
 	path = btrfs_alloc_path();
 	if (!path)
@@ -3151,17 +3150,10 @@ int btrfs_cross_ref_exist(struct btrfs_root *root, u64 objectid, u64 offset,
 		if (ret && ret != -ENOENT)
 			goto out;
 
-		ret2 = check_delayed_ref(root, path, objectid,
+		ret = check_delayed_ref(root, path, objectid,
 					 offset, bytenr);
-	} while (ret2 == -EAGAIN);
+	} while (ret == -EAGAIN);
 
-	if (ret2 && ret2 != -ENOENT) {
-		ret = ret2;
-		goto out;
-	}
-
-	if (ret != -ENOENT || ret2 != -ENOENT)
-		ret = 0;
 out:
 	btrfs_free_path(path);
 	if (root->root_key.objectid == BTRFS_DATA_RELOC_TREE_OBJECTID)
-- 
2.17.1

Re: [PATCH] btrfs: extent-tree.c: Remove redundant variable from btrfs_cross_ref_exist()

[David Sterba]

On Thu, Aug 30, 2018 at 10:59:16AM +0900, Misono Tomohiro wrote:
> Since commit d7df2c796d7e ("Btrfs attach delayed ref updates to
> delayed ref heads"), check_delaed_ref() won't return -ENOENT.
> 
> In btrfs_cross_ref_exist(), two variable 'ret' and 'ret2' is
> originally used to handle -ENOENT error case.
> 
> Since the code is not needed anymore, let's just remove 'ret2'.

Good cleanup and the patch would be ok as-is. I've noticed that
check_delayed_ref now returns only two values so it might make sense to
turn it to bool and update the name of check_delayed_ref to make it
clear what the return value means in the loop.

The concern here is that adding another error code in check_delayed_ref
in the future will not be caught in btrfs_cross_ref_exist. Adding an
assert filtering only EAGAIN would be sufficient as this might miss the
extra value in case it'd be an uncommon case.

Re: [PATCH] btrfs: extent-tree.c: Remove redundant variable from btrfs_cross_ref_exist()

[Misono Tomohiro]

On 2018/09/07 0:57, David Sterba wrote:
> On Thu, Aug 30, 2018 at 10:59:16AM +0900, Misono Tomohiro wrote:
>> Since commit d7df2c796d7e ("Btrfs attach delayed ref updates to
>> delayed ref heads"), check_delaed_ref() won't return -ENOENT.
>>
>> In btrfs_cross_ref_exist(), two variable 'ret' and 'ret2' is
>> originally used to handle -ENOENT error case.
>>
>> Since the code is not needed anymore, let's just remove 'ret2'.
> 
> Good cleanup and the patch would be ok as-is. I've noticed that
> check_delayed_ref now returns only two values so it might make sense to
> turn it to bool and update the name of check_delayed_ref to make it
> clear what the return value means in the loop.

check_delaed_ref()'s return value is:
 0       ... no cross ref found in delayed ref
 1       ... cross ref found in delayed ref
 -EAGAIN ... cannot acquire lock and try again later

so I don't think we can convert it to bool directly or
do you mean we should handle -EAGAIN in check_delayed_ref()?

> The concern here is that adding another error code in check_delayed_ref
> in the future will not be caught in btrfs_cross_ref_exist. Adding an
> assert filtering only EAGAIN would be sufficient as this might miss the
> extra value in case it'd be an uncommon case.

Re: [PATCH] btrfs: extent-tree.c: Remove redundant variable from btrfs_cross_ref_exist()

[David Sterba]

On Fri, Sep 07, 2018 at 12:01:21PM +0900, Misono Tomohiro wrote:
> On 2018/09/07 0:57, David Sterba wrote:
> > On Thu, Aug 30, 2018 at 10:59:16AM +0900, Misono Tomohiro wrote:
> >> Since commit d7df2c796d7e ("Btrfs attach delayed ref updates to
> >> delayed ref heads"), check_delaed_ref() won't return -ENOENT.
> >>
> >> In btrfs_cross_ref_exist(), two variable 'ret' and 'ret2' is
> >> originally used to handle -ENOENT error case.
> >>
> >> Since the code is not needed anymore, let's just remove 'ret2'.
> > 
> > Good cleanup and the patch would be ok as-is. I've noticed that
> > check_delayed_ref now returns only two values so it might make sense to
> > turn it to bool and update the name of check_delayed_ref to make it
> > clear what the return value means in the loop.
> 
> check_delaed_ref()'s return value is:
>  0       ... no cross ref found in delayed ref
>  1       ... cross ref found in delayed ref
>  -EAGAIN ... cannot acquire lock and try again later
> 
> so I don't think we can convert it to bool directly or
> do you mean we should handle -EAGAIN in check_delayed_ref()?

Oh right, I missed there's ret = 1 in one of the branches, that makes my
point moot.