* [PATCH] btrfs: replace manual next item search with btrfs_get_next_valid_item()
@ 2025-07-30 14:53 Sun YangKai
2025-07-30 22:07 ` Qu Wenruo
0 siblings, 1 reply; 3+ messages in thread
From: Sun YangKai @ 2025-07-30 14:53 UTC (permalink / raw)
Cc: linux-btrfs, Sun YangKai
The function btrfs_get_next_valid_item() was introduced in
commit 62142be363ae9("btrfs: introduce btrfs_for_each_slot iterator macro")
and has never been used in other places other than
the btrfs_for_each_slot macro.
However, in multiple locations, we implement the same pattern
for advancing to the next valid item in a btrfs path:
1. Increment path->slots[0]
2. Check if beyond last item in leaf
3. If so, call btrfs_next_leaf()
4. Check return value and handle errors
5. Update the key
This pattern can be replaced by the helper function
btrfs_get_next_valid_item() which encapsulates step 2-5.
The change simplifies the code by:
* Reducing code duplication
* Making control flow easier to follow
* Centralizing error handling logic
* Improving maintainability
The functionality remains identical as the helper implements the exact
same logic sequence.
There are still some places that have a similar pattern
but with more complex logic. These parts are left untouched.
One example: in file.c: btrfs_drop_extents(), the pattern after
the label next_slot is left untouched.
Signed-off-by: Sun YangKai <sunk67188@gmail.com>
---
fs/btrfs/disk-io.c | 15 +++---
fs/btrfs/file-item.c | 28 ++++-------
fs/btrfs/file.c | 18 +++-----
fs/btrfs/inode.c | 31 +++++--------
fs/btrfs/ioctl.c | 49 ++++++++++----------
fs/btrfs/props.c | 15 ++----
fs/btrfs/reflink.c | 16 ++-----
fs/btrfs/relocation.c | 12 ++---
fs/btrfs/send.c | 40 ++++++++--------
fs/btrfs/tree-log.c | 105 +++++++++++++++---------------------------
fs/btrfs/verity.c | 21 +++------
fs/btrfs/volumes.c | 15 ++----
fs/btrfs/zoned.c | 15 +++---
13 files changed, 145 insertions(+), 235 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 70fc4e7cc5a0..69f5af9d5e92 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2130,17 +2130,14 @@ static int load_global_roots_objectid(struct btrfs_root *tree_root,
if (ret < 0)
break;
- if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
- ret = btrfs_next_leaf(tree_root, path);
- if (ret) {
- if (ret > 0)
- ret = 0;
- break;
- }
+ ret = btrfs_get_next_valid_item(root, &key, path);
+ if (ret > 0) {
+ ret = 0;
+ break;
}
- ret = 0;
+ if (ret < 0)
+ break;
- btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
if (key.objectid != objectid)
break;
btrfs_release_path(path);
diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index c09fbc257634..4612eecade52 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -517,17 +517,13 @@ int btrfs_lookup_csums_list(struct btrfs_root *root, u64 start, u64 end,
while (start <= end) {
u64 csum_end;
+ ret = btrfs_get_next_valid_item(root, &key, path);
+ if (ret < 0)
+ goto out;
+ if (ret > 0)
+ break;
leaf = path->nodes[0];
- if (path->slots[0] >= btrfs_header_nritems(leaf)) {
- ret = btrfs_next_leaf(root, path);
- if (ret < 0)
- goto out;
- if (ret > 0)
- break;
- leaf = path->nodes[0];
- }
- btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
if (key.objectid != BTRFS_EXTENT_CSUM_OBJECTID ||
key.type != BTRFS_EXTENT_CSUM_KEY ||
key.offset > end)
@@ -674,17 +670,13 @@ int btrfs_lookup_csums_bitmap(struct btrfs_root *root, struct btrfs_path *path,
while (start <= end) {
u64 csum_end;
+ ret = btrfs_get_next_valid_item(root, &key, path);
+ if (ret < 0)
+ goto fail;
+ if (ret > 0)
+ break;
leaf = path->nodes[0];
- if (path->slots[0] >= btrfs_header_nritems(leaf)) {
- ret = btrfs_next_leaf(root, path);
- if (ret < 0)
- goto fail;
- if (ret > 0)
- break;
- leaf = path->nodes[0];
- }
- btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
if (key.objectid != BTRFS_EXTENT_CSUM_OBJECTID ||
key.type != BTRFS_EXTENT_CSUM_KEY ||
key.offset > end)
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 204674934795..24eb3285c01f 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -3623,22 +3623,18 @@ static loff_t find_desired_extent(struct file *file, loff_t offset, int whence)
}
while (start < i_size) {
- struct extent_buffer *leaf = path->nodes[0];
+ struct extent_buffer *leaf;
struct btrfs_file_extent_item *extent;
u64 extent_end;
u8 type;
- if (path->slots[0] >= btrfs_header_nritems(leaf)) {
- ret = btrfs_next_leaf(root, path);
- if (ret < 0)
- goto out;
- else if (ret > 0)
- break;
-
- leaf = path->nodes[0];
- }
+ ret = btrfs_get_next_valid_item(root, &key, path);
+ if (ret < 0)
+ goto out;
+ if (ret > 0)
+ break;
+ leaf = path->nodes[0];
- btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
if (key.objectid != ino || key.type != BTRFS_EXTENT_DATA_KEY)
break;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index b77dd22b8cdb..92531693a627 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2089,17 +2089,12 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
check_prev = false;
next_slot:
/* Go to next leaf if we have exhausted the current one */
+ ret = btrfs_get_next_valid_item(root, &found_key, path);
+ if (ret < 0)
+ goto error;
+ if (ret > 0)
+ break;
leaf = path->nodes[0];
- if (path->slots[0] >= btrfs_header_nritems(leaf)) {
- ret = btrfs_next_leaf(root, path);
- if (ret < 0)
- goto error;
- if (ret > 0)
- break;
- leaf = path->nodes[0];
- }
-
- btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
/* Didn't find anything for our INO */
if (found_key.objectid > ino)
@@ -7074,16 +7069,14 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
next:
if (start >= extent_end) {
path->slots[0]++;
- if (path->slots[0] >= btrfs_header_nritems(leaf)) {
- ret = btrfs_next_leaf(root, path);
- if (ret < 0)
- goto out;
- else if (ret > 0)
- goto not_found;
+ ret = btrfs_get_next_valid_item(root, &found_key, path);
+ if (ret < 0)
+ goto out;
+ if (ret > 0)
+ goto not_found;
+
+ leaf = path->nodes[0];
- leaf = path->nodes[0];
- }
- btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
if (found_key.objectid != objectid ||
found_key.type != BTRFS_EXTENT_DATA_KEY)
goto not_found;
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 7e13de2bdcbf..e0a27fd66853 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2126,22 +2126,20 @@ static int btrfs_ioctl_get_subvol_info(struct inode *inode, void __user *argp)
key.type = BTRFS_ROOT_BACKREF_KEY;
key.offset = 0;
ret = btrfs_search_slot(NULL, fs_info->tree_root, &key, path, 0, 0);
- if (ret < 0) {
+ if (ret < 0)
+ goto out;
+
+ ret = btrfs_get_next_valid_item(fs_info->tree_root, &key, path);
+ if (ret < 0)
+ goto out;
+ if (ret > 0) {
+ ret = -EUCLEAN;
goto out;
- } else if (path->slots[0] >=
- btrfs_header_nritems(path->nodes[0])) {
- ret = btrfs_next_leaf(fs_info->tree_root, path);
- if (ret < 0) {
- goto out;
- } else if (ret > 0) {
- ret = -EUCLEAN;
- goto out;
- }
}
leaf = path->nodes[0];
slot = path->slots[0];
- btrfs_item_key_to_cpu(leaf, &key, slot);
+
if (key.objectid == subvol_info->treeid &&
key.type == BTRFS_ROOT_BACKREF_KEY) {
subvol_info->parent_id = key.offset;
@@ -2209,23 +2207,20 @@ static int btrfs_ioctl_get_subvol_rootref(struct btrfs_root *root,
root = root->fs_info->tree_root;
ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
- if (ret < 0) {
+ if (ret < 0)
+ goto out;
+
+ ret = btrfs_get_next_valid_item(root, &key, path);
+ if (ret < 0)
+ goto out;
+ if (ret > 0) {
+ ret = -EUCLEAN;
goto out;
- } else if (path->slots[0] >=
- btrfs_header_nritems(path->nodes[0])) {
- ret = btrfs_next_leaf(root, path);
- if (ret < 0) {
- goto out;
- } else if (ret > 0) {
- ret = -EUCLEAN;
- goto out;
- }
}
- while (1) {
- leaf = path->nodes[0];
- slot = path->slots[0];
+ leaf = path->nodes[0];
+ slot = path->slots[0];
- btrfs_item_key_to_cpu(leaf, &key, slot);
+ while (1) {
if (key.objectid != objectid || key.type != BTRFS_ROOT_REF_KEY) {
ret = 0;
goto out;
@@ -2249,6 +2244,10 @@ static int btrfs_ioctl_get_subvol_rootref(struct btrfs_root *root,
ret = -EUCLEAN;
goto out;
}
+ leaf = path->nodes[0];
+ slot = path->slots[0];
+
+ btrfs_item_key_to_cpu(leaf, &key, slot);
}
out:
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index adc956432d2f..79b4ab5f51f7 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -166,19 +166,14 @@ static int iterate_object_props(struct btrfs_root *root,
int slot;
const struct hlist_head *handlers;
+ ret = btrfs_get_next_valid_item(root, &key, path);
+ if (ret < 0)
+ goto out;
+ else if (ret > 0)
+ break;
slot = path->slots[0];
leaf = path->nodes[0];
- if (slot >= btrfs_header_nritems(leaf)) {
- ret = btrfs_next_leaf(root, path);
- if (ret < 0)
- goto out;
- else if (ret > 0)
- break;
- continue;
- }
-
- btrfs_item_key_to_cpu(leaf, &key, slot);
if (key.objectid != objectid)
break;
if (key.type != BTRFS_XATTR_ITEM_KEY)
diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c
index ce25ab7f0e99..c70940c15d6b 100644
--- a/fs/btrfs/reflink.c
+++ b/fs/btrfs/reflink.c
@@ -345,7 +345,6 @@ static int btrfs_clone(struct inode *src, struct inode *inode,
struct btrfs_trans_handle *trans;
char *buf = NULL;
struct btrfs_key key;
- u32 nritems;
int slot;
int ret;
const u64 len = olen_aligned;
@@ -397,20 +396,15 @@ static int btrfs_clone(struct inode *src, struct inode *inode,
path->slots[0]--;
}
- nritems = btrfs_header_nritems(path->nodes[0]);
process_slot:
- if (path->slots[0] >= nritems) {
- ret = btrfs_next_leaf(BTRFS_I(src)->root, path);
- if (ret < 0)
- goto out;
- if (ret > 0)
- break;
- nritems = btrfs_header_nritems(path->nodes[0]);
- }
+ ret = btrfs_get_next_valid_item(BTRFS_I(src)->root, &key, path);
+ if (ret < 0)
+ goto out;
+ if (ret > 0)
+ break;
leaf = path->nodes[0];
slot = path->slots[0];
- btrfs_item_key_to_cpu(leaf, &key, slot);
if (key.type > BTRFS_EXTENT_DATA_KEY ||
key.objectid != btrfs_ino(BTRFS_I(src)))
break;
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index e58151933844..85af65a794c6 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3342,7 +3342,6 @@ int find_next_extent(struct reloc_control *rc, struct btrfs_path *path,
{
struct btrfs_fs_info *fs_info = rc->extent_root->fs_info;
struct btrfs_key key;
- struct extent_buffer *leaf;
u64 start, end, last;
int ret;
@@ -3367,15 +3366,10 @@ int find_next_extent(struct reloc_control *rc, struct btrfs_path *path,
if (ret < 0)
break;
next:
- leaf = path->nodes[0];
- if (path->slots[0] >= btrfs_header_nritems(leaf)) {
- ret = btrfs_next_leaf(rc->extent_root, path);
- if (ret != 0)
- break;
- leaf = path->nodes[0];
- }
+ ret = btrfs_get_next_valid_item(rc->extent_root, &key, path);
+ if (ret != 0)
+ break;
- btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
if (key.objectid >= last) {
ret = 1;
break;
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 7664025a5af4..3c17f3b0794c 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -5887,8 +5887,8 @@ static int clone_range(struct send_ctx *sctx, struct btrfs_path *dst_path,
}
while (true) {
- struct extent_buffer *leaf = path->nodes[0];
- int slot = path->slots[0];
+ struct extent_buffer *leaf;
+ int slot;
struct btrfs_file_extent_item *ei;
u8 type;
u64 ext_len;
@@ -5896,16 +5896,14 @@ static int clone_range(struct send_ctx *sctx, struct btrfs_path *dst_path,
u64 clone_data_offset;
bool crossed_src_i_size = false;
- if (slot >= btrfs_header_nritems(leaf)) {
- ret = btrfs_next_leaf(clone_root->root, path);
- if (ret < 0)
- goto out;
- else if (ret > 0)
- break;
- continue;
- }
+ ret = btrfs_get_next_valid_item(clone_root->root, &key, path);
+ if (ret < 0)
+ goto out;
+ if (ret > 0)
+ break;
- btrfs_item_key_to_cpu(leaf, &key, slot);
+ leaf = path->nodes[0];
+ slot = path->slots[0];
/*
* We might have an implicit trailing hole (NO_HOLES feature
@@ -6400,21 +6398,19 @@ static int range_is_hole_in_parent(struct send_ctx *sctx,
path->slots[0]--;
while (search_start < end) {
- struct extent_buffer *leaf = path->nodes[0];
- int slot = path->slots[0];
+ struct extent_buffer *leaf;
+ int slot;
struct btrfs_file_extent_item *fi;
u64 extent_end;
- if (slot >= btrfs_header_nritems(leaf)) {
- ret = btrfs_next_leaf(root, path);
- if (ret < 0)
- goto out;
- else if (ret > 0)
- break;
- continue;
- }
+ ret = btrfs_get_next_valid_item(root, &key, path);
+ if (ret < 0)
+ goto out;
+ if (ret > 0)
+ break;
- btrfs_item_key_to_cpu(leaf, &key, slot);
+ leaf = path->nodes[0];
+ slot = path->slots[0];
if (key.objectid < sctx->cur_ino ||
key.type < BTRFS_EXTENT_DATA_KEY)
goto next;
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 9f05d454b9df..d107c4f8bd4b 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -2077,7 +2077,6 @@ static noinline int find_dir_range(struct btrfs_root *root,
u64 found_end;
struct btrfs_dir_log_item *item;
int ret;
- int nritems;
if (*start_ret == (u64)-1)
return 1;
@@ -2114,15 +2113,10 @@ static noinline int find_dir_range(struct btrfs_root *root,
ret = 1;
next:
/* check the next slot in the tree to see if it is a valid item */
- nritems = btrfs_header_nritems(path->nodes[0]);
path->slots[0]++;
- if (path->slots[0] >= nritems) {
- ret = btrfs_next_leaf(root, path);
- if (ret)
- goto out;
- }
-
- btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
+ ret = btrfs_get_next_valid_item(root, &key, path);
+ if (ret)
+ goto out;
if (key.type != BTRFS_DIR_LOG_INDEX_KEY || key.objectid != dirid) {
ret = 1;
@@ -2378,22 +2372,17 @@ static noinline int replay_dir_deletes(struct btrfs_trans_handle *trans,
dir_key.offset = range_start;
while (1) {
- int nritems;
ret = btrfs_search_slot(NULL, root, &dir_key, path,
0, 0);
if (ret < 0)
goto out;
- nritems = btrfs_header_nritems(path->nodes[0]);
- if (path->slots[0] >= nritems) {
- ret = btrfs_next_leaf(root, path);
- if (ret == 1)
- break;
- else if (ret < 0)
- goto out;
- }
- btrfs_item_key_to_cpu(path->nodes[0], &found_key,
- path->slots[0]);
+ ret = btrfs_get_next_valid_item(root, &found_key, path);
+ if (ret > 0)
+ break;
+ if (ret < 0)
+ goto out;
+
if (found_key.objectid != dirid ||
found_key.type != dir_key.type) {
ret = 0;
@@ -5197,20 +5186,14 @@ static int btrfs_log_holes(struct btrfs_trans_handle *trans,
return ret;
while (true) {
- struct extent_buffer *leaf = path->nodes[0];
-
- if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
- ret = btrfs_next_leaf(root, path);
- if (ret < 0)
- return ret;
- if (ret > 0) {
- ret = 0;
- break;
- }
- leaf = path->nodes[0];
+ ret = btrfs_get_next_valid_item(root, &key, path);
+ if (ret < 0)
+ return ret;
+ if (ret > 0) {
+ ret = 0;
+ break;
}
- btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
if (key.objectid != ino || key.type != BTRFS_EXTENT_DATA_KEY)
break;
@@ -5242,7 +5225,6 @@ static int btrfs_log_holes(struct btrfs_trans_handle *trans,
return ret;
if (WARN_ON(ret > 0))
return -ENOENT;
- leaf = path->nodes[0];
}
prev_extent_end = btrfs_file_extent_end(path);
@@ -6800,22 +6782,21 @@ static int btrfs_log_all_parents(struct btrfs_trans_handle *trans,
goto out;
while (true) {
- struct extent_buffer *leaf = path->nodes[0];
- int slot = path->slots[0];
+ struct extent_buffer *leaf;
+ int slot;
u32 cur_offset = 0;
u32 item_size;
unsigned long ptr;
- if (slot >= btrfs_header_nritems(leaf)) {
- ret = btrfs_next_leaf(root, path);
- if (ret < 0)
- goto out;
- else if (ret > 0)
- break;
- continue;
- }
+ ret = btrfs_get_next_valid_item(root, &key, path);
+ if (ret < 0)
+ goto out;
+ if (ret > 0)
+ break;
+
+ leaf = path->nodes[0];
+ slot = path->slots[0];
- btrfs_item_key_to_cpu(leaf, &key, slot);
/* BTRFS_INODE_EXTREF_KEY is BTRFS_INODE_REF_KEY + 1 */
if (key.objectid != ino || key.type > BTRFS_INODE_EXTREF_KEY)
break;
@@ -6904,8 +6885,6 @@ static int log_new_ancestors(struct btrfs_trans_handle *trans,
btrfs_item_key_to_cpu(path->nodes[0], &found_key, path->slots[0]);
while (true) {
- struct extent_buffer *leaf;
- int slot;
struct btrfs_key search_key;
struct btrfs_inode *inode;
u64 ino;
@@ -6937,19 +6916,12 @@ static int log_new_ancestors(struct btrfs_trans_handle *trans,
if (ret < 0)
return ret;
- leaf = path->nodes[0];
- slot = path->slots[0];
- if (slot >= btrfs_header_nritems(leaf)) {
- ret = btrfs_next_leaf(root, path);
- if (ret < 0)
- return ret;
- else if (ret > 0)
- return -ENOENT;
- leaf = path->nodes[0];
- slot = path->slots[0];
- }
+ ret = btrfs_get_next_valid_item(root, &found_key, path);
+ if (ret < 0)
+ return ret;
+ if (ret > 0)
+ return -ENOENT;
- btrfs_item_key_to_cpu(leaf, &found_key, slot);
if (found_key.objectid != search_key.objectid ||
found_key.type != BTRFS_INODE_REF_KEY)
return -ENOENT;
@@ -7028,20 +7000,15 @@ static int log_all_new_ancestors(struct btrfs_trans_handle *trans,
path->slots[0]++;
while (true) {
- struct extent_buffer *leaf = path->nodes[0];
- int slot = path->slots[0];
struct btrfs_key found_key;
- if (slot >= btrfs_header_nritems(leaf)) {
- ret = btrfs_next_leaf(root, path);
- if (ret < 0)
- goto out;
- else if (ret > 0)
- break;
- continue;
- }
+ ret = btrfs_get_next_valid_item(root, &found_key, path);
+ if (ret < 0)
+ goto out;
+ if (ret > 0)
+ break;
+
- btrfs_item_key_to_cpu(leaf, &found_key, slot);
if (found_key.objectid != ino ||
found_key.type > BTRFS_INODE_EXTREF_KEY)
break;
diff --git a/fs/btrfs/verity.c b/fs/btrfs/verity.c
index b7a96a005487..f517d1f7a1da 100644
--- a/fs/btrfs/verity.c
+++ b/fs/btrfs/verity.c
@@ -332,8 +332,14 @@ static int read_key_bytes(struct btrfs_inode *inode, u8 key_type, u64 offset,
}
while (len > 0) {
+ ret = btrfs_get_next_valid_item(root, &key, path);
+ if (ret < 0)
+ break;
+ if (ret > 0) {
+ ret = 0;
+ break;
+ }
leaf = path->nodes[0];
- btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
if (key.objectid != btrfs_ino(inode) || key.type != key_type)
break;
@@ -389,19 +395,6 @@ static int read_key_bytes(struct btrfs_inode *inode, u8 key_type, u64 offset,
copied += copy_bytes;
path->slots[0]++;
- if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
- /*
- * We've reached the last slot in this leaf and we need
- * to go to the next leaf.
- */
- ret = btrfs_next_leaf(root, path);
- if (ret < 0) {
- break;
- } else if (ret > 0) {
- ret = 0;
- break;
- }
- }
}
out:
btrfs_free_path(path);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index fa7a929a0461..70e73cbf6b16 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1723,18 +1723,13 @@ static int find_free_dev_extent(struct btrfs_device *device, u64 num_bytes,
goto out;
while (search_start < search_end) {
+ ret = btrfs_get_next_valid_item(root, &key, path);
+ if (ret < 0)
+ goto out;
+ if (ret > 0)
+ break;
l = path->nodes[0];
slot = path->slots[0];
- if (slot >= btrfs_header_nritems(l)) {
- ret = btrfs_next_leaf(root, path);
- if (ret == 0)
- continue;
- if (ret < 0)
- goto out;
-
- break;
- }
- btrfs_item_key_to_cpu(l, &key, slot);
if (key.objectid < device->devid)
goto next;
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 245e813ecd78..8fe5b7e362c0 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -306,14 +306,13 @@ static int calculate_emulated_zone_size(struct btrfs_fs_info *fs_info)
if (ret < 0)
return ret;
- if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
- ret = btrfs_next_leaf(root, path);
- if (ret < 0)
- return ret;
- /* No dev extents at all? Not good */
- if (ret > 0)
- return -EUCLEAN;
- }
+ ret = btrfs_get_next_valid_item(root, &key, path);
+ if (ret < 0)
+ return ret;
+ /* No dev extents at all? Not good */
+ if (ret > 0)
+ return -EUCLEAN;
+
leaf = path->nodes[0];
dext = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_dev_extent);
--
2.50.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] btrfs: replace manual next item search with btrfs_get_next_valid_item()
2025-07-30 14:53 [PATCH] btrfs: replace manual next item search with btrfs_get_next_valid_item() Sun YangKai
@ 2025-07-30 22:07 ` Qu Wenruo
2025-07-31 14:19 ` Sun YangKai
0 siblings, 1 reply; 3+ messages in thread
From: Qu Wenruo @ 2025-07-30 22:07 UTC (permalink / raw)
To: Sun YangKai; +Cc: linux-btrfs
在 2025/7/31 00:23, Sun YangKai 写道:
> The function btrfs_get_next_valid_item() was introduced in
> commit 62142be363ae9("btrfs: introduce btrfs_for_each_slot iterator macro")
> and has never been used in other places other than
> the btrfs_for_each_slot macro.
That's because the function name is not that straight-forward.
The name btrfs_get_"next"_valid_item() implies it would move to the next
slot, but it's not the case.
Thus this can lead to quite some confusion and we're not that excited to
use it.
One example inlined below:
[...]
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index fa7a929a0461..70e73cbf6b16 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1723,18 +1723,13 @@ static int find_free_dev_extent(struct btrfs_device *device, u64 num_bytes,
> goto out;
>
> while (search_start < search_end) {
> + ret = btrfs_get_next_valid_item(root, &key, path);
> + if (ret < 0)
> + goto out;
> + if (ret > 0)
> + break;
Here just by the name, it looks like it will move to the next slot thus
can be problematic (as it will skip the current slot).
But it's not the case, as if the current slot is not beyond the current
leaf, we just return with the current item key stored into @key.
Another thing is, we may not need to bother the @key, or the caller will
manually check the item key anyway. Compared to the confusing name, this
is just a minor problem though.
But still it modifies the @key, which may or may not be what we want.
That's why we are not excited to use the new helper.
Thanks,
Qu
> l = path->nodes[0];
> slot = path->slots[0];
> - if (slot >= btrfs_header_nritems(l)) {
> - ret = btrfs_next_leaf(root, path);
> - if (ret == 0)
> - continue;
> - if (ret < 0)
> - goto out;
> -
> - break;
> - }
> - btrfs_item_key_to_cpu(l, &key, slot);
>
> if (key.objectid < device->devid)
> goto next;
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 245e813ecd78..8fe5b7e362c0 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -306,14 +306,13 @@ static int calculate_emulated_zone_size(struct btrfs_fs_info *fs_info)
> if (ret < 0)
> return ret;
>
> - if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
> - ret = btrfs_next_leaf(root, path);
> - if (ret < 0)
> - return ret;
> - /* No dev extents at all? Not good */
> - if (ret > 0)
> - return -EUCLEAN;
> - }
> + ret = btrfs_get_next_valid_item(root, &key, path);
> + if (ret < 0)
> + return ret;
> + /* No dev extents at all? Not good */
> + if (ret > 0)
> + return -EUCLEAN;
> +
>
> leaf = path->nodes[0];
> dext = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_dev_extent);
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] btrfs: replace manual next item search with btrfs_get_next_valid_item()
2025-07-30 22:07 ` Qu Wenruo
@ 2025-07-31 14:19 ` Sun YangKai
0 siblings, 0 replies; 3+ messages in thread
From: Sun YangKai @ 2025-07-31 14:19 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
> 在 2025/7/31 00:23, Sun YangKai 写道:
> > The function btrfs_get_next_valid_item() was introduced in
> > commit 62142be363ae9("btrfs: introduce btrfs_for_each_slot iterator
> > macro")
> > and has never been used in other places other than
> > the btrfs_for_each_slot macro.
>
> That's because the function name is not that straight-forward.
>
> The name btrfs_get_"next"_valid_item() implies it would move to the next
> slot, but it's not the case.
>
> Thus this can lead to quite some confusion and we're not that excited to
> use it.
Great point, and I also find the name is kind of confusing. Maybe we can have a
better name like btrfs_ensure_valid_item() or something similar.
> One example inlined below:
>
> [...]
>
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index fa7a929a0461..70e73cbf6b16 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -1723,18 +1723,13 @@ static int find_free_dev_extent(struct
> > btrfs_device *device, u64 num_bytes,>
> > goto out;
> >
> > while (search_start < search_end) {
> >
> > + ret = btrfs_get_next_valid_item(root, &key, path);
> > + if (ret < 0)
> > + goto out;
> > + if (ret > 0)
> > + break;
>
> Here just by the name, it looks like it will move to the next slot thus
> can be problematic (as it will skip the current slot).
>
> But it's not the case, as if the current slot is not beyond the current
> leaf, we just return with the current item key stored into @key.
>
>
> Another thing is, we may not need to bother the @key, or the caller will
> manually check the item key anyway. Compared to the confusing name, this
> is just a minor problem though.
>
> But still it modifies the @key, which may or may not be what we want.
If I recall correctly, there's only one case that we don't want to bother with
the @key in this patch, and I'll discard that change in the future versions
if necessary. In all other cases, we just read the key according to the path,
which is part of the pattern I want to replace with this helper.
> That's why we are not excited to use the new helper.
>
> Thanks,
> Qu
>
> > l = path->nodes[0];
> > slot = path->slots[0];
> >
> > - if (slot >= btrfs_header_nritems(l)) {
> > - ret = btrfs_next_leaf(root, path);
> > - if (ret == 0)
> > - continue;
> > - if (ret < 0)
> > - goto out;
> > -
> > - break;
> > - }
> > - btrfs_item_key_to_cpu(l, &key, slot);
> >
> > if (key.objectid < device->devid)
> >
> > goto next;
> >
Thanks,
YangKai
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-07-31 14:19 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-30 14:53 [PATCH] btrfs: replace manual next item search with btrfs_get_next_valid_item() Sun YangKai
2025-07-30 22:07 ` Qu Wenruo
2025-07-31 14:19 ` Sun YangKai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox