| Message ID | 20240402233006.1210262-2-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: allocation alignment for forced alignment | expand |
On 03/04/2024 00:28, Dave Chinner 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> This change seems to cause or at least expose a problem for me - I say that because it is the only difference to what I already had from https://lore.kernel.org/linux-xfs/ZeeaKrmVEkcXYjbK@dread.disaster.area/T/#me7abe09fe85292ca880f169a4af651eac5ed1424 and the xfs_alloc_fix_len() fix. With forcealign extsize=64KB, when I write to the end of a file I get 2x new extents, both of which are not a multiple of 64KB in size. Note that I am including https://lore.kernel.org/linux-xfs/20240304130428.13026-7-john.g.garry@oracle.com/, but I don't think it makes a difference. Before: EXT: FILE-OFFSET BLOCK-RANGE AG AG-OFFSET TOTAL 0: [0..383]: 73216..73599 0 (73216..73599) 384 1: [384..511]: 70528..70655 0 (70528..70655) 128 After: EXT: FILE-OFFSET BLOCK-RANGE AG AG-OFFSET TOTAL 0: [0..383]: 73216..73599 0 (73216..73599) 384 1: [384..511]: 70528..70655 0 (70528..70655) 128 2: [512..751]: 30336..30575 0 (30336..30575) 240 3: [752..767]: 48256..48271 0 (48256..48271) 16 > --- > 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 9da52e92172a..215265e0f68f 100644 > --- a/fs/xfs/libxfs/xfs_alloc.c > +++ b/fs/xfs/libxfs/xfs_alloc.c > @@ -2411,14 +2411,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/siz > + * 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; I added some kernel logs to assist debugging, and if I am reading them correctly, for ext #2 allocation we had at this point: longest = 46, alloc_len = 47, args->minlen=30, maxlen=32, alignslop=0 available=392; longest < alloc_len, so we set args->maxlen = args->minlen (= 30) For ext3: longest = 32, alloc_len = 17, args->minlen=2, args->maxlen=2, alignslop=0, available=362; longest > alloc_len, so do nothing > + if (longest < alloc_len) { > + args->maxlen = args->minlen; > ASSERT(args->maxlen > 0); > - ASSERT(args->maxlen >= args->minlen); > } > > return true;
On Wed, Apr 03, 2024 at 05:31:49PM +0100, John Garry wrote: > On 03/04/2024 00:28, Dave Chinner 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> > > This change seems to cause or at least expose a problem for me - I say that > because it is the only difference to what I already had from https://lore.kernel.org/linux-xfs/ZeeaKrmVEkcXYjbK@dread.disaster.area/T/#me7abe09fe85292ca880f169a4af651eac5ed1424 > and the xfs_alloc_fix_len() fix. > > With forcealign extsize=64KB, when I write to the end of a file I get 2x new > extents, both of which are not a multiple of 64KB in size. Note that I am > including https://lore.kernel.org/linux-xfs/20240304130428.13026-7-john.g.garry@oracle.com/, > but I don't think it makes a difference. > > Before: > EXT: FILE-OFFSET BLOCK-RANGE AG AG-OFFSET TOTAL > 0: [0..383]: 73216..73599 0 (73216..73599) 384 > 1: [384..511]: 70528..70655 0 (70528..70655) 128 > > After: > EXT: FILE-OFFSET BLOCK-RANGE AG AG-OFFSET TOTAL > 0: [0..383]: 73216..73599 0 (73216..73599) 384 > 1: [384..511]: 70528..70655 0 (70528..70655) 128 > 2: [512..751]: 30336..30575 0 (30336..30575) 240 So that's a 120kB (30 fsb) extent. > 3: [752..767]: 48256..48271 0 (48256..48271) 16 And that's the 2FSB tail to make it up to 64kB. > > --- > > 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 9da52e92172a..215265e0f68f 100644 > > --- a/fs/xfs/libxfs/xfs_alloc.c > > +++ b/fs/xfs/libxfs/xfs_alloc.c > > @@ -2411,14 +2411,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/siz > + * 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; > > I added some kernel logs to assist debugging, and if I am reading them > correctly, for ext #2 allocation we had at this point: > > longest = 46, alloc_len = 47, args->minlen=30, maxlen=32, alignslop=0 > available=392; longest < alloc_len, so we set args->maxlen = args->minlen (= > 30) Why was args->minlen set to 30? Where did that value come from? i.e. That is not correctly sized/aligned for force alignment - it should be rounded down to 16 fsbs, right? I'm guessing that "30" is (longest - alignment) = 46 - 16 = 30? And then it wasn't rounded down from there? > For ext3: > longest = 32, alloc_len = 17, args->minlen=2, args->maxlen=2, alignslop=0, > available=362; longest > alloc_len, so do nothing Same issue here - for a forced align allocation, minlen needs to be bound to alignment constraints. If minlen is set like this, then the allocation will always return a 2 block extent if they exist. IOWs, the allocation constraints are not correctly aligned, but the allocation is doing exactly what the constraints say it is allowed to do. Therefore the issue is in the constraint setup (args->minlen) for forced alignment and not the allocation code that selects a an args->minlen extent that is not correctly sized near ENOSPC. I'm guessing that we need something like the (untested) patch below to ensure args->minlen is properly aligned.... -Dave.
>>> - if (available < (int)args->maxlen && !(flags & XFS_ALLOC_FLAG_CHECK)) { >>> - args->maxlen = available; >>> + alloc_len = args->maxlen + (args->alignment - 1) + args->minalignslop; >> >> I added some kernel logs to assist debugging, and if I am reading them >> correctly, for ext #2 allocation we had at this point: >> >> longest = 46, alloc_len = 47, args->minlen=30, maxlen=32, alignslop=0 >> available=392; longest < alloc_len, so we set args->maxlen = args->minlen (= >> 30) > > Why was args->minlen set to 30? Where did that value come from? i.e. > That is not correctly sized/aligned for force alignment - it should > be rounded down to 16 fsbs, right? I could not recreate this exact scenario, but another similar one occurred which gave: /root/mnt2/file_2425: EXT: FILE-OFFSET BLOCK-RANGE AG AG-OFFSET TOTAL 0: [0..431]: 489600..490031 2 (131200..131631) 432 1: [432..511]: 529408..529487 2 (171008..171087) 80 # For allocating EXT #0, in xfs_bmap_select_minlen() we had initially args->minlen=0, maxlen=64 blen=70 then blen -= alignment (=16), an finally condition blen < args->maxlen passed and have minlen = 54 Then in xfs_alloc_space_available(), for check (longest < alloc_len) near the end, at that point we have: longest=70, alloc_len=79, args->minlen=54, maxlen=64, alignslop=0 available=155, and so we set maxlen = minlen = 54 Then in xfs_alloc_fix_len(), initially args->mod=0, prod=16, minlen=54, maxlen=54, len=54 rlen=54 And we end up with the 54 FSB extent. > > I'm guessing that "30" is (longest - alignment) = 46 - 16 = 30? And > then it wasn't rounded down from there? > >> For ext3: >> longest = 32, alloc_len = 17, args->minlen=2, args->maxlen=2, alignslop=0, >> available=362; longest > alloc_len, so do nothing > > Same issue here - for a forced align allocation, minlen needs to be > bound to alignment constraints. If minlen is set like this, then the > allocation will always return a 2 block extent if they exist. > > IOWs, the allocation constraints are not correctly aligned, but the > allocation is doing exactly what the constraints say it is allowed > to do. Therefore the issue is in the constraint setup (args->minlen) > for forced alignment and not the allocation code that selects a > an args->minlen extent that is not correctly sized near ENOSPC. > > I'm guessing that we need something like the (untested) patch below > to ensure args->minlen is properly aligned.... > > -Dave. For some reason that diff got excluded or mangled when I tried to reply. Anyway, it seems to work ok, but that's based only on a quite limited amount of testing. Thanks, John
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c index 9da52e92172a..215265e0f68f 100644 --- a/fs/xfs/libxfs/xfs_alloc.c +++ b/fs/xfs/libxfs/xfs_alloc.c @@ -2411,14 +2411,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;