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