[PATCH v2 0/3] btrfs: random code cleanup

[PATCH v2 0/3] btrfs: random code cleanup

[Sun YangKai]

These patches are not intended to change any behavior of current code.
Just trying to make the codes easier to read, and, maybe, perform better.
I'm working on btrfs_search_forward(), trying to improve it and fix some
misbehaviors. And I'll send some patches that will change the behavior of
the code later.

I'm new to the maillist, trying to read the implements, and improve it from
my perspective.
If you have any feedback or questions, please let me know :)

--
Changelog
v2:
- Improve the commit messages advised by David Sterba
- Fix some code style issues advised by David Sterba

Sun YangKai (3):
  btrfs: simplify the return value handling in search_ioctl()
  btrfs: remove the unnecessary local variable in btrfs_search_forward()
  btrfs: avoid redundant slot assignment in btrfs_search_forward()

 fs/btrfs/ctree.c | 15 ++++++---------
 fs/btrfs/ioctl.c | 15 +++++++--------
 2 files changed, 13 insertions(+), 17 deletions(-)

-- 
2.48.1

[PATCH v2 1/3] btrfs: simplify the return value handling in search_ioctl()

[Sun YangKai]

1. Move the assignment of ret = -EFAULT to within the error condition
   check in fault_in_subpage_writeable(). The previous placement outside
   the condition could lead to the error value being overwritten by
   subsequent assignments, cause unnecessary assignments.

2. Simplify loop exit logic by removing redundant `goto`.
   The original code used `goto err` to bypass post-loop processing after
   handling errors from `btrfs_search_forward()`. However, the loop's
   termination naturally falls through to the post-loop section, which
   already handles `ret` values. Replacing `goto err` with `break`
   eliminates redundant control flow, consolidates error handling, and
   makes the loop's exit conditions explicit.

The changes ensure proper error propagation and make the loop's exit
conditions clearer while maintaining functional equivalence.

Signed-off-by: Sun YangKai <sunk67188@gmail.com>
---
 fs/btrfs/ioctl.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 6c18bad53cd3..bef158a1260b 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1642,21 +1642,20 @@ static noinline int search_ioctl(struct inode *inode,
 	key.offset = sk->min_offset;
 
 	while (1) {
-		ret = -EFAULT;
 		/*
 		 * Ensure that the whole user buffer is faulted in at sub-page
 		 * granularity, otherwise the loop may live-lock.
 		 */
 		if (fault_in_subpage_writeable(ubuf + sk_offset,
-					       *buf_size - sk_offset))
+					       *buf_size - sk_offset)) {
+			ret = -EFAULT;
 			break;
+		}
 
 		ret = btrfs_search_forward(root, &key, path, sk->min_transid);
-		if (ret != 0) {
-			if (ret > 0)
-				ret = 0;
-			goto err;
-		}
+		if (ret)
+			break;
+
 		ret = copy_to_sk(path, &key, sk, buf_size, ubuf,
 				 &sk_offset, &num_found);
 		btrfs_release_path(path);
@@ -1666,7 +1665,7 @@ static noinline int search_ioctl(struct inode *inode,
 	}
 	if (ret > 0)
 		ret = 0;
-err:
+
 	sk->nr_items = num_found;
 	btrfs_put_root(root);
 	btrfs_free_path(path);
-- 
2.48.1

[PATCH v2 2/3] btrfs: remove the unnecessary local variable in btrfs_search_forward()

[Sun YangKai]

The 'found_key' variable was only used to temporarily store the found key
before copying it to 'min_key' at the end of the function when returning
success (ret=0).

So eliminating the 'found_key' variable, and directly store the key into
'min_key' at the exact loop exit points where ret=0 is set, maintaining
identical functionality.

Signed-off-by: Sun YangKai <sunk67188@gmail.com>
---
 fs/btrfs/ctree.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 3dc5a35dd19b..5f7c937b0f4d 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -4608,7 +4608,6 @@ int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key,
 			 u64 min_trans)
 {
 	struct extent_buffer *cur;
-	struct btrfs_key found_key;
 	int slot;
 	int sret;
 	u32 nritems;
@@ -4644,7 +4643,8 @@ int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key,
 				goto find_next_key;
 			ret = 0;
 			path->slots[level] = slot;
-			btrfs_item_key_to_cpu(cur, &found_key, slot);
+			/* Save our key for returning back. */
+			btrfs_item_key_to_cpu(cur, min_key, slot);
 			goto out;
 		}
 		if (sret && slot > 0)
@@ -4679,11 +4679,11 @@ int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key,
 				goto out;
 			}
 		}
-		/* save our key for returning back */
-		btrfs_node_key_to_cpu(cur, &found_key, slot);
 		path->slots[level] = slot;
 		if (level == path->lowest_level) {
 			ret = 0;
+			/* Save our key for returning back. */
+			btrfs_node_key_to_cpu(cur, min_key, slot);
 			goto out;
 		}
 		cur = btrfs_read_node_slot(cur, slot);
@@ -4700,10 +4700,8 @@ int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key,
 	}
 out:
 	path->keep_locks = keep_locks;
-	if (ret == 0) {
+	if (ret == 0)
 		btrfs_unlock_up_safe(path, path->lowest_level + 1);
-		memcpy(min_key, &found_key, sizeof(found_key));
-	}
 	return ret;
 }
 
-- 
2.48.1

[PATCH v2 3/3] btrfs: avoid redundant slot assignment in btrfs_search_forward()

[Sun YangKai]

Moving `path->slots[level] = slot` before the condition check to prevent
duplicate assignment. Previously, the slot was set both inside and after
the `slot >= nritems` block with no change in its value, which
is unnecessary.

Signed-off-by: Sun YangKai <sunk67188@gmail.com>
---
 fs/btrfs/ctree.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 5f7c937b0f4d..c982960d8a91 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -4668,8 +4668,8 @@ int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key,
 		 * we didn't find a candidate key in this node, walk forward
 		 * and find another one
 		 */
+		path->slots[level] = slot;
 		if (slot >= nritems) {
-			path->slots[level] = slot;
 			sret = btrfs_find_next_key(root, path, min_key, level,
 						  min_trans);
 			if (sret == 0) {
@@ -4679,7 +4679,6 @@ int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key,
 				goto out;
 			}
 		}
-		path->slots[level] = slot;
 		if (level == path->lowest_level) {
 			ret = 0;
 			/* Save our key for returning back. */
-- 
2.48.1

Re: [PATCH v2 0/3] btrfs: random code cleanup

[David Sterba]

On Tue, Mar 11, 2025 at 04:13:11PM +0800, Sun YangKai wrote:
> These patches are not intended to change any behavior of current code.
> Just trying to make the codes easier to read, and, maybe, perform better.
> I'm working on btrfs_search_forward(), trying to improve it and fix some
> misbehaviors. And I'll send some patches that will change the behavior of
> the code later.
> 
> I'm new to the maillist, trying to read the implements, and improve it from
> my perspective.
> If you have any feedback or questions, please let me know :)
> 
> --
> Changelog
> v2:
> - Improve the commit messages advised by David Sterba
> - Fix some code style issues advised by David Sterba

Added to for-next with some minor tweaks, thanks.