Re: [PATCH v2 02/14] fs: xfs: Don't use low-space allocator for alignment > 1

John Garry
2024-03-05T13:36:59Z

On 04/03/2024 22:15, Dave Chinner wrote:
> On Mon, Mar 04, 2024 at 01:04:16PM +0000, John Garry wrote:
>> The low-space allocator doesn't honour the alignment requirement, so don't
>> attempt to even use it (when we have an alignment requirement).
>>
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>> ---
>> fs/xfs/libxfs/xfs_bmap.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
>> index f362345467fa..60d100134280 100644
>> --- a/fs/xfs/libxfs/xfs_bmap.c
>> +++ b/fs/xfs/libxfs/xfs_bmap.c
>> @@ -3584,6 +3584,10 @@ xfs_bmap_btalloc_low_space(
>> {
>> int error;
>>
>> + /* The allocator doesn't honour args->alignment */
>> + if (args->alignment > 1)
>> + return 0;
>
> I think that's wrong.
>
> The alignment argument here is purely a best effort consideration -
> we ignore it several different allocation situations, not just low
> space.

Sure, but I am simply addressing the low-space allocator here.

In this series I am /we are effectively trying to conflate
args->alignment > 1 with forcealign. I thought that args->alignment was
guaranteed to be honoured, with some caveats. For forcealign, we
obviously require a guarantee.

>
> e.g. xfs_bmap_btalloc_at_eof() will try exact block
> allocation regardless of whether an alignment parameter is set.

For this specific issue, I think that we are ok, as:
- in xfs_bmap_compute_alignments(), stripe_align is aligned with
args->alignment for forcealign
- xfs_bmap_btalloc_at_eof() has the optimisation to alloc according to
stripe alignment

But obviously we should not be relying on optimisations.

Please also note that I have a modification later in this series to
always have EOF aligned for forcealign.

> It
> will then fall back to stripe alignment if exact block fails.
>
> If stripe aligned allocation fails, it will then set args->alignment
> = 1 and try a full filesystem allocation scan without alignment.
>
> And if that fails, then we finally get to the low space allocator
> with args->alignment = 1 even though we might be trying to allocate
> an aligned extent for an atomic IO....
>
> IOWs, I think this indicates deeper surgery is needed to ensure
> aligned allocations fail immediately and don't fall back to
> unaligned allocations and set XFS_TRANS_LOW_MODE...
>

ok, I'll look at what you write about all of this in the later patch review.

Thanks,
John