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

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

[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.

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>
---
 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);
-- 
2.55.0

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

[Qu Wenruo]

在 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);

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

[Boris Burkov]

On Thu, Sep 17, 2026 at 08:21:37AM +0930, Qu Wenruo wrote:
> 
> 
> 在 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 do not have a trace or any other error log in dmesg to conclude where
it came from. I just have boxes with a lot of empty bgs not linked on
the unused list and inferred this mechanism.

> 
> 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.

I suspect ENOSPC or ENOMEM from this one, personally. Agreed with the
rest of your points on the other possible error sites.

> 
>   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);
>