| Message ID | 20190918082453.25266-3-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | A small improvement in the allocation algorithm | expand |
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? 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. (I think the whole total thing is still kind of a confusing mess in this regard, but fixing that is a separate problem.) Brian > if (error) > return error; > } else if (ap->tp->t_flags & XFS_TRANS_LOWMODE) { > -- > 2.20.1 >
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? 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 > >
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
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@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. 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
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. 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.... Cheers, Dave.
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
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; + if (error) return error; } else if (ap->tp->t_flags & XFS_TRANS_LOWMODE) {
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(+)