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@xxxxxxxxxx>
> > ---
>
> 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?
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.
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.
>
> Brian
>
> > if (error)
> > return error;
> > } else if (ap->tp->t_flags & XFS_TRANS_LOWMODE) {
> > --
> > 2.20.1
> >
--
Carlos
