Re: [PATCH] btrfs: keep unused block groups queued when a pass fails

[Qu Wenruo <wqu@suse.com>]

在 2026/9/17 07:47, Boris Burkov 写道:
> Once any block_group sets ret!=0 in the main loop of
> btrfs_delete_unused_bgs(), the check
>    if (ret || btrfs_mixed_space_info(space_info)) {
>            btrfs_put_block_group(block_group);
>            continue;
>    }
> skips the rest of the unused bgs while unlinking them from
> fs_info->unused_bgs. There is no "level triggered" re-queueing of empty
> block groups onto fs_info->unused_bgs so it is possible to leak quite a
> bit of space this way and unless we happen to get a balance or
> re-use/re-empty one of these bgs, they are leaked for good, which can
> lead to a spurious enospc later.
> 
> While I have observed such leaked blocked groups that are empty but not
> on the unused_bgs list on production systems, I have not observed that
> it is definitely due to this issue. I also reproduced this behavior by
> injecting an ENOSPC error from btrfs_start_trans_remove_block_group
> which can also fail with ENOMEM, so this feels like a legitimate
> injection point.

Do have happen to know which error caused this non-zero @ret?

I did a quick glance into the loop, it looks like it's not that easy to 
get a non-zero @ret:

- inc_block_group_ro() failure
   @ret is reset to 0, so not this path.

- btrfs_zone_finish()
   I guess meta is not deploying zoned btrfs in production.

- btrfs_star_trans_remove_block_group()
   This can return -ENOSPC, especially considering we have just marked
   one bg read-only, thus even stealing from global rsv, we may still
   fail with ENOSPC here.

   Although I'd say, that means the inc_block_group_ro() checks are not
   doing the correct reserved space checking, and that may be the real
   problem.

- btrfs_remove_chunk()
   If it failed, the trans is already aborted.

> 
> To fix it, instead of checking ret in the loop, just break out of the
> loop when ret != 0. Also, link the bg to the retry list at the
> individual failure sites so that the failing bg is not leaked.
> 
> Assisted-by: LLM (reproducer/error injection)
> Signed-off-by: Boris Burkov <boris@bur.io>

Otherwise the handling looks correct to me, doing the proper handling on 
error, other than delaying it to the next iteration.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   fs/btrfs/block-group.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index ee182369254c..2eb09c9901c9 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -1612,7 +1612,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
>   
>   		space_info = block_group->space_info;
>   
> -		if (ret || btrfs_mixed_space_info(space_info)) {
> +		if (btrfs_mixed_space_info(space_info)) {
>   			btrfs_put_block_group(block_group);
>   			continue;
>   		}
> @@ -1727,6 +1727,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
>   		ret = inc_block_group_ro(block_group, false);
>   		up_write(&space_info->groups_sem);
>   		if (ret < 0) {
> +			btrfs_link_bg_list(block_group, &retry_list);
>   			ret = 0;
>   			goto next;
>   		}
> @@ -1749,6 +1750,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
>   						     block_group->start);
>   		if (IS_ERR(trans)) {
>   			btrfs_dec_block_group_ro(block_group);
> +			btrfs_link_bg_list(block_group, &retry_list);
>   			ret = PTR_ERR(trans);
>   			goto next;
>   		}
> @@ -1759,6 +1761,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
>   		 */
>   		if (!clean_pinned_extents(trans, block_group)) {
>   			btrfs_dec_block_group_ro(block_group);
> +			btrfs_link_bg_list(block_group, &retry_list);
>   			goto end_trans;
>   		}
>   
> @@ -1845,6 +1848,8 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
>   next:
>   		btrfs_put_block_group(block_group);
>   		spin_lock(&fs_info->unused_bgs_lock);
> +		if (ret)
> +			break;
>   	}
>   	list_splice_tail(&retry_list, &fs_info->unused_bgs);
>   	spin_unlock(&fs_info->unused_bgs_lock);