Re: [PATCH 1/5] xfs: only allow minlen allocations when near ENOSPC

John Garry
2024-04-03T16:31:58Z

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;