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]
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.1On 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
> > >
> > >
>
>
© 2016 - 2024 Red Hat, Inc.