Re: [PATCH v2 3/3] xfs: incorporate increased AGFL min requirement for minleft allocs

On Tue, Aug 18, 2026 at 08:55:19AM +1000, Dave Chinner wrote:
> On Fri, Aug 14, 2026 at 09:22:39AM -0400, Brian Foster wrote:
...
> > 
> > Reported-by: Matt Fleming <[email protected]>
> > Assisted-by: LLM
> > Signed-off-by: Brian Foster <[email protected]>
> > ---
> >  fs/xfs/libxfs/xfs_alloc.c | 29 ++++++++++++++++++++++++++++-
> >  fs/xfs/libxfs/xfs_alloc.h |  2 ++
> >  fs/xfs/libxfs/xfs_bmap.c  |  2 +-
> >  3 files changed, 31 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index dbb85fb6314b..74c5b587c87b 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
...
> > @@ -2533,10 +2548,22 @@ xfs_alloc_space_available(
> >  	 * Do we have enough free space remaining for the allocation? Don't
> >  	 * account extra agfl blocks because we are about to defer free them,
> >  	 * making them unavailable until the current transaction commits.
> > +	 *
> > +	 * If minleft is set, this allocation might cause an allocbt split that
> > +	 * increases the AGFL minimum for the next allocation in the
> > +	 * transaction. Reserve that space from the available block count
> > +	 * (without prematurely growing the AGFL) to prevent the subsequent
> > +	 * allocation from failing due to an increased min_free requirement.
> >  	 */
> > +	minleft = args->minleft;
> > +	if (minleft) {
> > +		minleft += xfs_alloc_min_freelist_minleft(args->mp, pag) -
> > +					min_free;
> > +	}
> 
> This seems fragile to me. It is based on the assumption that
> min_free is calculated from xfs_alloc_min_freelist() by the caller,
> and then this calculates the difference between what the caller should
> have calculated and what is actually needed.
> 
> Where this mod is placed also results in the longest available
> extent check not taking into account this modified min_free
> requirement, whereas the check in xfs_bmap_longest_free_extent() is
> modified to take this modified minleft value into account. i.e. the
> checks w.r.t. minleft and longest extents are no longer consistent
> across the layers.
> 
> I'm also concerned that this results in the
> xfs_alloc_space_available() caller using different values of "need"
> and "minleft" to what the actual space availablity calculation is
> using; that feels like a future landmine to me.
> 

I agree in principle wrt the fragility. This is kind of what I meant by
expressing preference for something more elegant in the other subthread
conversation w/ Darrick; I just haven't quite figured out what that
looks like yet.

> i.e. the xfs_alloc_space_available() caller already knows is minleft
> is set, so if it were to use xfs_alloc_min_freelist_minleft(), then
> there would not need to be this "correction" in this code and all
> the values would be consistent.
> 
> Unless I'm missing something subtle, I think that the callers should
> not need to know it should call xfs_alloc_min_freelist_minleft() or
> xfs_alloc_min_freelist() as it feels like exposing internal AGFL
> space/btree accounting requirements into an external API. All the
> caller needs to signal is whether this is the first of a chain of
> allocations or not (i.e. args->minleft != 0), and the internal alloc
> code should handle it from there.
> 

I also agree wrt to the layering confusion, though I find that the
existing API also suffers a bit from this.

> Hence I suspect it would be much cleaner just to add a 'bool
> multialloc' parameter to xfs_alloc_min_freelist() and have all
> callers set it appropriately. That would avoid the need for the
> wrapper functions and keep this AGFL reservation wart
> completely internal to the AGFL reservation calculation....
> 
> Thoughts?
> 

If I follow correctly, this centralizes the extra level calculation to
_min_freelist(). I agree the code is cleaner, but the reason I didn't do
that is because this would artificially alloc and free blocks through
the AGFL for every such multi-allocation.

I.e., a multi-alloc comes in and we calculate the multi-level worst case
bump for each allocbt, set that to min_free, bump the AGFL to the new
min_free and carry on with the alloc. The common case is going to be no
new splits (and certainly not "everything splits"), so either the next
allocation in the multi-alloc (or the next alloc thereafter) will remove
every block that was added for splits that never occurred. So IMO the
tradeoff for cleaner code here is worse behavior.

Hmmm.. I suppose we could still implement this same sort of behavior
using a multi-alloc parameter. For example, suppose we replaced
xfs_alloc_min_freelist() with an xfs_alloc_freelist() that took your
multi-alloc param and returned a min and a max. The min is the current
return value, the max is the prospective worst case (i.e. min == max if
multi-alloc == false, else +1 level).

The internal alloc path passes both min/max to _space_available() so it
can do consistent max length checks and calculate/add the delta to
minleft for the available calculation. The AGFL min_free value is still
based on the min. The bmap case passes multi-alloc = true and uses the
max for its own length checks. Hm?

Brian

> -Dave.
> -- 
> Dave Chinner
> [email protected]
>