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

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

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