Re: [PATCH 1/2] btrfs: allocate additional SYSTEM space earlier

[Boris Burkov <boris@bur.io>]

On Mon, Sep 28, 2026 at 11:58:00AM +0100, Filipe Manana wrote:
> On Tue, Sep 22, 2026 at 5:57 PM Boris Burkov <boris@bur.io> wrote:
> >
> > Currently, reserve_chunk_space() allocates a new system chunk when the
> > space left is smaller than the reservation for a single chunk tree
> > update.
> >
> > Therefore, a filesystem in single metadata mode on a very large block
> > device can accumulate tens of thousands (TB) of chunks while never
> > allocating more than the original 4MiB SYSTEM block group created by
> > mkfs. If it then fills up (with large fallocates for example) and then
> > that data is freed, en-masse, we end up trying to delete thousands of
> > block groups in a single transaction in btrfs_delete_unused_bgs().
> >
> > This process touches most of the nodes and leaves of the chunk tree,
> > essentially trying to allocate roughly double the current usage of the
> > chunk tree for cow. When this runs into needing a fresh system chunk,
> > the fs is full of the empty bgs and we abort with ENOSPC in
> > btrfs_remove_chunk() like:
> >
> >   BTRFS error (device loop0 state A): Transaction 30733 aborted (-ENOSPC)
> >   space_info SYSTEM (sub-group id 0) has 0 free, is not full
> >   space_info total=4194304, used=3276800, pinned=0, reserved=917504, may_use=0, readonly=0 zone_unusable=0
> >   BTRFS: error (device loop0 state A) in btrfs_remove_chunk:3643: errno=-28 No space left
> >
> > This was seen on a 30TiB production filesystem with 10TiB of unused
> > block groups and reproduces on a 30TiB sparse loop device: mkfs with
> > -m single, fallocate 1GiB files until ENOSPC, delete two of every three
> > adjacent files, sync.
> >
> > To fix this, we should allocate the (relatively tiny) system chunks a
> > little bit more eagerly. If we do it when it is half full, we ensure we
> > can delete all of the block groups in one go. To fill up half of a 4MiB
> > system bg requires thousands of block_groups so this should only affect
> > very large fileystems.
> >
> > Assisted-by: LLM
> > Signed-off-by: Boris Burkov <boris@bur.io>
> > ---
> >  fs/btrfs/block-group.c | 26 ++++++++++++++++++++++++--
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> > index 2eb09c9901c9..cf26dbeaabdb 100644
> > --- a/fs/btrfs/block-group.c
> > +++ b/fs/btrfs/block-group.c
> > @@ -1574,6 +1574,19 @@ static bool btrfs_link_bg_list(struct btrfs_block_group *bg, struct list_head *l
> >         return added;
> >  }
> >
> > +/*
> > + * Compute an upper bound on the bytes needed to modify every leaf in the chunk
> > + * tree. Normally, we could just allocate a new system chunk then, but that can
> > + * fail if there is no free space for a new dev extent. This can happen when
> > + * we fill up a large filesystem then rapidly delete a large portion of it.
> > + */
> > +static u64 system_space_needed(struct btrfs_space_info *sinfo)
> > +{
> > +       lockdep_assert_held(&sinfo->lock);
> > +
> > +       return btrfs_space_info_used(sinfo, true) + sinfo->bytes_used;
> > +}
> > +
> >  /*
> >   * Process the unused_bgs list and remove any that don't have any allocated
> >   * space inside of them.
> > @@ -1661,6 +1674,9 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
> >                 if (btrfs_is_block_group_used(block_group) ||
> >                     (block_group->ro && !(block_group->flags & BTRFS_BLOCK_GROUP_REMAPPED)) ||
> >                     list_is_singular(&block_group->list) ||
> > +                   ((block_group->flags & BTRFS_BLOCK_GROUP_SYSTEM) &&
> > +                    space_info->total_bytes - block_group->length <
> > +                    system_space_needed(space_info)) ||
> >                     test_bit(BLOCK_GROUP_FLAG_FULLY_REMAPPED, &block_group->runtime_flags)) {
> >                         /*
> >                          * We want to bail if we made new allocations or have
> > @@ -1674,6 +1690,10 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
> >                          * next block group of this type would be created with a
> >                          * "single" profile (even if we're in a raid fs) because
> >                          * fs_info->avail_*_alloc_bits would be 0.
> > +                        *
> > +                        * Also bail out if this is a system block group that
> > +                        * system_space_needed() relies on to ensure head room for
> > +                        * mass deletion.
> >                          */
> >                         trace_btrfs_skip_unused_block_group(block_group);
> >                         spin_unlock(&block_group->lock);
> > @@ -4509,6 +4529,7 @@ static void reserve_chunk_space(struct btrfs_trans_handle *trans,
> >         struct btrfs_fs_info *fs_info = trans->fs_info;
> >         struct btrfs_space_info *info;
> >         u64 left;
> > +       bool low;
> >         int ret = 0;
> >
> >         /*
> > @@ -4520,6 +4541,7 @@ static void reserve_chunk_space(struct btrfs_trans_handle *trans,
> >         info = btrfs_find_space_info(fs_info, BTRFS_BLOCK_GROUP_SYSTEM);
> >         spin_lock(&info->lock);
> >         left = info->total_bytes - btrfs_space_info_used(info, true);
> > +       low = left < bytes || info->total_bytes < system_space_needed(info);
> >         spin_unlock(&info->lock);
> >
> >         if (left < bytes && btrfs_test_opt(fs_info, ENOSPC_DEBUG)) {
> > @@ -4528,7 +4550,7 @@ static void reserve_chunk_space(struct btrfs_trans_handle *trans,
> >                 btrfs_dump_space_info(info, 0, false);
> >         }
> >
> > -       if (left < bytes) {
> > +       if (low) {
> >                 u64 flags = btrfs_system_alloc_profile(fs_info);
> >                 struct btrfs_block_group *bg;
> >                 struct btrfs_space_info *space_info;
> > @@ -4572,7 +4594,7 @@ static void reserve_chunk_space(struct btrfs_trans_handle *trans,
> >                 }
> >         }
> >
> > -       if (!ret) {
> > +       if (!ret || left >= bytes) {
> 
> So this hunk I have trouble understanding it, because if left >= bytes
> should always be true at this point, as if if left < bytes we have
> allocated a system chunk (if we haven't then ret has an error value).

It is a bit confusing and I am not sure how to make it as nice as
possible. So the condition is:

  low = left < bytes || info->total_bytes < system_space_needed(info)
  ...
  if (low) {
  ...
  }

So ret != 0 implies low is true, but not necessarily left < bytes.
Basically the idea is that if the chunk allocation designed to protect
the chunk space in the future fails but we still have enough for right
now (ret && left >= bytes) then we can still add bytes to the block_rsv.

Would it help if I tried to give separate names to the two conditions
and did something like:

needs_space = left < bytes
wants_space = total < system_space_target()
low = needs_space || wants_space

...

if (!ret || !needs_space) {
 ...
}

So rename system_space_needed() to system_space_target() or something
like that, and then differentiate the minimum to add to the block_rsv
more clearly from the desire to add a system chunk? The current
system_space_needed() is kinda misleaded cause with writeback re-cowing,
we might use more than that in a txn anyway so it is not a hard
guarantee.

Thanks for the review,
Boris

> 
> Otherwise it looks good (as well as the second patch).
> 
> Thanks.
> 
> >                 ret = btrfs_block_rsv_add(fs_info,
> >                                           &fs_info->chunk_block_rsv,
> >                                           bytes, BTRFS_RESERVE_NO_FLUSH);
> > --
> > 2.55.0
> >
> >