| Message ID | 20240813163638.3751939-3-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | forcealign for xfs | expand |
On Tue, Aug 13, 2024 at 04:36:26PM +0000, John Garry wrote: > From: Dave Chinner <dchinner@redhat.com> > > When we do a large allocation, the core free space allocation code > assumes that args->maxlen is aligned to args->prod/args->mod. hence > if we get a maximum sized extent allocated, it does not do tail > alignment of the extent. > > However, this assumes that nothing modifies args->maxlen between the > original allocation context setup and trimming the selected free > space extent to size. This assumption has recently been found to be > invalid - xfs_alloc_space_available() modifies args->maxlen in low > space situations - and there may be more situations we haven't yet > found like this. > > Force aligned allocation introduces the requirement that extents are > correctly tail aligned, resulting in this occasional latent > alignment failure to be reclassified from an unimportant curiousity > to a must-fix bug. > > Removing the assumption about args->maxlen allocations always being > tail aligned is trivial, and should not impact anything because > args->maxlen for inodes with extent size hints configured are > already aligned. Hence all this change does it avoid weird corner > cases that would have resulted in unaligned extent sizes by always > trimming the extent down to an aligned size. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > Reviewed-by: "Darrick J. Wong" <djwong@kernel.org> [provisional on v1 series comment] Still provisional -- neither the original patch author nor the submitter have answered my question from June: IOWs, we always trim rlen, unless there is no alignment (prod==1) or rlen is less than mod. For a forcealign file, it should never be the case that minlen < mod because we'll have returned ENOSPC, right? --D > Signed-off-by: John Garry <john.g.garry@oracle.com> > --- > fs/xfs/libxfs/xfs_alloc.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c > index d559d992c6ef..bf08b9e9d9ac 100644 > --- a/fs/xfs/libxfs/xfs_alloc.c > +++ b/fs/xfs/libxfs/xfs_alloc.c > @@ -433,20 +433,18 @@ xfs_alloc_compute_diff( > * Fix up the length, based on mod and prod. > * len should be k * prod + mod for some k. > * If len is too small it is returned unchanged. > - * If len hits maxlen it is left alone. > */ > -STATIC void > +static void > xfs_alloc_fix_len( > - xfs_alloc_arg_t *args) /* allocation argument structure */ > + struct xfs_alloc_arg *args) > { > - xfs_extlen_t k; > - xfs_extlen_t rlen; > + xfs_extlen_t k; > + xfs_extlen_t rlen = args->len; > > ASSERT(args->mod < args->prod); > - rlen = args->len; > ASSERT(rlen >= args->minlen); > ASSERT(rlen <= args->maxlen); > - if (args->prod <= 1 || rlen < args->mod || rlen == args->maxlen || > + if (args->prod <= 1 || rlen < args->mod || > (args->mod == 0 && rlen < args->prod)) > return; > k = rlen % args->prod; > -- > 2.31.1 > >
On 23/08/2024 17:31, Darrick J. Wong wrote: sorry for the slow reply... > On Tue, Aug 13, 2024 at 04:36:26PM +0000, John Garry wrote: >> From: Dave Chinner <dchinner@redhat.com> >>>> When we do a large allocation, the core free space allocation code >> assumes that args->maxlen is aligned to args->prod/args->mod. hence >> if we get a maximum sized extent allocated, it does not do tail >> alignment of the extent. >> >> However, this assumes that nothing modifies args->maxlen between the >> original allocation context setup and trimming the selected free >> space extent to size. This assumption has recently been found to be >> invalid - xfs_alloc_space_available() modifies args->maxlen in low >> space situations - and there may be more situations we haven't yet >> found like this. >> >> Force aligned allocation introduces the requirement that extents are >> correctly tail aligned, resulting in this occasional latent >> alignment failure to be reclassified from an unimportant curiousity >> to a must-fix bug. >> >> Removing the assumption about args->maxlen allocations always being >> tail aligned is trivial, and should not impact anything because >> args->maxlen for inodes with extent size hints configured are >> already aligned. Hence all this change does it avoid weird corner >> cases that would have resulted in unaligned extent sizes by always >> trimming the extent down to an aligned size. >> >> Signed-off-by: Dave Chinner <dchinner@redhat.com> >> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org> [provisional on v1 series comment] > > Still provisional -- neither the original patch author nor the submitter > have answered my question from June: > > IOWs, we always trim rlen, unless there is no alignment (prod==1) or > rlen is less than mod. For a forcealign file, it should never be the > case that minlen < mod because we'll have returned ENOSPC, right? For forcealign, mod == 0, so naturally that (minlen < mod) would not happen. We want to alloc a multiple of align only, which is in prod. If we consider minlen < prod, then that should not happen either as we would have returned ENOSPC. In xfs_bmap_select_minlen() we rounddown blen by args->alignment, and if that is less than the ap->minlen (1), i.e. if after rounddown we have 0, then we return ENOSPC for forcealign. So then minlen would not be less than prod after selecting minlen in xfs_bmap_select_minlen(). I hope that I am answering the question asked... Thanks, John > > --D > >> Signed-off-by: John Garry <john.g.garry@oracle.com> >> --- >> fs/xfs/libxfs/xfs_alloc.c | 12 +++++------- >> 1 file changed, 5 insertions(+), 7 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c >> index d559d992c6ef..bf08b9e9d9ac 100644 >> --- a/fs/xfs/libxfs/xfs_alloc.c >> +++ b/fs/xfs/libxfs/xfs_alloc.c >> @@ -433,20 +433,18 @@ xfs_alloc_compute_diff( >> * Fix up the length, based on mod and prod. >> * len should be k * prod + mod for some k. >> * If len is too small it is returned unchanged. >> - * If len hits maxlen it is left alone. >> */ >> -STATIC void >> +static void >> xfs_alloc_fix_len( >> - xfs_alloc_arg_t *args) /* allocation argument structure */ >> + struct xfs_alloc_arg *args) >> { >> - xfs_extlen_t k; >> - xfs_extlen_t rlen; >> + xfs_extlen_t k; >> + xfs_extlen_t rlen = args->len; >> >> ASSERT(args->mod < args->prod); >> - rlen = args->len; >> ASSERT(rlen >= args->minlen); >> ASSERT(rlen <= args->maxlen); >> - if (args->prod <= 1 || rlen < args->mod || rlen == args->maxlen || >> + if (args->prod <= 1 || rlen < args->mod || >> (args->mod == 0 && rlen < args->prod)) >> return; >> k = rlen % args->prod; >> -- >> 2.31.1 >> >>
On Thu, Aug 29, 2024 at 06:58:02PM +0100, John Garry wrote: > On 23/08/2024 17:31, Darrick J. Wong wrote: > > sorry for the slow reply... > > > On Tue, Aug 13, 2024 at 04:36:26PM +0000, John Garry wrote: > > > From: Dave Chinner <dchinner@redhat.com> > > > > > When we do a large allocation, the core free space allocation code > > > assumes that args->maxlen is aligned to args->prod/args->mod. hence > > > if we get a maximum sized extent allocated, it does not do tail > > > alignment of the extent. > > > > > > However, this assumes that nothing modifies args->maxlen between the > > > original allocation context setup and trimming the selected free > > > space extent to size. This assumption has recently been found to be > > > invalid - xfs_alloc_space_available() modifies args->maxlen in low > > > space situations - and there may be more situations we haven't yet > > > found like this. > > > > > > Force aligned allocation introduces the requirement that extents are > > > correctly tail aligned, resulting in this occasional latent > > > alignment failure to be reclassified from an unimportant curiousity > > > to a must-fix bug. > > > > > > Removing the assumption about args->maxlen allocations always being > > > tail aligned is trivial, and should not impact anything because > > > args->maxlen for inodes with extent size hints configured are > > > already aligned. Hence all this change does it avoid weird corner > > > cases that would have resulted in unaligned extent sizes by always > > > trimming the extent down to an aligned size. > > > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > > Reviewed-by: "Darrick J. Wong" <djwong@kernel.org> [provisional on v1 series comment] > > > > Still provisional -- neither the original patch author nor the submitter > > have answered my question from June: > > > > IOWs, we always trim rlen, unless there is no alignment (prod==1) or > > rlen is less than mod. For a forcealign file, it should never be the > > case that minlen < mod because we'll have returned ENOSPC, right? > > For forcealign, mod == 0, so naturally that (minlen < mod) would not happen. > We want to alloc a multiple of align only, which is in prod. > > If we consider minlen < prod, then that should not happen either as we would > have returned ENOSPC. In xfs_bmap_select_minlen() we rounddown blen by > args->alignment, and if that is less than the ap->minlen (1), i.e. if after > rounddown we have 0, then we return ENOSPC for forcealign. So then minlen > would not be less than prod after selecting minlen in > xfs_bmap_select_minlen(). > > I hope that I am answering the question asked... Yep, that satisfies my curiosity! Thanks for getting back to me, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > > Thanks, > John > > > > > --D > > > > > Signed-off-by: John Garry <john.g.garry@oracle.com> > > > --- > > > fs/xfs/libxfs/xfs_alloc.c | 12 +++++------- > > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > > > > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c > > > index d559d992c6ef..bf08b9e9d9ac 100644 > > > --- a/fs/xfs/libxfs/xfs_alloc.c > > > +++ b/fs/xfs/libxfs/xfs_alloc.c > > > @@ -433,20 +433,18 @@ xfs_alloc_compute_diff( > > > * Fix up the length, based on mod and prod. > > > * len should be k * prod + mod for some k. > > > * If len is too small it is returned unchanged. > > > - * If len hits maxlen it is left alone. > > > */ > > > -STATIC void > > > +static void > > > xfs_alloc_fix_len( > > > - xfs_alloc_arg_t *args) /* allocation argument structure */ > > > + struct xfs_alloc_arg *args) > > > { > > > - xfs_extlen_t k; > > > - xfs_extlen_t rlen; > > > + xfs_extlen_t k; > > > + xfs_extlen_t rlen = args->len; > > > ASSERT(args->mod < args->prod); > > > - rlen = args->len; > > > ASSERT(rlen >= args->minlen); > > > ASSERT(rlen <= args->maxlen); > > > - if (args->prod <= 1 || rlen < args->mod || rlen == args->maxlen || > > > + if (args->prod <= 1 || rlen < args->mod || > > > (args->mod == 0 && rlen < args->prod)) > > > return; > > > k = rlen % args->prod; > > > -- > > > 2.31.1 > > > > > > > >
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c index d559d992c6ef..bf08b9e9d9ac 100644 --- a/fs/xfs/libxfs/xfs_alloc.c +++ b/fs/xfs/libxfs/xfs_alloc.c @@ -433,20 +433,18 @@ xfs_alloc_compute_diff( * Fix up the length, based on mod and prod. * len should be k * prod + mod for some k. * If len is too small it is returned unchanged. - * If len hits maxlen it is left alone. */ -STATIC void +static void xfs_alloc_fix_len( - xfs_alloc_arg_t *args) /* allocation argument structure */ + struct xfs_alloc_arg *args) { - xfs_extlen_t k; - xfs_extlen_t rlen; + xfs_extlen_t k; + xfs_extlen_t rlen = args->len; ASSERT(args->mod < args->prod); - rlen = args->len; ASSERT(rlen >= args->minlen); ASSERT(rlen <= args->maxlen); - if (args->prod <= 1 || rlen < args->mod || rlen == args->maxlen || + if (args->prod <= 1 || rlen < args->mod || (args->mod == 0 && rlen < args->prod)) return; k = rlen % args->prod;