Re: [PATCH 2/2] xfs: Limit total allocation request to maximum possible

[Brian Foster <bfoster@redhat.com>]

On Wed, Sep 25, 2019 at 06:50:29AM +1000, Dave Chinner wrote:
> On Wed, Sep 18, 2019 at 10:24:53AM +0200, Carlos Maiolino wrote:
> > The original allocation request may have a total value way beyond
> > possible limits.
> > 
> > Trim it down to the maximum possible if needed
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_bmap.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 07aad70f3931..3aa0bf5cc7e3 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -3477,6 +3477,11 @@ xfs_bmap_btalloc(
> >  			error = xfs_bmap_btalloc_filestreams(ap, &args, &blen);
> >  		else
> >  			error = xfs_bmap_btalloc_nullfb(ap, &args, &blen);
> > +
> > +		/* We can never have total larger than blen, so trim it now */
> 
> Yes we can. blen is typically the largest contiguous extent in the
> filesystem or AG in question. It is not typically the total free
> space in the AG, which only occurs when the AG is empty. i.e. in
> normal situations, we can allocate both blen and the rest of the
> metadata from the same AG as there is more than one free extent in
> the AG.
> 

Right..

> I think that for the purposes of a single > AG size allocation, the
> total needs to be clamped to the free space in the AG that is
> selected, not the length of the allocation we are trying....
> 

As already noted, I do think args.total could use some work. But unless
I'm missing something about the set of callers modified in the original
patch, I'd rather not tweak a core bmap mechanism in service to callers
that have no reason to use said mechanism in the first place.

And I know that such a change would affect legitimate args.total users
too and so still might be appropriate, I just think that's something for
a separate patch (even if in the same series)...

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com