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

[Brian Foster <bfoster@redhat.com>]

On Mon, Sep 23, 2019 at 02:39:34PM +0200, Carlos Maiolino wrote:
> On Wed, Sep 18, 2019 at 08:28:59AM -0400, Brian Foster 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>
> > > ---
> > 
> > Confused.. what was wrong with the original bma.total patch that it
> > needs to be replaced?
> 
> At this point in time, what you mean by the 'original' patch? :) Yours? Or
> Dave's?
> 

The original patch I posted..

> If you meant yours, I was just trying to find out a way to fix it without
> modifying the callers, nothing else than that.
> 
> If you meant regarding Dave's proposal, as he tagged his proposal as a /* Hack
> */, I was just looking for ways to change total, instead of cropping it to 0.
> 
> And giving the fact args.total > blen seems unreasonable, giving it will
> certainly tail here, I just thought it might be a reasonable way to change
> args.total value.
> 

I think the code is flaky, but I'm not sure why that's unreasonable. The
intent of args.total is to be larger than the mapping length.

> By no means this patchset was meant to supersede yours or Dave's idea though, I
> was just looking for a different approach, if feasible.
> 
> 
> > I was assuming we'd replace the allocation retry
> > patch with the minlen alignment fixups and combine those with the
> > bma.total patch to fix the problem. Hm?
> > 
> > >  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 */
> > > +		if (args.total > blen)
> > > +			args.total = blen;
> > > +
> > 
> > I don't think this is safe. The reason the original patch only updated
> > certain callers is because those callers only used it for extra blocks
> > that are already incorported into bma.minleft by the bmap layer itself.
> > There are still other callers for which bma.total is specifically
> > intended to be larger than the map size.
> 
> Afaik, yes, but still, total is basically used to attempt an allocation of data
> + metadata on the same AG if possible, reducing args.total to match blen, the
> 'worst' case would be to have an allocation of data + metadata on different ags,
> which, if total is larger than blen, it will fall into that behavior anyway.
> 

Maybe..? There is no requirement that the additional blocks accounted by
args.total be contiguous with the allocation for the mapping, so I don't
see how you could reliably predict that.

Brian

> 
> > 
> > Brian
> > 
> > >  		if (error)
> > >  			return error;
> > >  	} else if (ap->tp->t_flags & XFS_TRANS_LOWMODE) {
> > > -- 
> > > 2.20.1
> > > 
> 
> -- 
> Carlos