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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 5 Mar 2024 13:36:33 +0000
From: John Garry <john.g.garry@...cle.com>
To: Dave Chinner <david@...morbit.com>
Cc: djwong@...nel.org, hch@....de, viro@...iv.linux.org.uk, brauner@...nel.org,
        jack@...e.cz, chandan.babu@...cle.com, axboe@...nel.dk,
        martin.petersen@...cle.com, linux-kernel@...r.kernel.org,
        linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        tytso@....edu, jbongio@...gle.com, ojaswin@...ux.ibm.com,
        ritesh.list@...il.com, linux-block@...r.kernel.org
Subject: Re: [PATCH v2 02/14] fs: xfs: Don't use low-space allocator for
 alignment > 1

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@...cle.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