Re: [PATCH-RFC] btrfs: for unclustered allocation don't consider ffe_ctl->empty_cluster

[Alex Lyakas <alex.lyakas@zadara.com>]

Hi Boris,

On Thu, Feb 26, 2026 at 8:28 PM Boris Burkov <boris@bur.io> wrote:
>
> On Thu, Feb 26, 2026 at 09:37:46AM -0800, Boris Burkov wrote:
> > On Thu, Feb 26, 2026 at 01:34:19PM +0200, Alex Lyakas wrote:
> > > I encountered an issue when performing unclustered allocation for metadata:
> > >
> > > free_space_ctl->free_space = 2MB
> > > ffe_ctl->num_bytes = 4096
> > > ffe_ctl->empty_cluster = 2MB
> > > ffe_ctl->empty_size = 0
> > >
> > > So early check for free_space_ctl->free_space skips the block group, even though
> > > it has enough space to do the allocation.
> > > I think when doing unclustered allocation we should not look at ffe_ctl->empty_cluster.
> >
> > I see what you are saying, and a the level of this situation and this
> > line of code, I think you are right.
> >
> > But as-is, this change does not contain enough reasoning about the
> > semantics of the allocation algorithm to realistically be anything
> > but exchanging one bug for two new ones.
> >

Thank you for your review and your comments below.

However, I am not sure what is it that you are actually requesting.
Do you expect me to describe the whole mechanics of find_free_extent()
and how its state is tracked through ffe_ctl?

> > What is empty_cluster modelling? Why is it included in this calculation?
> > Why should it not be included? Where else is empty_cluster used and
> > should it change under this new interpretation? Does any of this change
> > if there is actually a cluster active for clustered allocations?
I am not changing the meaning of any field of ffe_ctl, which tracks
the state of every call to find_free_extent().

I found an issue where "empty_cluster" is used erroneously in my opinion.

According to the code, "empty_cluster" describes the minimal amount of
free space in the block group, which we are considering to allocate a
cluster from. I am not changing the meaning of it.

My fix is suggesting that "empty_cluster" should not be used when
looking at free space in a block group during unclustered allocation.
Nothing else is changed. In the issue that I saw, this bug prevented
us from allocating a metadata block from a block group, when the block
group still had enough space to make a 4Kb allocation.

I tagged the patch as RFC, because I am unable to test it on the
latest mainline kernel, and I tested it on a stable LTS kernel 6.6.

Thanks,
Alex.





> >
> > etc. etc.
> >
> > Thanks,
> > Boris
> >
>
> I missed the RFC tag in the patch, so I would like to apologize for my
> negativity. In my experience with other bugs, the interplay between the
> clustered algorithm and the unclustered algorithm is under-specified so
> I think it is likely you have indeed found a bug.
>
> If you want to fix it, I would proceed along the lines I complained
> about in my first response and try to define the relationships between
> the variables in a consistent way that explains why we shouldn't count
> that variable here.
>
> If you do go through with sharpening the definition of empty_cluster,
> I would be happy to review that work and help get it in.
>



> Thanks,
> Boris
>
> > >
> > > I tested this on stable kernel 6.6.
> > >
> > > Signed-off-by: Alex Lyakas <alex.lyakas@zadara.com>
> > > ---
> > >  fs/btrfs/extent-tree.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > > index 03cf9f242c70..84b340a67882 100644
> > > --- a/fs/btrfs/extent-tree.c
> > > +++ b/fs/btrfs/extent-tree.c
> > > @@ -3885,7 +3885,7 @@ static int find_free_extent_unclustered(struct btrfs_block_group *bg,
> > >             free_space_ctl = bg->free_space_ctl;
> > >             spin_lock(&free_space_ctl->tree_lock);
> > >             if (free_space_ctl->free_space <
> > > -               ffe_ctl->num_bytes + ffe_ctl->empty_cluster +
> > > +               ffe_ctl->num_bytes +
> > >                 ffe_ctl->empty_size) {
> > >                     ffe_ctl->total_free_space = max_t(u64,
> > >                                     ffe_ctl->total_free_space,
> > > --
> > > 2.43.0
> > >