[PATCH v3 0/6] part3 trivial adjustments for return variable coding style

[PATCH v3 0/6] part3 trivial adjustments for return variable coding style

[Anand Jain]

This is part 3 of the series, containing renaming with optimization of the
return variable.

Some of the patches are new it wasn't part of v1 or v2. The new patches follow
verb-first format for titles. Older patches not renamed for backward reference.

Patchset passed tests -g quick without regressions, sending them first.

Patch 3/6 and 4/6 can be merged; they are separated for easier diff.

v2 part2:
  https://lore.kernel.org/linux-btrfs/cover.1713370756.git.anand.jain@oracle.com/
v1:
  https://lore.kernel.org/linux-btrfs/cover.1710857863.git.anand.jain@oracle.com/

Anand Jain (6):
  btrfs: btrfs_cleanup_fs_roots handle ret variable
  btrfs: simplify ret in btrfs_recover_relocation
  btrfs: rename ret in btrfs_recover_relocation
  btrfs: rename err in btrfs_recover_relocation
  btrfs: btrfs_drop_snapshot optimize return variable
  btrfs: rename and optimize return variable in btrfs_find_orphan_roots

 fs/btrfs/disk-io.c     | 38 ++++++++++++++--------------
 fs/btrfs/extent-tree.c | 48 ++++++++++++++++++------------------
 fs/btrfs/relocation.c  | 56 +++++++++++++++++++-----------------------
 fs/btrfs/root-tree.c   | 32 ++++++++++++------------
 4 files changed, 85 insertions(+), 89 deletions(-)

-- 
2.38.1

[PATCH v3 1/6] btrfs: btrfs_cleanup_fs_roots handle ret variable

[Anand Jain]

Since err represents the function return value, rename it as ret,
and rename the original ret, which serves as a helper return value,
to found. Also, optimize the code to continue call btrfs_put_root()
for the rest of the root if even after btrfs_orphan_cleanup() returns
error.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3: Add a code comment.
v2: Rename to 'found' instead of 'ret2' (Josef).
    Call btrfs_put_root() in the while-loop, avoids use of the variable
	'found' outside of the while loop (Qu).
    Use 'unsigned int i' instead of 'int' (Goffredo).

 fs/btrfs/disk-io.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index a91a8056758a..d38cf973b02a 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2925,22 +2925,23 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
 {
 	u64 root_objectid = 0;
 	struct btrfs_root *gang[8];
-	int i = 0;
-	int err = 0;
-	unsigned int ret = 0;
+	int ret = 0;
 
 	while (1) {
+		unsigned int i;
+		unsigned int found;
+
 		spin_lock(&fs_info->fs_roots_radix_lock);
-		ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
+		found = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
 					     (void **)gang, root_objectid,
 					     ARRAY_SIZE(gang));
-		if (!ret) {
+		if (!found) {
 			spin_unlock(&fs_info->fs_roots_radix_lock);
 			break;
 		}
-		root_objectid = btrfs_root_id(gang[ret - 1]) + 1;
+		root_objectid = btrfs_root_id(gang[found - 1]) + 1;
 
-		for (i = 0; i < ret; i++) {
+		for (i = 0; i < found; i++) {
 			/* Avoid to grab roots in dead_roots. */
 			if (btrfs_root_refs(&gang[i]->root_item) == 0) {
 				gang[i] = NULL;
@@ -2951,24 +2952,25 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
 		}
 		spin_unlock(&fs_info->fs_roots_radix_lock);
 
-		for (i = 0; i < ret; i++) {
+		for (i = 0; i < found; i++) {
 			if (!gang[i])
 				continue;
 			root_objectid = btrfs_root_id(gang[i]);
-			err = btrfs_orphan_cleanup(gang[i]);
-			if (err)
-				goto out;
+			/*
+			 * Continue to release the remaining roots after the first
+			 * error without cleanup and preserve the first error
+			 * for the return.
+			 */
+			if (!ret)
+				ret = btrfs_orphan_cleanup(gang[i]);
 			btrfs_put_root(gang[i]);
 		}
+		if (ret)
+			break;
+
 		root_objectid++;
 	}
-out:
-	/* Release the uncleaned roots due to error. */
-	for (; i < ret; i++) {
-		if (gang[i])
-			btrfs_put_root(gang[i]);
-	}
-	return err;
+	return ret;
 }
 
 /*
-- 
2.38.1

[PATCH v3 2/6] btrfs: simplify ret in btrfs_recover_relocation

[Anand Jain]

In the function btrfs_recover_relocation(), currently the variable 'err'
carries the return value and 'ret' holds the intermediary return value.
However, in some lines, we don't need this two-step approach; we can
directly use 'err'. So, optimize them, which requires reinitializing 'err'
to zero at two locations.

This is a preparatory patch to fix the code style by renaming 'err'
to 'ret'.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3: splits optimization part from the rename part.

 fs/btrfs/relocation.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 8b24bb5a0aa1..bfcecf6701a0 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4234,17 +4234,16 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 	key.offset = (u64)-1;
 
 	while (1) {
-		ret = btrfs_search_slot(NULL, fs_info->tree_root, &key,
+		err = btrfs_search_slot(NULL, fs_info->tree_root, &key,
 					path, 0, 0);
-		if (ret < 0) {
-			err = ret;
+		if (err < 0)
 			goto out;
-		}
-		if (ret > 0) {
+		if (err > 0) {
 			if (path->slots[0] == 0)
 				break;
 			path->slots[0]--;
 		}
+		err = 0;
 		leaf = path->nodes[0];
 		btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
 		btrfs_release_path(path);
@@ -4266,16 +4265,13 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 			fs_root = btrfs_get_fs_root(fs_info,
 					reloc_root->root_key.offset, false);
 			if (IS_ERR(fs_root)) {
-				ret = PTR_ERR(fs_root);
-				if (ret != -ENOENT) {
-					err = ret;
+				err = PTR_ERR(fs_root);
+				if (err != -ENOENT)
 					goto out;
-				}
-				ret = mark_garbage_root(reloc_root);
-				if (ret < 0) {
-					err = ret;
+				err = mark_garbage_root(reloc_root);
+				if (err < 0)
 					goto out;
-				}
+				err = 0;
 			} else {
 				btrfs_put_root(fs_root);
 			}
@@ -4297,11 +4293,9 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 		goto out;
 	}
 
-	ret = reloc_chunk_start(fs_info);
-	if (ret < 0) {
-		err = ret;
+	err = reloc_chunk_start(fs_info);
+	if (err < 0)
 		goto out_end;
-	}
 
 	rc->extent_root = btrfs_extent_root(fs_info, 0);
 
-- 
2.38.1

[PATCH v3 3/6] btrfs: rename ret in btrfs_recover_relocation

[Anand Jain]

A preparatory patch to rename 'err' to 'ret', but ret is already used as an
intermediary return value, so first rename 'ret' to 'ret2'.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
This patch can be merged with the next one; they were separated for easier
review. Thx.

v3: new

 fs/btrfs/relocation.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index bfcecf6701a0..e24deb7f0504 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4221,7 +4221,7 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 	struct extent_buffer *leaf;
 	struct reloc_control *rc = NULL;
 	struct btrfs_trans_handle *trans;
-	int ret;
+	int ret2;
 	int err = 0;
 
 	path = btrfs_alloc_path();
@@ -4356,9 +4356,9 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 	}
 	err = btrfs_commit_transaction(trans);
 out_clean:
-	ret = clean_dirty_subvols(rc);
-	if (ret < 0 && !err)
-		err = ret;
+	ret2 = clean_dirty_subvols(rc);
+	if (ret2 < 0 && !err)
+		err = ret2;
 out_unset:
 	unset_reloc_control(rc);
 out_end:
-- 
2.38.1

[PATCH v3 4/6] btrfs: rename err in btrfs_recover_relocation

[Anand Jain]

Fix coding style: rename the return variable to 'ret' in the function
btrfs_recover_relocation instead of 'err'.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3: new 

 fs/btrfs/relocation.c | 56 +++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index e24deb7f0504..ccaa359648ef 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4222,7 +4222,7 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 	struct reloc_control *rc = NULL;
 	struct btrfs_trans_handle *trans;
 	int ret2;
-	int err = 0;
+	int ret = 0;
 
 	path = btrfs_alloc_path();
 	if (!path)
@@ -4234,16 +4234,16 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 	key.offset = (u64)-1;
 
 	while (1) {
-		err = btrfs_search_slot(NULL, fs_info->tree_root, &key,
+		ret = btrfs_search_slot(NULL, fs_info->tree_root, &key,
 					path, 0, 0);
-		if (err < 0)
+		if (ret < 0)
 			goto out;
-		if (err > 0) {
+		if (ret > 0) {
 			if (path->slots[0] == 0)
 				break;
 			path->slots[0]--;
 		}
-		err = 0;
+		ret = 0;
 		leaf = path->nodes[0];
 		btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
 		btrfs_release_path(path);
@@ -4254,7 +4254,7 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 
 		reloc_root = btrfs_read_tree_root(fs_info->tree_root, &key);
 		if (IS_ERR(reloc_root)) {
-			err = PTR_ERR(reloc_root);
+			ret = PTR_ERR(reloc_root);
 			goto out;
 		}
 
@@ -4265,13 +4265,13 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 			fs_root = btrfs_get_fs_root(fs_info,
 					reloc_root->root_key.offset, false);
 			if (IS_ERR(fs_root)) {
-				err = PTR_ERR(fs_root);
-				if (err != -ENOENT)
+				ret = PTR_ERR(fs_root);
+				if (ret != -ENOENT)
 					goto out;
-				err = mark_garbage_root(reloc_root);
-				if (err < 0)
+				ret = mark_garbage_root(reloc_root);
+				if (ret < 0)
 					goto out;
-				err = 0;
+				ret = 0;
 			} else {
 				btrfs_put_root(fs_root);
 			}
@@ -4289,12 +4289,12 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 
 	rc = alloc_reloc_control(fs_info);
 	if (!rc) {
-		err = -ENOMEM;
+		ret = -ENOMEM;
 		goto out;
 	}
 
-	err = reloc_chunk_start(fs_info);
-	if (err < 0)
+	ret = reloc_chunk_start(fs_info);
+	if (ret < 0)
 		goto out_end;
 
 	rc->extent_root = btrfs_extent_root(fs_info, 0);
@@ -4303,7 +4303,7 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 
 	trans = btrfs_join_transaction(rc->extent_root);
 	if (IS_ERR(trans)) {
-		err = PTR_ERR(trans);
+		ret = PTR_ERR(trans);
 		goto out_unset;
 	}
 
@@ -4323,15 +4323,15 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 		fs_root = btrfs_get_fs_root(fs_info, reloc_root->root_key.offset,
 					    false);
 		if (IS_ERR(fs_root)) {
-			err = PTR_ERR(fs_root);
+			ret = PTR_ERR(fs_root);
 			list_add_tail(&reloc_root->root_list, &reloc_roots);
 			btrfs_end_transaction(trans);
 			goto out_unset;
 		}
 
-		err = __add_reloc_root(reloc_root);
-		ASSERT(err != -EEXIST);
-		if (err) {
+		ret = __add_reloc_root(reloc_root);
+		ASSERT(ret != -EEXIST);
+		if (ret) {
 			list_add_tail(&reloc_root->root_list, &reloc_roots);
 			btrfs_put_root(fs_root);
 			btrfs_end_transaction(trans);
@@ -4341,8 +4341,8 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 		btrfs_put_root(fs_root);
 	}
 
-	err = btrfs_commit_transaction(trans);
-	if (err)
+	ret = btrfs_commit_transaction(trans);
+	if (ret)
 		goto out_unset;
 
 	merge_reloc_roots(rc);
@@ -4351,14 +4351,14 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 
 	trans = btrfs_join_transaction(rc->extent_root);
 	if (IS_ERR(trans)) {
-		err = PTR_ERR(trans);
+		ret = PTR_ERR(trans);
 		goto out_clean;
 	}
-	err = btrfs_commit_transaction(trans);
+	ret = btrfs_commit_transaction(trans);
 out_clean:
 	ret2 = clean_dirty_subvols(rc);
-	if (ret2 < 0 && !err)
-		err = ret2;
+	if (ret2 < 0 && !ret)
+		ret = ret2;
 out_unset:
 	unset_reloc_control(rc);
 out_end:
@@ -4369,14 +4369,14 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 
 	btrfs_free_path(path);
 
-	if (err == 0) {
+	if (ret == 0) {
 		/* cleanup orphan inode in data relocation tree */
 		fs_root = btrfs_grab_root(fs_info->data_reloc_root);
 		ASSERT(fs_root);
-		err = btrfs_orphan_cleanup(fs_root);
+		ret = btrfs_orphan_cleanup(fs_root);
 		btrfs_put_root(fs_root);
 	}
-	return err;
+	return ret;
 }
 
 /*
-- 
2.38.1

[PATCH v3 5/6] btrfs: btrfs_drop_snapshot optimize return variable

[Anand Jain]

Drop the variable 'err', reuse the variable 'ret' by reinitializing it to
zero where necessary.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3: Fix comment formatting.
v2: handle return error better, no need of original 'ret'. (Josef).

 fs/btrfs/extent-tree.c | 48 +++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 47d48233b592..578a0e2ec884 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5833,8 +5833,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 	struct btrfs_root_item *root_item = &root->root_item;
 	struct walk_control *wc;
 	struct btrfs_key key;
-	int err = 0;
-	int ret;
+	int ret = 0;
 	int level;
 	bool root_dropped = false;
 	bool unfinished_drop = false;
@@ -5843,14 +5842,14 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 
 	path = btrfs_alloc_path();
 	if (!path) {
-		err = -ENOMEM;
+		ret = -ENOMEM;
 		goto out;
 	}
 
 	wc = kzalloc(sizeof(*wc), GFP_NOFS);
 	if (!wc) {
 		btrfs_free_path(path);
-		err = -ENOMEM;
+		ret = -ENOMEM;
 		goto out;
 	}
 
@@ -5863,12 +5862,12 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 	else
 		trans = btrfs_start_transaction(tree_root, 0);
 	if (IS_ERR(trans)) {
-		err = PTR_ERR(trans);
+		ret = PTR_ERR(trans);
 		goto out_free;
 	}
 
-	err = btrfs_run_delayed_items(trans);
-	if (err)
+	ret = btrfs_run_delayed_items(trans);
+	if (ret)
 		goto out_end_trans;
 
 	/*
@@ -5899,11 +5898,11 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 		path->lowest_level = level;
 		ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
 		path->lowest_level = 0;
-		if (ret < 0) {
-			err = ret;
+		if (ret < 0)
 			goto out_end_trans;
-		}
+
 		WARN_ON(ret > 0);
+		ret = 0;
 
 		/*
 		 * unlock our path, this is safe because only this
@@ -5916,14 +5915,17 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 			btrfs_tree_lock(path->nodes[level]);
 			path->locks[level] = BTRFS_WRITE_LOCK;
 
+			/*
+			 * btrfs_lookup_extent_info() returns 0 for success,
+			 * or < 0 for error.
+			 */
 			ret = btrfs_lookup_extent_info(trans, fs_info,
 						path->nodes[level]->start,
 						level, 1, &wc->refs[level],
 						&wc->flags[level], NULL);
-			if (ret < 0) {
-				err = ret;
+			if (ret < 0)
 				goto out_end_trans;
-			}
+
 			BUG_ON(wc->refs[level] == 0);
 
 			if (level == btrfs_root_drop_level(root_item))
@@ -5949,19 +5951,18 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 		ret = walk_down_tree(trans, root, path, wc);
 		if (ret < 0) {
 			btrfs_abort_transaction(trans, ret);
-			err = ret;
 			break;
 		}
 
 		ret = walk_up_tree(trans, root, path, wc, BTRFS_MAX_LEVEL);
 		if (ret < 0) {
 			btrfs_abort_transaction(trans, ret);
-			err = ret;
 			break;
 		}
 
 		if (ret > 0) {
 			BUG_ON(wc->stage != DROP_REFERENCE);
+			ret = 0;
 			break;
 		}
 
@@ -5983,7 +5984,6 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 						root_item);
 			if (ret) {
 				btrfs_abort_transaction(trans, ret);
-				err = ret;
 				goto out_end_trans;
 			}
 
@@ -5994,7 +5994,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 			if (!for_reloc && btrfs_need_cleaner_sleep(fs_info)) {
 				btrfs_debug(fs_info,
 					    "drop snapshot early exit");
-				err = -EAGAIN;
+				ret = -EAGAIN;
 				goto out_free;
 			}
 
@@ -6008,19 +6008,18 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 			else
 				trans = btrfs_start_transaction(tree_root, 0);
 			if (IS_ERR(trans)) {
-				err = PTR_ERR(trans);
+				ret = PTR_ERR(trans);
 				goto out_free;
 			}
 		}
 	}
 	btrfs_release_path(path);
-	if (err)
+	if (ret)
 		goto out_end_trans;
 
 	ret = btrfs_del_root(trans, &root->root_key);
 	if (ret) {
 		btrfs_abort_transaction(trans, ret);
-		err = ret;
 		goto out_end_trans;
 	}
 
@@ -6029,10 +6028,11 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 				      NULL, NULL);
 		if (ret < 0) {
 			btrfs_abort_transaction(trans, ret);
-			err = ret;
 			goto out_end_trans;
 		} else if (ret > 0) {
-			/* if we fail to delete the orphan item this time
+			ret = 0;
+			/*
+			 * If we fail to delete the orphan item this time
 			 * around, it'll get picked up the next time.
 			 *
 			 * The most common failure here is just -ENOENT.
@@ -6067,7 +6067,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 	 * We were an unfinished drop root, check to see if there are any
 	 * pending, and if not clear and wake up any waiters.
 	 */
-	if (!err && unfinished_drop)
+	if (!ret && unfinished_drop)
 		btrfs_maybe_wake_unfinished_drop(fs_info);
 
 	/*
@@ -6079,7 +6079,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 	 */
 	if (!for_reloc && !root_dropped)
 		btrfs_add_dead_root(root);
-	return err;
+	return ret;
 }
 
 /*
-- 
2.38.1

[PATCH v3 6/6] btrfs: rename and optimize return variable in btrfs_find_orphan_roots

[Anand Jain]

The variable err is the actual return value of this function, and the
variable ret is a helper variable for err, which actually is not
needed and can be handled just by err, which is renamed to ret.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3: drop ret2 as there is no need for it.
v2: n/a
 fs/btrfs/root-tree.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index 33962671a96c..c11b0bccf513 100644
--- a/fs/btrfs/root-tree.c
+++ b/fs/btrfs/root-tree.c
@@ -220,8 +220,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
 	struct btrfs_path *path;
 	struct btrfs_key key;
 	struct btrfs_root *root;
-	int err = 0;
-	int ret;
+	int ret = 0;
 
 	path = btrfs_alloc_path();
 	if (!path)
@@ -235,18 +234,19 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
 		u64 root_objectid;
 
 		ret = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0);
-		if (ret < 0) {
-			err = ret;
+		if (ret < 0)
 			break;
-		}
+		ret = 0;
 
 		leaf = path->nodes[0];
 		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
 			ret = btrfs_next_leaf(tree_root, path);
 			if (ret < 0)
-				err = ret;
-			if (ret != 0)
 				break;
+			if (ret > 0) {
+				ret = 0;
+				break;
+			}
 			leaf = path->nodes[0];
 		}
 
@@ -261,26 +261,26 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
 		key.offset++;
 
 		root = btrfs_get_fs_root(fs_info, root_objectid, false);
-		err = PTR_ERR_OR_ZERO(root);
-		if (err && err != -ENOENT) {
+		ret = PTR_ERR_OR_ZERO(root);
+		if (ret && ret != -ENOENT) {
 			break;
-		} else if (err == -ENOENT) {
+		} else if (ret == -ENOENT) {
 			struct btrfs_trans_handle *trans;
 
 			btrfs_release_path(path);
 
 			trans = btrfs_join_transaction(tree_root);
 			if (IS_ERR(trans)) {
-				err = PTR_ERR(trans);
-				btrfs_handle_fs_error(fs_info, err,
+				ret = PTR_ERR(trans);
+				btrfs_handle_fs_error(fs_info, ret,
 					    "Failed to start trans to delete orphan item");
 				break;
 			}
-			err = btrfs_del_orphan_item(trans, tree_root,
+			ret = btrfs_del_orphan_item(trans, tree_root,
 						    root_objectid);
 			btrfs_end_transaction(trans);
-			if (err) {
-				btrfs_handle_fs_error(fs_info, err,
+			if (ret) {
+				btrfs_handle_fs_error(fs_info, ret,
 					    "Failed to delete root orphan item");
 				break;
 			}
@@ -311,7 +311,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
 	}
 
 	btrfs_free_path(path);
-	return err;
+	return ret;
 }
 
 /* drop the root item for 'key' from the tree root */
-- 
2.38.1

Re: [PATCH v3 0/6] part3 trivial adjustments for return variable coding style

[Anand Jain]

Any RB please?

Thanks, Anand


On 5/16/24 19:12, Anand Jain wrote:
> This is part 3 of the series, containing renaming with optimization of the
> return variable.
> 
> Some of the patches are new it wasn't part of v1 or v2. The new patches follow
> verb-first format for titles. Older patches not renamed for backward reference.
> 
> Patchset passed tests -g quick without regressions, sending them first.
> 
> Patch 3/6 and 4/6 can be merged; they are separated for easier diff.
> 
> v2 part2:
>    https://lore.kernel.org/linux-btrfs/cover.1713370756.git.anand.jain@oracle.com/
> v1:
>    https://lore.kernel.org/linux-btrfs/cover.1710857863.git.anand.jain@oracle.com/
> 
> Anand Jain (6):
>    btrfs: btrfs_cleanup_fs_roots handle ret variable
>    btrfs: simplify ret in btrfs_recover_relocation
>    btrfs: rename ret in btrfs_recover_relocation
>    btrfs: rename err in btrfs_recover_relocation
>    btrfs: btrfs_drop_snapshot optimize return variable
>    btrfs: rename and optimize return variable in btrfs_find_orphan_roots
> 
>   fs/btrfs/disk-io.c     | 38 ++++++++++++++--------------
>   fs/btrfs/extent-tree.c | 48 ++++++++++++++++++------------------
>   fs/btrfs/relocation.c  | 56 +++++++++++++++++++-----------------------
>   fs/btrfs/root-tree.c   | 32 ++++++++++++------------
>   4 files changed, 85 insertions(+), 89 deletions(-)
> 

Re: [PATCH v3 1/6] btrfs: btrfs_cleanup_fs_roots handle ret variable

[David Sterba]

On Thu, May 16, 2024 at 07:12:10PM +0800, Anand Jain wrote:
> Since err represents the function return value, rename it as ret,
> and rename the original ret, which serves as a helper return value,
> to found. Also, optimize the code to continue call btrfs_put_root()
> for the rest of the root if even after btrfs_orphan_cleanup() returns
> error.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v3: Add a code comment.
> v2: Rename to 'found' instead of 'ret2' (Josef).
>     Call btrfs_put_root() in the while-loop, avoids use of the variable
> 	'found' outside of the while loop (Qu).
>     Use 'unsigned int i' instead of 'int' (Goffredo).
> 
>  fs/btrfs/disk-io.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index a91a8056758a..d38cf973b02a 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2925,22 +2925,23 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
>  {
>  	u64 root_objectid = 0;
>  	struct btrfs_root *gang[8];
> -	int i = 0;
> -	int err = 0;
> -	unsigned int ret = 0;
> +	int ret = 0;
>  
>  	while (1) {
> +		unsigned int i;
> +		unsigned int found;
> +
>  		spin_lock(&fs_info->fs_roots_radix_lock);
> -		ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
> +		found = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
>  					     (void **)gang, root_objectid,
>  					     ARRAY_SIZE(gang));
> -		if (!ret) {
> +		if (!found) {
>  			spin_unlock(&fs_info->fs_roots_radix_lock);
>  			break;
>  		}
> -		root_objectid = btrfs_root_id(gang[ret - 1]) + 1;
> +		root_objectid = btrfs_root_id(gang[found - 1]) + 1;
>  
> -		for (i = 0; i < ret; i++) {
> +		for (i = 0; i < found; i++) {

You could also move the declaration of 'i' to the for loop as you move
the other definition anyway.

>  			/* Avoid to grab roots in dead_roots. */
>  			if (btrfs_root_refs(&gang[i]->root_item) == 0) {
>  				gang[i] = NULL;
> @@ -2951,24 +2952,25 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
>  		}
>  		spin_unlock(&fs_info->fs_roots_radix_lock);
>  
> -		for (i = 0; i < ret; i++) {
> +		for (i = 0; i < found; i++) {
>  			if (!gang[i])
>  				continue;
>  			root_objectid = btrfs_root_id(gang[i]);
> -			err = btrfs_orphan_cleanup(gang[i]);
> -			if (err)
> -				goto out;
> +			/*
> +			 * Continue to release the remaining roots after the first
> +			 * error without cleanup and preserve the first error
> +			 * for the return.
> +			 */
> +			if (!ret)
> +				ret = btrfs_orphan_cleanup(gang[i]);
>  			btrfs_put_root(gang[i]);
>  		}
> +		if (ret)
> +			break;
> +
>  		root_objectid++;
>  	}
> -out:
> -	/* Release the uncleaned roots due to error. */
> -	for (; i < ret; i++) {
> -		if (gang[i])
> -			btrfs_put_root(gang[i]);
> -	}
> -	return err;
> +	return ret;
>  }
>  
>  /*
> -- 
> 2.38.1
> 

Re: [PATCH v3 6/6] btrfs: rename and optimize return variable in btrfs_find_orphan_roots

[David Sterba]

On Thu, May 16, 2024 at 07:12:15PM +0800, Anand Jain wrote:
> The variable err is the actual return value of this function, and the
> variable ret is a helper variable for err, which actually is not
> needed and can be handled just by err, which is renamed to ret.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v3: drop ret2 as there is no need for it.
> v2: n/a
>  fs/btrfs/root-tree.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> index 33962671a96c..c11b0bccf513 100644
> --- a/fs/btrfs/root-tree.c
> +++ b/fs/btrfs/root-tree.c
> @@ -220,8 +220,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
>  	struct btrfs_path *path;
>  	struct btrfs_key key;
>  	struct btrfs_root *root;
> -	int err = 0;
> -	int ret;
> +	int ret = 0;
>  
>  	path = btrfs_alloc_path();
>  	if (!path)
> @@ -235,18 +234,19 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
>  		u64 root_objectid;
>  
>  		ret = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0);
> -		if (ret < 0) {
> -			err = ret;
> +		if (ret < 0)
>  			break;
> -		}
> +		ret = 0;

Should this be handled when ret > 0? This would be unexpected and
probably means a corruption but simply overwriting the value does not
seem right.

>  
>  		leaf = path->nodes[0];
>  		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
>  			ret = btrfs_next_leaf(tree_root, path);
>  			if (ret < 0)
> -				err = ret;
> -			if (ret != 0)
>  				break;
> +			if (ret > 0) {
> +				ret = 0;
> +				break;
> +			}
>  			leaf = path->nodes[0];
>  		}
>  
> @@ -261,26 +261,26 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
>  		key.offset++;
>  
>  		root = btrfs_get_fs_root(fs_info, root_objectid, false);
> -		err = PTR_ERR_OR_ZERO(root);
> -		if (err && err != -ENOENT) {
> +		ret = PTR_ERR_OR_ZERO(root);
> +		if (ret && ret != -ENOENT) {
>  			break;
> -		} else if (err == -ENOENT) {
> +		} else if (ret == -ENOENT) {
>  			struct btrfs_trans_handle *trans;
>  
>  			btrfs_release_path(path);
>  
>  			trans = btrfs_join_transaction(tree_root);
>  			if (IS_ERR(trans)) {
> -				err = PTR_ERR(trans);
> -				btrfs_handle_fs_error(fs_info, err,
> +				ret = PTR_ERR(trans);
> +				btrfs_handle_fs_error(fs_info, ret,
>  					    "Failed to start trans to delete orphan item");
>  				break;
>  			}
> -			err = btrfs_del_orphan_item(trans, tree_root,
> +			ret = btrfs_del_orphan_item(trans, tree_root,
>  						    root_objectid);
>  			btrfs_end_transaction(trans);
> -			if (err) {
> -				btrfs_handle_fs_error(fs_info, err,
> +			if (ret) {
> +				btrfs_handle_fs_error(fs_info, ret,
>  					    "Failed to delete root orphan item");
>  				break;
>  			}
> @@ -311,7 +311,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
>  	}
>  
>  	btrfs_free_path(path);
> -	return err;
> +	return ret;
>  }
>  
>  /* drop the root item for 'key' from the tree root */
> -- 
> 2.38.1
> 

Re: [PATCH v3 0/6] part3 trivial adjustments for return variable coding style

[David Sterba]

On Thu, May 16, 2024 at 07:12:09PM +0800, Anand Jain wrote:
> This is part 3 of the series, containing renaming with optimization of the
> return variable.
> 
> Some of the patches are new it wasn't part of v1 or v2. The new patches follow
> verb-first format for titles. Older patches not renamed for backward reference.
> 
> Patchset passed tests -g quick without regressions, sending them first.
> 
> Patch 3/6 and 4/6 can be merged; they are separated for easier diff.

Splitting the patches like might seem strange but reviewing the changes
individually is indeed a bit easier so you can keep it like that.

> v2 part2:
>   https://lore.kernel.org/linux-btrfs/cover.1713370756.git.anand.jain@oracle.com/
> v1:
>   https://lore.kernel.org/linux-btrfs/cover.1710857863.git.anand.jain@oracle.com/
> 
> Anand Jain (6):
>   btrfs: btrfs_cleanup_fs_roots handle ret variable
>   btrfs: simplify ret in btrfs_recover_relocation
>   btrfs: rename ret in btrfs_recover_relocation
>   btrfs: rename err in btrfs_recover_relocation
>   btrfs: btrfs_drop_snapshot optimize return variable
>   btrfs: rename and optimize return variable in btrfs_find_orphan_roots

I've edited the subject lines from the previous series, please have a
look and copy the subjects when the kind of change is the same in the
patch. Also use the () when a function si mentioned in the subject.
Thanks.

Re: [PATCH v3 1/6] btrfs: btrfs_cleanup_fs_roots handle ret variable

[Anand Jain]

On 5/21/24 23:10, David Sterba wrote:
> On Thu, May 16, 2024 at 07:12:10PM +0800, Anand Jain wrote:
>> Since err represents the function return value, rename it as ret,
>> and rename the original ret, which serves as a helper return value,
>> to found. Also, optimize the code to continue call btrfs_put_root()
>> for the rest of the root if even after btrfs_orphan_cleanup() returns
>> error.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v3: Add a code comment.
>> v2: Rename to 'found' instead of 'ret2' (Josef).
>>      Call btrfs_put_root() in the while-loop, avoids use of the variable
>> 	'found' outside of the while loop (Qu).
>>      Use 'unsigned int i' instead of 'int' (Goffredo).
>>
>>   fs/btrfs/disk-io.c | 38 ++++++++++++++++++++------------------
>>   1 file changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index a91a8056758a..d38cf973b02a 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -2925,22 +2925,23 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
>>   {
>>   	u64 root_objectid = 0;
>>   	struct btrfs_root *gang[8];
>> -	int i = 0;
>> -	int err = 0;
>> -	unsigned int ret = 0;
>> +	int ret = 0;
>>   
>>   	while (1) {
>> +		unsigned int i;
>> +		unsigned int found;
>> +
>>   		spin_lock(&fs_info->fs_roots_radix_lock);
>> -		ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
>> +		found = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
>>   					     (void **)gang, root_objectid,
>>   					     ARRAY_SIZE(gang));
>> -		if (!ret) {
>> +		if (!found) {
>>   			spin_unlock(&fs_info->fs_roots_radix_lock);
>>   			break;
>>   		}
>> -		root_objectid = btrfs_root_id(gang[ret - 1]) + 1;
>> +		root_objectid = btrfs_root_id(gang[found - 1]) + 1;
>>   
>> -		for (i = 0; i < ret; i++) {
>> +		for (i = 0; i < found; i++) {
> 
> You could also move the declaration of 'i' to the for loop as you move
> the other definition anyway.

Yep. Done in v4.

Thanks.

> 
>>   			/* Avoid to grab roots in dead_roots. */
>>   			if (btrfs_root_refs(&gang[i]->root_item) == 0) {
>>   				gang[i] = NULL;
>> @@ -2951,24 +2952,25 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
>>   		}
>>   		spin_unlock(&fs_info->fs_roots_radix_lock);
>>   
>> -		for (i = 0; i < ret; i++) {
>> +		for (i = 0; i < found; i++) {
>>   			if (!gang[i])
>>   				continue;
>>   			root_objectid = btrfs_root_id(gang[i]);
>> -			err = btrfs_orphan_cleanup(gang[i]);
>> -			if (err)
>> -				goto out;
>> +			/*
>> +			 * Continue to release the remaining roots after the first
>> +			 * error without cleanup and preserve the first error
>> +			 * for the return.
>> +			 */
>> +			if (!ret)
>> +				ret = btrfs_orphan_cleanup(gang[i]);
>>   			btrfs_put_root(gang[i]);
>>   		}
>> +		if (ret)
>> +			break;
>> +
>>   		root_objectid++;
>>   	}
>> -out:
>> -	/* Release the uncleaned roots due to error. */
>> -	for (; i < ret; i++) {
>> -		if (gang[i])
>> -			btrfs_put_root(gang[i]);
>> -	}
>> -	return err;
>> +	return ret;
>>   }
>>   
>>   /*
>> -- 
>> 2.38.1
>>

Re: [PATCH v3 6/6] btrfs: rename and optimize return variable in btrfs_find_orphan_roots

[Anand Jain]

On 5/21/24 23:18, David Sterba wrote:
> On Thu, May 16, 2024 at 07:12:15PM +0800, Anand Jain wrote:
>> The variable err is the actual return value of this function, and the
>> variable ret is a helper variable for err, which actually is not
>> needed and can be handled just by err, which is renamed to ret.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v3: drop ret2 as there is no need for it.
>> v2: n/a
>>   fs/btrfs/root-tree.c | 32 ++++++++++++++++----------------
>>   1 file changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
>> index 33962671a96c..c11b0bccf513 100644
>> --- a/fs/btrfs/root-tree.c
>> +++ b/fs/btrfs/root-tree.c
>> @@ -220,8 +220,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
>>   	struct btrfs_path *path;
>>   	struct btrfs_key key;
>>   	struct btrfs_root *root;
>> -	int err = 0;
>> -	int ret;
>> +	int ret = 0;
>>   
>>   	path = btrfs_alloc_path();
>>   	if (!path)
>> @@ -235,18 +234,19 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
>>   		u64 root_objectid;
>>   
>>   		ret = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0);
>> -		if (ret < 0) {
>> -			err = ret;
>> +		if (ret < 0)
>>   			break;
>> -		}
>> +		ret = 0;
> 
> Should this be handled when ret > 0? This would be unexpected and
> probably means a corruption but simply overwriting the value does not
> seem right.
> 

Agreed.

+               if (ret > 0)
+                       ret = 0;

is much neater.

As in v4.

Thanks, Anand

>>   
>>   		leaf = path->nodes[0];
>>   		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
>>   			ret = btrfs_next_leaf(tree_root, path);
>>   			if (ret < 0)
>> -				err = ret;
>> -			if (ret != 0)
>>   				break;
>> +			if (ret > 0) {
>> +				ret = 0;
>> +				break;
>> +			}
>>   			leaf = path->nodes[0];
>>   		}
>>   
>> @@ -261,26 +261,26 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
>>   		key.offset++;
>>   
>>   		root = btrfs_get_fs_root(fs_info, root_objectid, false);
>> -		err = PTR_ERR_OR_ZERO(root);
>> -		if (err && err != -ENOENT) {
>> +		ret = PTR_ERR_OR_ZERO(root);
>> +		if (ret && ret != -ENOENT) {
>>   			break;
>> -		} else if (err == -ENOENT) {
>> +		} else if (ret == -ENOENT) {
>>   			struct btrfs_trans_handle *trans;
>>   
>>   			btrfs_release_path(path);
>>   
>>   			trans = btrfs_join_transaction(tree_root);
>>   			if (IS_ERR(trans)) {
>> -				err = PTR_ERR(trans);
>> -				btrfs_handle_fs_error(fs_info, err,
>> +				ret = PTR_ERR(trans);
>> +				btrfs_handle_fs_error(fs_info, ret,
>>   					    "Failed to start trans to delete orphan item");
>>   				break;
>>   			}
>> -			err = btrfs_del_orphan_item(trans, tree_root,
>> +			ret = btrfs_del_orphan_item(trans, tree_root,
>>   						    root_objectid);
>>   			btrfs_end_transaction(trans);
>> -			if (err) {
>> -				btrfs_handle_fs_error(fs_info, err,
>> +			if (ret) {
>> +				btrfs_handle_fs_error(fs_info, ret,
>>   					    "Failed to delete root orphan item");
>>   				break;
>>   			}
>> @@ -311,7 +311,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
>>   	}
>>   
>>   	btrfs_free_path(path);
>> -	return err;
>> +	return ret;
>>   }
>>   
>>   /* drop the root item for 'key' from the tree root */
>> -- 
>> 2.38.1
>>

Re: [PATCH v3 0/6] part3 trivial adjustments for return variable coding style

[Anand Jain]

On 5/21/24 23:21, David Sterba wrote:
> On Thu, May 16, 2024 at 07:12:09PM +0800, Anand Jain wrote:
>> This is part 3 of the series, containing renaming with optimization of the
>> return variable.
>>
>> Some of the patches are new it wasn't part of v1 or v2. The new patches follow
>> verb-first format for titles. Older patches not renamed for backward reference.
>>
>> Patchset passed tests -g quick without regressions, sending them first.
>>
>> Patch 3/6 and 4/6 can be merged; they are separated for easier diff.
> 
> Splitting the patches like might seem strange but reviewing the changes
> individually is indeed a bit easier so you can keep it like that.
> 
>> v2 part2:
>>    https://lore.kernel.org/linux-btrfs/cover.1713370756.git.anand.jain@oracle.com/
>> v1:
>>    https://lore.kernel.org/linux-btrfs/cover.1710857863.git.anand.jain@oracle.com/
>>
>> Anand Jain (6):
>>    btrfs: btrfs_cleanup_fs_roots handle ret variable
>>    btrfs: simplify ret in btrfs_recover_relocation
>>    btrfs: rename ret in btrfs_recover_relocation
>>    btrfs: rename err in btrfs_recover_relocation
>>    btrfs: btrfs_drop_snapshot optimize return variable
>>    btrfs: rename and optimize return variable in btrfs_find_orphan_roots
> 
> I've edited the subject lines from the previous series, please have a
> look and copy the subjects when the kind of change is the same in the
> patch. Also use the () when a function si mentioned in the subject.
> Thanks.

yep. I have updated the patch titles in v4.

Thx. Anand

Re: [PATCH v3 6/6] btrfs: rename and optimize return variable in btrfs_find_orphan_roots

[David Sterba]

On Wed, May 22, 2024 at 01:10:08AM +0800, Anand Jain wrote:
> 
> 
> On 5/21/24 23:18, David Sterba wrote:
> > On Thu, May 16, 2024 at 07:12:15PM +0800, Anand Jain wrote:
> >> The variable err is the actual return value of this function, and the
> >> variable ret is a helper variable for err, which actually is not
> >> needed and can be handled just by err, which is renamed to ret.
> >>
> >> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> >> ---
> >> v3: drop ret2 as there is no need for it.
> >> v2: n/a
> >>   fs/btrfs/root-tree.c | 32 ++++++++++++++++----------------
> >>   1 file changed, 16 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> >> index 33962671a96c..c11b0bccf513 100644
> >> --- a/fs/btrfs/root-tree.c
> >> +++ b/fs/btrfs/root-tree.c
> >> @@ -220,8 +220,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
> >>   	struct btrfs_path *path;
> >>   	struct btrfs_key key;
> >>   	struct btrfs_root *root;
> >> -	int err = 0;
> >> -	int ret;
> >> +	int ret = 0;
> >>   
> >>   	path = btrfs_alloc_path();
> >>   	if (!path)
> >> @@ -235,18 +234,19 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
> >>   		u64 root_objectid;
> >>   
> >>   		ret = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0);
> >> -		if (ret < 0) {
> >> -			err = ret;
> >> +		if (ret < 0)
> >>   			break;
> >> -		}
> >> +		ret = 0;
> > 
> > Should this be handled when ret > 0? This would be unexpected and
> > probably means a corruption but simply overwriting the value does not
> > seem right.
> > 
> 
> Agreed.
> 
> +               if (ret > 0)
> +                       ret = 0;
> 
> is much neater.

That's not what I meant. When btrfs_search_slot() returns 1 then the key
was not found and could be inserted, path points to the slot. This is
done in many other places, so in the orphan root lookup it should be
also handled. 

Re: [PATCH v3 6/6] btrfs: rename and optimize return variable in btrfs_find_orphan_roots

[Anand Jain]

On 22/05/2024 01:59, David Sterba wrote:
> On Wed, May 22, 2024 at 01:10:08AM +0800, Anand Jain wrote:
>>
>>
>> On 5/21/24 23:18, David Sterba wrote:
>>> On Thu, May 16, 2024 at 07:12:15PM +0800, Anand Jain wrote:
>>>> The variable err is the actual return value of this function, and the
>>>> variable ret is a helper variable for err, which actually is not
>>>> needed and can be handled just by err, which is renamed to ret.
>>>>
>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>> ---
>>>> v3: drop ret2 as there is no need for it.
>>>> v2: n/a
>>>>    fs/btrfs/root-tree.c | 32 ++++++++++++++++----------------
>>>>    1 file changed, 16 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
>>>> index 33962671a96c..c11b0bccf513 100644
>>>> --- a/fs/btrfs/root-tree.c
>>>> +++ b/fs/btrfs/root-tree.c
>>>> @@ -220,8 +220,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
>>>>    	struct btrfs_path *path;
>>>>    	struct btrfs_key key;
>>>>    	struct btrfs_root *root;
>>>> -	int err = 0;
>>>> -	int ret;
>>>> +	int ret = 0;
>>>>    
>>>>    	path = btrfs_alloc_path();
>>>>    	if (!path)
>>>> @@ -235,18 +234,19 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
>>>>    		u64 root_objectid;
>>>>    
>>>>    		ret = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0);
>>>> -		if (ret < 0) {
>>>> -			err = ret;
>>>> +		if (ret < 0)
>>>>    			break;
>>>> -		}
>>>> +		ret = 0;
>>>
>>> Should this be handled when ret > 0? This would be unexpected and
>>> probably means a corruption but simply overwriting the value does not
>>> seem right.
>>>
>>
>> Agreed.
>>
>> +               if (ret > 0)
>> +                       ret = 0;
>>
>> is much neater.
> 
> That's not what I meant. When btrfs_search_slot() returns 1 then the key
> was not found and could be inserted, path points to the slot. This is
> done in many other places, so in the orphan root lookup it should be
> also handled.

For the scenario where ret > 0 is good, we generally do varied tasks.
However, here we need to reassign ret = 0. Originally, err remained 0
and returned 0.

Or, my bad, I didn't understand which usual error handling pattern you 
are referring to.

Thanks, Anand