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

[John Garry <john.g.garry@oracle.com>]

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;