Re: [PATCH] btrfs: fix node balancing condition in balance_level()

[Boris Burkov <boris@bur.io>]

On Tue, Aug 05, 2025 at 11:57:04AM +0800, Sun YangKai wrote:
> Commit cfbb9308463f ("Btrfs: balance btree more often") intended to
> trigger node balancing when node utilization drops below 50% (capacity/2)
> by modifying the condition in setup_nodes_for_search(). However, an
> undetected early return condition in balance_level() prevented this
> behavior from taking effect.
> 
> The early return condition:
>     if (btrfs_header_nritems(mid) > BTRFS_NODEPTRS_PER_BLOCK(fs_info) / 4)
>         return 0;
> 
> caused balance_level() to abort when nodes were still more than 25% full,
> effectively maintaining the original 25% threshold. This unintended
> behavior persisted for over a decade. Since setup_nodes_for_search() is
> the sole caller of balance_level(), remove the obsolete early return
> condition to:
> 
> 1. Align with the original intent of commit cfbb9308463f ("Btrfs: balance btree more often")
> 2. Allow proper node balancing at the 50% utilization threshold
> 3. Improve btree performance by more frequent node compaction

This is interesting, good catch.

However, we don't really have any evidence one way or the other which is
better. For better or worse, this logic has been around since ~2007, so
to change it I think requires more justification than the reasoning on a
faulty patch from 2009. Do you have any evidence for your #3:
  > Improve btree performance by more frequent node compaction

I would advise that you run:
- fsperf for generic benchmarking.
- a targeted workload that would get compacted more now that you removed
  the surprising check.

Thanks,
Boris

> 
> Fixes: cfbb9308463f ("Btrfs: balance btree more often")
> Signed-off-by: Sun YangKai <sunk67188@gmail.com>
> ---
>  fs/btrfs/ctree.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index acd85e317564..8cc52c8b38f3 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -939,9 +939,6 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
>  		}
>  		return 0;
>  	}
> -	if (btrfs_header_nritems(mid) >
> -	    BTRFS_NODEPTRS_PER_BLOCK(fs_info) / 4)
> -		return 0;
>  
>  	if (pslot) {
>  		left = btrfs_read_node_slot(parent, pslot - 1);
> -- 
> 2.50.1
>