[PATCH v2 00/11] part2 trivial adjustments for return variable coding style

[PATCH v2 00/11] part2 trivial adjustments for return variable coding style

[Anand Jain]

This is Part 2 of the series, following v1 as below. Changes includes
optimizations on top of reorganizing the return variables in each function,
as stated in the each patch, based on the received review feedback. Thank you.

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

Anand Jain (11):
  btrfs: btrfs_cleanup_fs_roots handle ret variable
  btrfs: btrfs_write_marked_extents rename werr and err to ret
  btrfs: __btrfs_wait_marked_extents rename werr and err to ret
  btrfs: build_backref_tree rename err and ret to ret
  btrfs: relocate_tree_blocks reuse ret instead of err
  btrfs: btrfs_recover_relocation rename ret to ret2 and err to ret
  btrfs: quick_update_accounting drop variable err
  btrfs: btrfs_qgroup_rescan_worker rename ret to ret2 and err to ret
  btrfs: lookup_extent_data_ref code optimize return
  btrfs: btrfs_drop_snapshot optimize return variable
  btrfs: btrfs_drop_subtree optimize return variable

 fs/btrfs/disk-io.c     |  33 ++++++--------
 fs/btrfs/extent-tree.c |  93 ++++++++++++++++++-------------------
 fs/btrfs/qgroup.c      |  45 +++++++++---------
 fs/btrfs/relocation.c  | 101 +++++++++++++++++++----------------------
 fs/btrfs/transaction.c |  44 ++++++++----------
 5 files changed, 146 insertions(+), 170 deletions(-)

-- 
2.41.0

[PATCH v2 01/11] 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>
---
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 | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index c2dc88f909b0..d1d23736de3c 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2926,22 +2926,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;
@@ -2952,24 +2953,20 @@ 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;
+			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.41.0

[PATCH v2 02/11] btrfs: btrfs_write_marked_extents rename werr and err to ret

[Anand Jain]

Rename the function's local variable werr and err to ret.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
v2:  On top of the patch
      [PATCH v2] btrfs: report filemap_fdata<write|wait>_range error
     Just one variable 'ret' for the return error code.

 fs/btrfs/transaction.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 8c3b3cda1390..defdb0979d68 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1118,8 +1118,7 @@ int btrfs_end_transaction_throttle(struct btrfs_trans_handle *trans)
 int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
 			       struct extent_io_tree *dirty_pages, int mark)
 {
-	int err = 0;
-	int werr = 0;
+	int ret = 0;
 	struct address_space *mapping = fs_info->btree_inode->i_mapping;
 	struct extent_state *cached_state = NULL;
 	u64 start = 0;
@@ -1129,7 +1128,7 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
 				     mark, &cached_state)) {
 		bool wait_writeback = false;
 
-		err = convert_extent_bit(dirty_pages, start, end,
+		ret = convert_extent_bit(dirty_pages, start, end,
 					 EXTENT_NEED_WAIT,
 					 mark, &cached_state);
 		/*
@@ -1145,24 +1144,22 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
 		 * We cleanup any entries left in the io tree when committing
 		 * the transaction (through extent_io_tree_release()).
 		 */
-		if (err == -ENOMEM) {
-			err = 0;
+		if (ret == -ENOMEM) {
+			ret = 0;
 			wait_writeback = true;
 		}
-		if (!err)
-			err = filemap_fdatawrite_range(mapping, start, end);
-		if (err)
-			werr = err;
-		else if (wait_writeback)
-			werr = filemap_fdatawait_range(mapping, start, end);
+		if (!ret)
+			ret = filemap_fdatawrite_range(mapping, start, end);
+		if (!ret && wait_writeback)
+			ret = filemap_fdatawait_range(mapping, start, end);
 		free_extent_state(cached_state);
-		if (werr)
+		if (ret)
 			break;
 		cached_state = NULL;
 		cond_resched();
 		start = end + 1;
 	}
-	return werr;
+	return ret;
 }
 
 /*
-- 
2.41.0

[PATCH v2 03/11] btrfs: __btrfs_wait_marked_extents rename werr and err to ret

[Anand Jain]

Rename the function's local return variables err and werr to ret.
Also, align the variable declarations with the other declarations in
the function for better function space alignment.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2:
    On top of the patch:
      [PATCH v2] btrfs: report filemap_fdata<write|wait>_range error
    Single return value 'ret' will suffice.

 fs/btrfs/transaction.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index defdb0979d68..3388c836b9a5 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1171,12 +1171,11 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
 static int __btrfs_wait_marked_extents(struct btrfs_fs_info *fs_info,
 				       struct extent_io_tree *dirty_pages)
 {
-	int err = 0;
-	int werr = 0;
 	struct address_space *mapping = fs_info->btree_inode->i_mapping;
 	struct extent_state *cached_state = NULL;
 	u64 start = 0;
 	u64 end;
+	int ret = 0;
 
 	while (find_first_extent_bit(dirty_pages, start, &start, &end,
 				     EXTENT_NEED_WAIT, &cached_state)) {
@@ -1188,24 +1187,20 @@ static int __btrfs_wait_marked_extents(struct btrfs_fs_info *fs_info,
 		 * concurrently - we do it only at transaction commit time when
 		 * it's safe to do it (through extent_io_tree_release()).
 		 */
-		err = clear_extent_bit(dirty_pages, start, end,
+		ret = clear_extent_bit(dirty_pages, start, end,
 				       EXTENT_NEED_WAIT, &cached_state);
-		if (err == -ENOMEM)
-			err = 0;
-		if (!err)
-			err = filemap_fdatawait_range(mapping, start, end);
-		if (err)
-			werr = err;
+		if (ret == -ENOMEM)
+			ret = 0;
+		if (!ret)
+			ret = filemap_fdatawait_range(mapping, start, end);
 		free_extent_state(cached_state);
-		if (werr)
+		if (ret)
 			break;
 		cached_state = NULL;
 		cond_resched();
 		start = end + 1;
 	}
-	if (err)
-		werr = err;
-	return werr;
+	return ret;
 }
 
 static int btrfs_wait_extents(struct btrfs_fs_info *fs_info,
-- 
2.41.0

[PATCH v2 04/11] btrfs: build_backref_tree rename err and ret to ret

[Anand Jain]

Code sytle fix in the function build_backref_tree().
Drop the ret initialization 0, as we don't need it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: drop ret2 (Josef).

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

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 7e7799b4560b..aef7d286252b 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -473,20 +473,19 @@ static noinline_for_stack struct btrfs_backref_node *build_backref_tree(
 	struct btrfs_backref_node *node = NULL;
 	struct btrfs_backref_edge *edge;
 	int ret;
-	int err = 0;
 
 	iter = btrfs_backref_iter_alloc(rc->extent_root->fs_info);
 	if (!iter)
 		return ERR_PTR(-ENOMEM);
 	path = btrfs_alloc_path();
 	if (!path) {
-		err = -ENOMEM;
+		ret = -ENOMEM;
 		goto out;
 	}
 
 	node = btrfs_backref_alloc_node(cache, bytenr, level);
 	if (!node) {
-		err = -ENOMEM;
+		ret = -ENOMEM;
 		goto out;
 	}
 
@@ -497,10 +496,9 @@ static noinline_for_stack struct btrfs_backref_node *build_backref_tree(
 	do {
 		ret = btrfs_backref_add_tree_node(trans, cache, path, iter,
 						  node_key, cur);
-		if (ret < 0) {
-			err = ret;
+		if (ret < 0)
 			goto out;
-		}
+
 		edge = list_first_entry_or_null(&cache->pending_edge,
 				struct btrfs_backref_edge, list[UPPER]);
 		/*
@@ -515,10 +513,8 @@ static noinline_for_stack struct btrfs_backref_node *build_backref_tree(
 
 	/* Finish the upper linkage of newly added edges/nodes */
 	ret = btrfs_backref_finish_upper_links(cache, node);
-	if (ret < 0) {
-		err = ret;
+	if (ret < 0)
 		goto out;
-	}
 
 	if (handle_useless_nodes(rc, node))
 		node = NULL;
@@ -526,9 +522,9 @@ static noinline_for_stack struct btrfs_backref_node *build_backref_tree(
 	btrfs_free_path(iter->path);
 	kfree(iter);
 	btrfs_free_path(path);
-	if (err) {
+	if (ret) {
 		btrfs_backref_error_cleanup(cache, node);
-		return ERR_PTR(err);
+		return ERR_PTR(ret);
 	}
 	ASSERT(!node || !node->detached);
 	ASSERT(list_empty(&cache->useless_node) &&
-- 
2.41.0

[PATCH v2 05/11] btrfs: relocate_tree_blocks reuse ret instead of err

[Anand Jain]

Coding style fixes the function relocate_tree_blocks().
After the fix, ret is the return value variable.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: drop 'ret2' (Josef)

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

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index aef7d286252b..bd573a0ec270 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2742,12 +2742,11 @@ int relocate_tree_blocks(struct btrfs_trans_handle *trans,
 	struct btrfs_path *path;
 	struct tree_block *block;
 	struct tree_block *next;
-	int ret;
-	int err = 0;
+	int ret = 0;
 
 	path = btrfs_alloc_path();
 	if (!path) {
-		err = -ENOMEM;
+		ret = -ENOMEM;
 		goto out_free_blocks;
 	}
 
@@ -2762,8 +2761,8 @@ int relocate_tree_blocks(struct btrfs_trans_handle *trans,
 	/* Get first keys */
 	rbtree_postorder_for_each_entry_safe(block, next, blocks, rb_node) {
 		if (!block->key_ready) {
-			err = get_tree_block_key(fs_info, block);
-			if (err)
+			ret = get_tree_block_key(fs_info, block);
+			if (ret)
 				goto out_free_path;
 		}
 	}
@@ -2773,25 +2772,23 @@ int relocate_tree_blocks(struct btrfs_trans_handle *trans,
 		node = build_backref_tree(trans, rc, &block->key,
 					  block->level, block->bytenr);
 		if (IS_ERR(node)) {
-			err = PTR_ERR(node);
+			ret = PTR_ERR(node);
 			goto out;
 		}
 
 		ret = relocate_tree_block(trans, rc, node, &block->key,
 					  path);
-		if (ret < 0) {
-			err = ret;
+		if (ret < 0)
 			break;
-		}
 	}
 out:
-	err = finish_pending_nodes(trans, rc, path, err);
+	ret = finish_pending_nodes(trans, rc, path, ret);
 
 out_free_path:
 	btrfs_free_path(path);
 out_free_blocks:
 	free_block_list(blocks);
-	return err;
+	return ret;
 }
 
 static noinline_for_stack int prealloc_file_extent_cluster(
-- 
2.41.0

[PATCH v2 06/11] btrfs: btrfs_recover_relocation rename ret to ret2 and err to ret

[Anand Jain]

Fix the code style for the return variable. First, rename ret to ret2,
compile it, and then rename err to ret. This method of changing helped
confirm that there are no instances of the old ret not renamed to ret2.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: no change from v1

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

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index bd573a0ec270..0b802d0c5a65 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4222,8 +4222,8 @@ 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 err = 0;
+	int ret2;
+	int ret = 0;
 
 	path = btrfs_alloc_path();
 	if (!path)
@@ -4235,13 +4235,13 @@ 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,
+		ret2 = btrfs_search_slot(NULL, fs_info->tree_root, &key,
 					path, 0, 0);
-		if (ret < 0) {
-			err = ret;
+		if (ret2 < 0) {
+			ret = ret2;
 			goto out;
 		}
-		if (ret > 0) {
+		if (ret2 > 0) {
 			if (path->slots[0] == 0)
 				break;
 			path->slots[0]--;
@@ -4256,7 +4256,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;
 		}
 
@@ -4267,14 +4267,14 @@ 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;
+				ret2 = PTR_ERR(fs_root);
+				if (ret2 != -ENOENT) {
+					ret = ret2;
 					goto out;
 				}
-				ret = mark_garbage_root(reloc_root);
-				if (ret < 0) {
-					err = ret;
+				ret2 = mark_garbage_root(reloc_root);
+				if (ret2 < 0) {
+					ret = ret2;
 					goto out;
 				}
 			} else {
@@ -4294,13 +4294,13 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 
 	rc = alloc_reloc_control(fs_info);
 	if (!rc) {
-		err = -ENOMEM;
+		ret = -ENOMEM;
 		goto out;
 	}
 
-	ret = reloc_chunk_start(fs_info);
-	if (ret < 0) {
-		err = ret;
+	ret2 = reloc_chunk_start(fs_info);
+	if (ret2 < 0) {
+		ret = ret2;
 		goto out_end;
 	}
 
@@ -4310,7 +4310,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;
 	}
 
@@ -4330,15 +4330,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);
@@ -4348,8 +4348,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);
@@ -4358,14 +4358,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:
-	ret = clean_dirty_subvols(rc);
-	if (ret < 0 && !err)
-		err = ret;
+	ret2 = clean_dirty_subvols(rc);
+	if (ret2 < 0 && !ret)
+		ret = ret2;
 out_unset:
 	unset_reloc_control(rc);
 out_end:
@@ -4376,14 +4376,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.41.0

[PATCH v2 07/11] btrfs: quick_update_accounting drop variable err

[Anand Jain]

In quick_update_accounting err is used as 2nd return value, which could
be achieved just with ret.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: optimize __qgroup_excl_accounting() error out, so drop 'ret2' (Josef).

 fs/btrfs/qgroup.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 9a9f84c4d3b8..9a0f47d0c6bf 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1541,18 +1541,15 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
 {
 	struct btrfs_qgroup *qgroup;
 	int ret = 1;
-	int err = 0;
 
 	qgroup = find_qgroup_rb(fs_info, src);
 	if (!qgroup)
 		goto out;
 	if (qgroup->excl == qgroup->rfer) {
-		ret = 0;
-		err = __qgroup_excl_accounting(fs_info, dst, qgroup, sign);
-		if (err < 0) {
-			ret = err;
+		ret = __qgroup_excl_accounting(fs_info, dst, qgroup, sign);
+		if (ret < 0)
 			goto out;
-		}
+		ret = 0;
 	}
 out:
 	if (ret)
-- 
2.41.0

[PATCH v2 08/11] btrfs: btrfs_qgroup_rescan_worker rename ret to ret2 and err to ret

[Anand Jain]

Rename ret to ret2 compile and then err to ret. Also, new ret2 is found to
be localized within the 'if (trans)' statement, so move its declaration there.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: No change

 fs/btrfs/qgroup.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 9a0f47d0c6bf..8ae0e1048b23 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -3703,8 +3703,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
 						     qgroup_rescan_work);
 	struct btrfs_path *path;
 	struct btrfs_trans_handle *trans = NULL;
-	int err = -ENOMEM;
-	int ret = 0;
+	int ret = -ENOMEM;
 	bool stopped = false;
 	bool did_leaf_rescans = false;
 
@@ -3721,18 +3720,18 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
 	path->search_commit_root = 1;
 	path->skip_locking = 1;
 
-	err = 0;
-	while (!err && !(stopped = rescan_should_stop(fs_info))) {
+	ret = 0;
+	while (!ret && !(stopped = rescan_should_stop(fs_info))) {
 		trans = btrfs_start_transaction(fs_info->fs_root, 0);
 		if (IS_ERR(trans)) {
-			err = PTR_ERR(trans);
+			ret = PTR_ERR(trans);
 			break;
 		}
 
-		err = qgroup_rescan_leaf(trans, path);
+		ret = qgroup_rescan_leaf(trans, path);
 		did_leaf_rescans = true;
 
-		if (err > 0)
+		if (ret > 0)
 			btrfs_commit_transaction(trans);
 		else
 			btrfs_end_transaction(trans);
@@ -3742,10 +3741,10 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
 	btrfs_free_path(path);
 
 	mutex_lock(&fs_info->qgroup_rescan_lock);
-	if (err > 0 &&
+	if (ret > 0 &&
 	    fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT) {
 		fs_info->qgroup_flags &= ~BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
-	} else if (err < 0 || stopped) {
+	} else if (ret < 0 || stopped) {
 		fs_info->qgroup_flags |= BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
 	}
 	mutex_unlock(&fs_info->qgroup_rescan_lock);
@@ -3760,11 +3759,11 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
 	if (did_leaf_rescans) {
 		trans = btrfs_start_transaction(fs_info->quota_root, 1);
 		if (IS_ERR(trans)) {
-			err = PTR_ERR(trans);
+			ret = PTR_ERR(trans);
 			trans = NULL;
 			btrfs_err(fs_info,
 				  "fail to start transaction for status update: %d",
-				  err);
+				  ret);
 		}
 	} else {
 		trans = NULL;
@@ -3775,11 +3774,12 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
 	    fs_info->qgroup_flags & BTRFS_QGROUP_RUNTIME_FLAG_CANCEL_RESCAN)
 		fs_info->qgroup_flags &= ~BTRFS_QGROUP_STATUS_FLAG_RESCAN;
 	if (trans) {
-		ret = update_qgroup_status_item(trans);
-		if (ret < 0) {
-			err = ret;
+		int ret2 = update_qgroup_status_item(trans);
+
+		if (ret2 < 0) {
+			ret = ret2;
 			btrfs_err(fs_info, "fail to update qgroup status: %d",
-				  err);
+				  ret);
 		}
 	}
 	fs_info->qgroup_rescan_running = false;
@@ -3796,11 +3796,11 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
 		btrfs_info(fs_info, "qgroup scan paused");
 	} else if (fs_info->qgroup_flags & BTRFS_QGROUP_RUNTIME_FLAG_CANCEL_RESCAN) {
 		btrfs_info(fs_info, "qgroup scan cancelled");
-	} else if (err >= 0) {
+	} else if (ret >= 0) {
 		btrfs_info(fs_info, "qgroup scan completed%s",
-			err > 0 ? " (inconsistency flag cleared)" : "");
+			ret > 0 ? " (inconsistency flag cleared)" : "");
 	} else {
-		btrfs_err(fs_info, "qgroup scan failed with %d", err);
+		btrfs_err(fs_info, "qgroup scan failed with %d", ret);
 	}
 }
 
-- 
2.41.0

[PATCH v2 09/11] btrfs: lookup_extent_data_ref code optimize return

[Anand Jain]

First, drop err instead reuse ret, choose to return the error instead of
goto fail and then return the same error. Do not initialize the ret
until where it has to be initialized. Slight logic change in handling
the btrfs_search_slot() and btrfs_next_leaf() return value.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: rework so that 'ret2' can be dropped  (Josef)

 fs/btrfs/extent-tree.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 023920d0d971..78dc94a97e35 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -446,9 +446,8 @@ static noinline int lookup_extent_data_ref(struct btrfs_trans_handle *trans,
 	struct btrfs_extent_data_ref *ref;
 	struct extent_buffer *leaf;
 	u32 nritems;
-	int ret;
 	int recow;
-	int err = -ENOENT;
+	int ret;
 
 	key.objectid = bytenr;
 	if (parent) {
@@ -462,26 +461,26 @@ static noinline int lookup_extent_data_ref(struct btrfs_trans_handle *trans,
 again:
 	recow = 0;
 	ret = btrfs_search_slot(trans, root, &key, path, -1, 1);
-	if (ret < 0) {
-		err = ret;
-		goto fail;
-	}
+	if (ret < 0)
+		return ret;
 
 	if (parent) {
-		if (!ret)
-			return 0;
-		goto fail;
+		if (ret)
+			return -ENOENT;
+		return 0;
 	}
 
+	ret = -ENOENT;
 	leaf = path->nodes[0];
 	nritems = btrfs_header_nritems(leaf);
 	while (1) {
 		if (path->slots[0] >= nritems) {
 			ret = btrfs_next_leaf(root, path);
-			if (ret < 0)
-				err = ret;
-			if (ret)
-				goto fail;
+			if (ret) {
+				if (ret > 1)
+					return -ENOENT;
+				return ret;
+			}
 
 			leaf = path->nodes[0];
 			nritems = btrfs_header_nritems(leaf);
@@ -502,13 +501,13 @@ static noinline int lookup_extent_data_ref(struct btrfs_trans_handle *trans,
 				btrfs_release_path(path);
 				goto again;
 			}
-			err = 0;
+			ret = 0;
 			break;
 		}
 		path->slots[0]++;
 	}
 fail:
-	return err;
+	return ret;
 }
 
 static noinline int insert_extent_data_ref(struct btrfs_trans_handle *trans,
-- 
2.41.0

[PATCH v2 10/11] 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>
---
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 78dc94a97e35..17aa45b906bb 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.41.0

[PATCH v2 11/11] btrfs: btrfs_drop_subtree optimize return variable

[Anand Jain]

retw is a helper return variable used to update the actual return value
ret. Instead, just use ret.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: rework walk_up_tree() returned error, leading unusing wret. (Josef).

 fs/btrfs/extent-tree.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 17aa45b906bb..2d0c03806d80 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -6099,7 +6099,6 @@ int btrfs_drop_subtree(struct btrfs_trans_handle *trans,
 	int level;
 	int parent_level;
 	int ret = 0;
-	int wret;
 
 	BUG_ON(btrfs_root_id(root) != BTRFS_TREE_RELOC_OBJECTID);
 
@@ -6135,17 +6134,16 @@ int btrfs_drop_subtree(struct btrfs_trans_handle *trans,
 	wc->reada_count = BTRFS_NODEPTRS_PER_BLOCK(fs_info);
 
 	while (1) {
-		wret = walk_down_tree(trans, root, path, wc);
-		if (wret < 0) {
-			ret = wret;
+		ret = walk_down_tree(trans, root, path, wc);
+		if (ret < 0)
 			break;
-		}
 
-		wret = walk_up_tree(trans, root, path, wc, parent_level);
-		if (wret < 0)
-			ret = wret;
-		if (wret != 0)
+		ret = walk_up_tree(trans, root, path, wc, parent_level);
+		if (ret) {
+			if (ret > 0)
+				ret = 0;
 			break;
+		}
 	}
 
 	kfree(wc);
-- 
2.41.0

Re: [PATCH v2 01/11] btrfs: btrfs_cleanup_fs_roots handle ret variable

[David Sterba]

On Thu, Apr 18, 2024 at 03:08:33PM +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>
> ---
> 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 | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index c2dc88f909b0..d1d23736de3c 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2926,22 +2926,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;
> @@ -2952,24 +2953,20 @@ 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;
> +			if (!ret)
> +				ret = btrfs_orphan_cleanup(gang[i]);

Please add a comment, this is not a common pattern.

>  			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.41.0
> 

Re: [PATCH v2 06/11] btrfs: btrfs_recover_relocation rename ret to ret2 and err to ret

[David Sterba]

On Thu, Apr 18, 2024 at 03:08:38PM +0800, Anand Jain wrote:
> Fix the code style for the return variable. First, rename ret to ret2,
> compile it, and then rename err to ret. This method of changing helped
> confirm that there are no instances of the old ret not renamed to ret2.

There's only one place where the ret2 would make sense.

> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2: no change from v1
> 
>  fs/btrfs/relocation.c | 64 +++++++++++++++++++++----------------------
>  1 file changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index bd573a0ec270..0b802d0c5a65 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -4222,8 +4222,8 @@ 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 err = 0;
> +	int ret2;
> +	int ret = 0;
>  
>  	path = btrfs_alloc_path();
>  	if (!path)
> @@ -4235,13 +4235,13 @@ 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,
> +		ret2 = btrfs_search_slot(NULL, fs_info->tree_root, &key,
>  					path, 0, 0);
> -		if (ret < 0) {
> -			err = ret;
> +		if (ret2 < 0) {
> +			ret = ret2;

If you do just ret = ret2 then it's not needed, ret is always reset and
then is some check and 'goto out'. Using ret2 should be only for cases
where the previous value must be preserved.

>  			goto out;
>  		}
> -		if (ret > 0) {
> +		if (ret2 > 0) {
>  			if (path->slots[0] == 0)
>  				break;
>  			path->slots[0]--;
> @@ -4256,7 +4256,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;
>  		}
>  
> @@ -4267,14 +4267,14 @@ 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;
> +				ret2 = PTR_ERR(fs_root);
> +				if (ret2 != -ENOENT) {
> +					ret = ret2;

Same.

>  					goto out;
>  				}
> -				ret = mark_garbage_root(reloc_root);
> -				if (ret < 0) {
> -					err = ret;
> +				ret2 = mark_garbage_root(reloc_root);
> +				if (ret2 < 0) {
> +					ret = ret2;

And here.

>  					goto out;
>  				}
>  			} else {
> @@ -4294,13 +4294,13 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
>  
>  	rc = alloc_reloc_control(fs_info);
>  	if (!rc) {
> -		err = -ENOMEM;
> +		ret = -ENOMEM;
>  		goto out;
>  	}
>  
> -	ret = reloc_chunk_start(fs_info);
> -	if (ret < 0) {
> -		err = ret;
> +	ret2 = reloc_chunk_start(fs_info);
> +	if (ret2 < 0) {
> +		ret = ret2;
>  		goto out_end;
>  	}
>  
> @@ -4310,7 +4310,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;
>  	}
>  
> @@ -4330,15 +4330,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);
> @@ -4348,8 +4348,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);
> @@ -4358,14 +4358,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:
> -	ret = clean_dirty_subvols(rc);
> -	if (ret < 0 && !err)
> -		err = ret;
> +	ret2 = clean_dirty_subvols(rc);
> +	if (ret2 < 0 && !ret)
> +		ret = ret2;

This is probably the only place where it makes sense but only because
the original code does not directly handle btrfs_commit_transaction()
and calls clean_dirty_subvols(), so the two values have to be checked.

Changing the logic here should be a separate patch so ret2 can stay
otherwise the whole function can use single ret for everything as far as
I can see.

Re: [PATCH v2 10/11] btrfs: btrfs_drop_snapshot optimize return variable

[David Sterba]

On Thu, Apr 18, 2024 at 03:08:42PM +0800, Anand Jain wrote:
> Drop the variable 'err', reuse the variable 'ret' by reinitializing it to
> zero where necessary.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> 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 78dc94a97e35..17aa45b906bb 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

If you change comments please also fix the formatting or spelling, in
this case first letter is uppercase.

>  			 * 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.41.0
> 

Re: [PATCH v2 00/11] part2 trivial adjustments for return variable coding style

[David Sterba]

On Thu, Apr 18, 2024 at 03:08:32PM +0800, Anand Jain wrote:
> This is Part 2 of the series, following v1 as below. Changes includes
> optimizations on top of reorganizing the return variables in each function,
> as stated in the each patch, based on the received review feedback. Thank you.

Most of the patches are simple renames, so please add them to for-next
as before. For the commented patches we may need another round but we're
getting closer to fixing all cases.