On Mon, Sep 23, 2019 at 09:11:36AM -0400, Brian Foster wrote:
> 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@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?
> >
>
> 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.
I'm not predicting, {bma,ap}.total is basically:
data size requested + all metadata space it may need, so the original request
can try to allocate everything as close as possible. And, if not possible, we
give up on it by reducing .total.
One of the issues here, is that we reduce .total maybe 'too late', when we
consider it already LOW SPACE mode, that's why Dave's fix does not 'fix' the
original problem without zeroing .total.
My patch was just an attempt to use a different approach to round down .total to
a reasonable size, without messing with the callers of xfs_bmap_alloc().
Anyway, again, I'm not saying my patch is the right approach. I was just trying
a different one.
>
> Brian
>
> >
> > >
> > > Brian
> > >
> > > > if (error)
> > > > return error;
> > > > } else if (ap->tp->t_flags & XFS_TRANS_LOWMODE) {
> > > > --
> > > > 2.20.1
> > > >
> >
> > --
> > Carlos
--
Carlos
