btree: fix merge logic to use btree_last() return value

Posted by Guan-Chun.Wu 1 month, 1 week ago

Previously btree_merge() called btree_last() only to test existence,
then performed an extra btree_lookup() to fetch the value. This patch
changes it to directly use the value returned by btree_last(), avoiding
redundant lookups and simplifying the merge loop.

Signed-off-by: Guan-Chun.Wu <409411716@gms.tku.edu.tw>
---
 lib/btree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/btree.c b/lib/btree.c
index bb81d3393ac5..9c80c0c7bba8 100644
--- a/lib/btree.c
+++ b/lib/btree.c
@@ -653,9 +653,9 @@ int btree_merge(struct btree_head *target, struct btree_head *victim,
 	 * walks to remove a single object from the victim.
 	 */
 	for (;;) {
-		if (!btree_last(victim, geo, key))
+		val = btree_last(victim, geo, key);
+		if (!val)
 			break;
-		val = btree_lookup(victim, geo, key);
 		err = btree_insert(target, geo, key, val, gfp);
 		if (err)
 			return err;
-- 
2.34.1

Re: btree: fix merge logic to use btree_last() return value

Posted by Kuan-Wei Chiu 1 month, 1 week ago

Hi Guan-Chun,

On Fri, Aug 22, 2025 at 04:58:51PM +0800, Guan-Chun.Wu wrote:
> Previously btree_merge() called btree_last() only to test existence,
> then performed an extra btree_lookup() to fetch the value. This patch
> changes it to directly use the value returned by btree_last(), avoiding
> redundant lookups and simplifying the merge loop.

The code change itself looks correct.

However, the subject line gives the impression that this patch fixes a
bug, while it actually just simplifies the logic. Could you consider
updating the subject to better reflect the nature of the change?

BTW, it seems that only the qla2xxx SCSI driver uses the btree
library, and that driver doesn't actually call btree_merge(). So in
practice, this function is unused in the kernel. Should we consider
removing it entirely?

> 
> Signed-off-by: Guan-Chun.Wu <409411716@gms.tku.edu.tw>

Is the dot in your real name intentional?

Regards,
Kuan-Wei

> ---
>  lib/btree.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/btree.c b/lib/btree.c
> index bb81d3393ac5..9c80c0c7bba8 100644
> --- a/lib/btree.c
> +++ b/lib/btree.c
> @@ -653,9 +653,9 @@ int btree_merge(struct btree_head *target, struct btree_head *victim,
>  	 * walks to remove a single object from the victim.
>  	 */
>  	for (;;) {
> -		if (!btree_last(victim, geo, key))
> +		val = btree_last(victim, geo, key);
> +		if (!val)
>  			break;
> -		val = btree_lookup(victim, geo, key);
>  		err = btree_insert(target, geo, key, val, gfp);
>  		if (err)
>  			return err;
> -- 
> 2.34.1
> 
>

btree: fix merge logic to use btree_last() return value

Posted by Guan-Chun Wu 1 month, 1 week ago

Hi Kuan-Wei,

Thanks for the review.

> On Fri, Aug 22, 2025 at 04:58:51PM +0800, Guan-Chun.Wu wrote:
>> Previously btree_merge() called btree_last() only to test existence,
>> then performed an extra btree_lookup() to fetch the value. This patch
>> changes it to directly use the value returned by btree_last(), avoiding
>> redundant lookups and simplifying the merge loop.
>
> The code change itself looks correct.
>
> However, the subject line gives the impression that this patch fixes a
> bug, while it actually just simplifies the logic. Could you consider
> updating the subject to better reflect the nature of the change?

Good point. I will update the subject to:
    btree: simplify merge logic by using btree_last() return value

> BTW, it seems that only the qla2xxx SCSI driver uses the btree
> library, and that driver doesn't actually call btree_merge(). So in
> practice, this function is unused in the kernel. Should we consider
> removing it entirely?

That makes sense. Since btree_merge() is currently unused, maybe it's
worth considering removal in a follow-up patch.

> Signed-off-by: Guan-Chun.Wu <409411716@gms.tku.edu.tw>
>
> Is the dot in your real name intentional?

No, it was unintentional. My correct name should be "Guan-Chun Wu"
(without the dot). I have updated my git config so future submissions
will use the correct name.

Thanks,
Guan-Chun

> ---
>  lib/btree.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/btree.c b/lib/btree.c
> index bb81d3393ac5..9c80c0c7bba8 100644
> --- a/lib/btree.c
> +++ b/lib/btree.c
> @@ -653,9 +653,9 @@ int btree_merge(struct btree_head *target, struct btree_head *victim,
>        * walks to remove a single object from the victim.
>        */
>       for (;;) {
> -         if (!btree_last(victim, geo, key))
> +         val = btree_last(victim, geo, key);
> +         if (!val)
>               break;
> -         val = btree_lookup(victim, geo, key);
>           err = btree_insert(target, geo, key, val, gfp);
>           if (err)
>               return err;
> -- 
> 2.34.1