[PATCH v3] btrfs: fix nonzero lowest level handling in btrfs_search_forward()

[PATCH v3] btrfs: fix nonzero lowest level handling in btrfs_search_forward()

[Sun YangKai]

Commit 323ac95bce44 ("Btrfs: don't read leaf blocks containing only
checksums during truncate") changed the condition from `level == 0` to
`level == path->lowest_level`, while its origional purpose is just to do
some leaf nodes handling (calling btrfs_item_key_to_cpu()) and skip some
code that doesn't fit leaf nodes.

After changing the condition, the code path
1. also handle the non-leaf nodes when path->lowest_level is nonzero,
   which is wrong. However, it seems that btrfs_search_forward() is never
   called with a nonzero path->lowest_level, which makes this bug not
   found before.
2. makes the later if block with the same condition, which is origionally
   used to handle non-leaf node (calling btrfs_node_key_to_cpu()) when
   lowest_level is not zero, dead code.

This changes the behavior when btrfs_search_forward() is called with
nonzero path->lowest_level. But this never happens in the current code
base, and the previous behavior is wrong. So the change of behavior will
not be a problem.

Fix: commit 323ac95bce44 ("Btrfs: don't read leaf blocks containing only checksums during truncate")
Signed-off-by: Sun YangKai <sunk67188@gmail.com>
---
 fs/btrfs/ctree.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index a2e7979372cc..56a49d85b2a4 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -4585,16 +4585,13 @@ int btrfs_del_items(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 
 /*
  * A helper function to walk down the tree starting at min_key, and looking
- * for nodes or leaves that are have a minimum transaction id.
+ * for leaves that are have a minimum transaction id.
  * This is used by the btree defrag code, and tree logging
  *
  * This does not cow, but it does stuff the starting key it finds back
  * into min_key, so you can call btrfs_search_slot with cow=1 on the
  * key and get a writable path.
  *
- * This honors path->lowest_level to prevent descent past a given level
- * of the tree.
- *
  * min_trans indicates the oldest transaction that you are interested
  * in walking through.  Any nodes or leaves older than min_trans are
  * skipped over (without reading them).
@@ -4615,6 +4612,7 @@ int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key,
 	int keep_locks = path->keep_locks;
 
 	ASSERT(!path->nowait);
+	ASSERT(path->lowest_level == 0);
 	path->keep_locks = 1;
 again:
 	cur = btrfs_read_lock_root_node(root);
@@ -4636,8 +4634,8 @@ int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key,
 			goto out;
 		}
 
-		/* at the lowest level, we're done, setup the path and exit */
-		if (level == path->lowest_level) {
+		/* at the level 0, we're done, setup the path and exit */
+		if (level == 0) {
 			if (slot >= nritems)
 				goto find_next_key;
 			ret = 0;
@@ -4678,12 +4676,6 @@ int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key,
 				goto out;
 			}
 		}
-		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);
 		if (IS_ERR(cur)) {
 			ret = PTR_ERR(cur);
@@ -4699,7 +4691,7 @@ int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key,
 out:
 	path->keep_locks = keep_locks;
 	if (ret == 0)
-		btrfs_unlock_up_safe(path, path->lowest_level + 1);
+		btrfs_unlock_up_safe(path, 1);
 	return ret;
 }
 
-- 
2.49.0

Re: [PATCH v3] btrfs: fix nonzero lowest level handling in btrfs_search_forward()

[Sun YangKai]

> Commit 323ac95bce44 ("Btrfs: don't read leaf blocks containing only
> checksums during truncate") changed the condition from `level == 0` to
> `level == path->lowest_level`, while its origional purpose is just to do
> some leaf nodes handling (calling btrfs_item_key_to_cpu()) and skip some
> code that doesn't fit leaf nodes.
> 
> After changing the condition, the code path
> 1. also handle the non-leaf nodes when path->lowest_level is nonzero,
>    which is wrong. However, it seems that btrfs_search_forward() is never
>    called with a nonzero path->lowest_level, which makes this bug not
>    found before.
> 2. makes the later if block with the same condition, which is origionally
>    used to handle non-leaf node (calling btrfs_node_key_to_cpu()) when
>    lowest_level is not zero, dead code.
> 
> This changes the behavior when btrfs_search_forward() is called with
> nonzero path->lowest_level. But this never happens in the current code
> base, and the previous behavior is wrong. So the change of behavior will
> not be a problem.
> 
> Fix: commit 323ac95bce44 ("Btrfs: don't read leaf blocks containing only
> checksums during truncate") Signed-off-by: Sun YangKai
> <sunk67188@gmail.com>
> ---
>  fs/btrfs/ctree.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index a2e7979372cc..56a49d85b2a4 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -4585,16 +4585,13 @@ int btrfs_del_items(struct btrfs_trans_handle
> *trans, struct btrfs_root *root,
> 
>  /*
>   * A helper function to walk down the tree starting at min_key, and looking
> - * for nodes or leaves that are have a minimum transaction id.
> + * for leaves that are have a minimum transaction id.
>   * This is used by the btree defrag code, and tree logging
>   *
>   * This does not cow, but it does stuff the starting key it finds back
>   * into min_key, so you can call btrfs_search_slot with cow=1 on the
>   * key and get a writable path.
>   *
> - * This honors path->lowest_level to prevent descent past a given level
> - * of the tree.
> - *
>   * min_trans indicates the oldest transaction that you are interested
>   * in walking through.  Any nodes or leaves older than min_trans are
>   * skipped over (without reading them).
> @@ -4615,6 +4612,7 @@ int btrfs_search_forward(struct btrfs_root *root,
> struct btrfs_key *min_key, int keep_locks = path->keep_locks;
> 
>  	ASSERT(!path->nowait);
> +	ASSERT(path->lowest_level == 0);
>  	path->keep_locks = 1;
>  again:
>  	cur = btrfs_read_lock_root_node(root);
> @@ -4636,8 +4634,8 @@ int btrfs_search_forward(struct btrfs_root *root,
> struct btrfs_key *min_key, goto out;
>  		}
> 
> -		/* at the lowest level, we're done, setup the path and exit */
> -		if (level == path->lowest_level) {
> +		/* at the level 0, we're done, setup the path and exit */
> +		if (level == 0) {
>  			if (slot >= nritems)
>  				goto find_next_key;
>  			ret = 0;
> @@ -4678,12 +4676,6 @@ int btrfs_search_forward(struct btrfs_root *root,
> struct btrfs_key *min_key, goto out;
>  			}
>  		}
> -		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);
>  		if (IS_ERR(cur)) {
>  			ret = PTR_ERR(cur);
> @@ -4699,7 +4691,7 @@ int btrfs_search_forward(struct btrfs_root *root,
> struct btrfs_key *min_key, out:
>  	path->keep_locks = keep_locks;
>  	if (ret == 0)
> -		btrfs_unlock_up_safe(path, path->lowest_level + 1);
> +		btrfs_unlock_up_safe(path, 1);
>  	return ret;
>  }

This patch is suggest by Qu Wenruo.
Should I add a line like
Suggest-by: Qu Wenruo <wqu@suse.com>

Re: [PATCH v3] btrfs: fix nonzero lowest level handling in btrfs_search_forward()

[Filipe Manana]

On Thu, Jun 12, 2025 at 9:35 AM Sun YangKai <sunk67188@gmail.com> wrote:
>
> Commit 323ac95bce44 ("Btrfs: don't read leaf blocks containing only
> checksums during truncate") changed the condition from `level == 0` to
> `level == path->lowest_level`, while its origional purpose is just to do

origional -> original

> some leaf nodes handling (calling btrfs_item_key_to_cpu()) and skip some
> code that doesn't fit leaf nodes.
>
> After changing the condition, the code path

Add a full colon at the end, so "... the code path:"
Plus a blank line for better readability.

> 1. also handle the non-leaf nodes when path->lowest_level is nonzero,

also -> Also, it's the start of a sentence.

>    which is wrong. However, it seems that btrfs_search_forward() is never

Don't say that "it seems" - that gives the idea you are not sure and
haven't checked it.
It's true that it's never called with a path->lowest_level > 0.

>    called with a nonzero path->lowest_level, which makes this bug not
>    found before.

Another blank line here improves readability.

> 2. makes the later if block with the same condition, which is origionally

makes -> Makes
origionally -> originally

>    used to handle non-leaf node (calling btrfs_node_key_to_cpu()) when
>    lowest_level is not zero, dead code.
>
> This changes the behavior when btrfs_search_forward() is called with
> nonzero path->lowest_level. But this never happens in the current code
> base, and the previous behavior is wrong. So the change of behavior will
> not be a problem.

Just say something more straightforward and clear such as:

"Since btrfs_search_forward() is nevel called for a path with a
lowest_level different from zero,
just completely remove the partial support for a non-zero
lowest_level, simplifying a bit the code,
and assert that lowest_level is zero at the start of the function."

>
> Fix: commit 323ac95bce44 ("Btrfs: don't read leaf blocks containing only checksums during truncate")

The correct format is:

Fixes: 323ac95bce44 ("Btrfs: don't read leaf blocks containing only
checksums during truncate")

> Signed-off-by: Sun YangKai <sunk67188@gmail.com>
> ---
>  fs/btrfs/ctree.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index a2e7979372cc..56a49d85b2a4 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -4585,16 +4585,13 @@ int btrfs_del_items(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>
>  /*
>   * A helper function to walk down the tree starting at min_key, and looking
> - * for nodes or leaves that are have a minimum transaction id.
> + * for leaves that are have a minimum transaction id.

Since we are updating the comment, it's a good opportunity to fix the grammar.
Replace "that are have a minimum..." with just "that have a minimum...".

>   * This is used by the btree defrag code, and tree logging
>   *
>   * This does not cow, but it does stuff the starting key it finds back
>   * into min_key, so you can call btrfs_search_slot with cow=1 on the
>   * key and get a writable path.
>   *
> - * This honors path->lowest_level to prevent descent past a given level
> - * of the tree.
> - *
>   * min_trans indicates the oldest transaction that you are interested
>   * in walking through.  Any nodes or leaves older than min_trans are
>   * skipped over (without reading them).
> @@ -4615,6 +4612,7 @@ int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key,
>         int keep_locks = path->keep_locks;
>
>         ASSERT(!path->nowait);
> +       ASSERT(path->lowest_level == 0);
>         path->keep_locks = 1;
>  again:
>         cur = btrfs_read_lock_root_node(root);
> @@ -4636,8 +4634,8 @@ int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key,
>                         goto out;
>                 }
>
> -               /* at the lowest level, we're done, setup the path and exit */
> -               if (level == path->lowest_level) {
> +               /* at the level 0, we're done, setup the path and exit */

Since the comment is being modified, it's a good opportunity to fix
the style for what we prefer nowadays: capitalize the first word and
add ending punctuation.

Finally, answering your question from the reply you made later:

Yes, "Suggested-by: Qu Wenruo <wqu@suse.com>" can be added, but
there's no need to resend a patch just for that.

Also note that the tag is Suggested-by, and not Suggest-by as you typed.

If you agree, I can update the patch with all those changes and add it
to for-next, let me know.

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.

> +               if (level == 0) {
>                         if (slot >= nritems)
>                                 goto find_next_key;
>                         ret = 0;
> @@ -4678,12 +4676,6 @@ int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key,
>                                 goto out;
>                         }
>                 }
> -               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);
>                 if (IS_ERR(cur)) {
>                         ret = PTR_ERR(cur);
> @@ -4699,7 +4691,7 @@ int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key,
>  out:
>         path->keep_locks = keep_locks;
>         if (ret == 0)
> -               btrfs_unlock_up_safe(path, path->lowest_level + 1);
> +               btrfs_unlock_up_safe(path, 1);
>         return ret;
>  }
>
> --
> 2.49.0
>
>