| Message ID | 20240801163057.3981192-2-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | forcealign for xfs | expand |
On Thu, Aug 01, 2024 at 04:30:44PM +0000, John Garry wrote: > From: Dave Chinner <dchinner@redhat.com> > > When we are near ENOSPC and don't have enough free > space for an args->maxlen allocation, xfs_alloc_space_available() > will trim args->maxlen to equal the available space. However, this > function has only checked that there is enough contiguous free space > for an aligned args->minlen allocation to succeed. Hence there is no > guarantee that an args->maxlen allocation will succeed, nor that the > available space will allow for correct alignment of an args->maxlen > allocation. > > Further, by trimming args->maxlen arbitrarily, it breaks an > assumption made in xfs_alloc_fix_len() that if the caller wants > aligned allocation, then args->maxlen will be set to an aligned > value. It then skips the tail alignment and so we end up with > extents that aren't aligned to extent size hint boundaries as we > approach ENOSPC. > > To avoid this problem, don't reduce args->maxlen by some random, > arbitrary amount. If args->maxlen is too large for the available > space, reduce the allocation to a minlen allocation as we know we > have contiguous free space available for this to succeed and always > be correctly aligned. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > Signed-off-by: John Garry <john.g.garry@oracle.com> > --- > fs/xfs/libxfs/xfs_alloc.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c > index 59326f84f6a5..d559d992c6ef 100644 > --- a/fs/xfs/libxfs/xfs_alloc.c > +++ b/fs/xfs/libxfs/xfs_alloc.c > @@ -2524,14 +2524,23 @@ xfs_alloc_space_available( > if (available < (int)max(args->total, alloc_len)) > return false; > > + if (flags & XFS_ALLOC_FLAG_CHECK) > + return true; > + > /* > - * Clamp maxlen to the amount of free space available for the actual > - * extent allocation. > + * If we can't do a maxlen allocation, then we must reduce the size of > + * the allocation to match the available free space. We know how big > + * the largest contiguous free space we can allocate is, so that's our > + * upper bound. However, we don't exaclty know what alignment/size > + * constraints have been placed on the allocation, so we can't > + * arbitrarily select some new max size. Hence make this a minlen > + * allocation as we know that will definitely succeed and match the > + * callers alignment constraints. > */ > - if (available < (int)args->maxlen && !(flags & XFS_ALLOC_FLAG_CHECK)) { > - args->maxlen = available; > + alloc_len = args->maxlen + (args->alignment - 1) + args->minalignslop; > + if (longest < alloc_len) { > + args->maxlen = args->minlen; Same question as the June 21st posting: Is it possible to reduce maxlen the largest multiple of the alignment that is still less than @longest? --D > ASSERT(args->maxlen > 0); > - ASSERT(args->maxlen >= args->minlen); > } > > return true; > -- > 2.31.1 > >
On Tue, Aug 06, 2024 at 11:51:38AM -0700, Darrick J. Wong wrote: > On Thu, Aug 01, 2024 at 04:30:44PM +0000, John Garry wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > When we are near ENOSPC and don't have enough free > > space for an args->maxlen allocation, xfs_alloc_space_available() > > will trim args->maxlen to equal the available space. However, this > > function has only checked that there is enough contiguous free space > > for an aligned args->minlen allocation to succeed. Hence there is no > > guarantee that an args->maxlen allocation will succeed, nor that the > > available space will allow for correct alignment of an args->maxlen > > allocation. > > > > Further, by trimming args->maxlen arbitrarily, it breaks an > > assumption made in xfs_alloc_fix_len() that if the caller wants > > aligned allocation, then args->maxlen will be set to an aligned > > value. It then skips the tail alignment and so we end up with > > extents that aren't aligned to extent size hint boundaries as we > > approach ENOSPC. > > > > To avoid this problem, don't reduce args->maxlen by some random, > > arbitrary amount. If args->maxlen is too large for the available > > space, reduce the allocation to a minlen allocation as we know we > > have contiguous free space available for this to succeed and always > > be correctly aligned. > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > Signed-off-by: John Garry <john.g.garry@oracle.com> > > --- > > fs/xfs/libxfs/xfs_alloc.c | 19 ++++++++++++++----- > > 1 file changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c > > index 59326f84f6a5..d559d992c6ef 100644 > > --- a/fs/xfs/libxfs/xfs_alloc.c > > +++ b/fs/xfs/libxfs/xfs_alloc.c > > @@ -2524,14 +2524,23 @@ xfs_alloc_space_available( > > if (available < (int)max(args->total, alloc_len)) > > return false; > > > > + if (flags & XFS_ALLOC_FLAG_CHECK) > > + return true; > > + > > /* > > - * Clamp maxlen to the amount of free space available for the actual > > - * extent allocation. > > + * If we can't do a maxlen allocation, then we must reduce the size of > > + * the allocation to match the available free space. We know how big > > + * the largest contiguous free space we can allocate is, so that's our > > + * upper bound. However, we don't exaclty know what alignment/size > > + * constraints have been placed on the allocation, so we can't > > + * arbitrarily select some new max size. Hence make this a minlen > > + * allocation as we know that will definitely succeed and match the > > + * callers alignment constraints. > > */ > > - if (available < (int)args->maxlen && !(flags & XFS_ALLOC_FLAG_CHECK)) { > > - args->maxlen = available; > > + alloc_len = args->maxlen + (args->alignment - 1) + args->minalignslop; > > + if (longest < alloc_len) { > > + args->maxlen = args->minlen; > > Same question as the June 21st posting: > > Is it possible to reduce maxlen the largest multiple of the alignment > that is still less than @longest? Perhaps. The comment does say "we don't exaclty know what alignment/size constraints have been placed on the allocation, so we can't arbitrarily select some new max size." Given this unknown I simply punted the issue and went straight to selecting a size the caller has guaranteed will be valid for their constraints. -Dave.
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c index 59326f84f6a5..d559d992c6ef 100644 --- a/fs/xfs/libxfs/xfs_alloc.c +++ b/fs/xfs/libxfs/xfs_alloc.c @@ -2524,14 +2524,23 @@ xfs_alloc_space_available( if (available < (int)max(args->total, alloc_len)) return false; + if (flags & XFS_ALLOC_FLAG_CHECK) + return true; + /* - * Clamp maxlen to the amount of free space available for the actual - * extent allocation. + * If we can't do a maxlen allocation, then we must reduce the size of + * the allocation to match the available free space. We know how big + * the largest contiguous free space we can allocate is, so that's our + * upper bound. However, we don't exaclty know what alignment/size + * constraints have been placed on the allocation, so we can't + * arbitrarily select some new max size. Hence make this a minlen + * allocation as we know that will definitely succeed and match the + * callers alignment constraints. */ - if (available < (int)args->maxlen && !(flags & XFS_ALLOC_FLAG_CHECK)) { - args->maxlen = available; + alloc_len = args->maxlen + (args->alignment - 1) + args->minalignslop; + if (longest < alloc_len) { + args->maxlen = args->minlen; ASSERT(args->maxlen > 0); - ASSERT(args->maxlen >= args->minlen); } return true;