linux-kernel - Re: [PATCH v5 05/10] xfs: Iomap SW-based atomic write support

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <9337105f-d35a-4985-ad21-bf0c36c8fd50@oracle.com>
Date: Wed, 12 Mar 2025 14:57:38 +0000
From: John Garry <john.g.garry@...cle.com>
To: Christoph Hellwig <hch@...radead.org>
Cc: brauner@...nel.org, djwong@...nel.org, cem@...nel.org,
        linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, ojaswin@...ux.ibm.com,
        ritesh.list@...il.com, martin.petersen@...cle.com
Subject: Re: [PATCH v5 05/10] xfs: Iomap SW-based atomic write support

On 12/03/2025 13:52, Christoph Hellwig wrote:
> On Wed, Mar 12, 2025 at 09:00:52AM +0000, John Garry wrote:
>>> How is -EAGAIN going to work here given that it is also used to defer
>>> non-blocking requests to the caller blocking context?
>>
>> You are talking about IOMAP_NOWAIT handling, right?
> 
> Yes.
> 
>> If so, we handle that in
>> xfs_file_dio_write_atomic(), similar to xfs_file_dio_write_unaligned(), i.e.
>> if IOMAP_NOWAIT is set and we get -EAGAIN, then we will return -EAGAIN
>> directly to the caller.
> 
> Can you document this including the interaction between the different
> cases of -EAGAIN somewhere?

Sure

We do the same for dio write unaligned - but that already has a big 
comment explaining the retry mechanism.

> 
>>> What is the probem with only setting the flag that causes REQ_ATOMIC
>>> to be set from the file system instead of forcing it when calling
>>> iomap_dio_rw?
>>
>> We have this in __iomap_dio_rw():
>>
>> 	if (dio_flags & IOMAP_DIO_ATOMIC_SW)
>> 		iomi.flags |= IOMAP_ATOMIC_SW;
>> 	else if (iocb->ki_flags & IOCB_ATOMIC)
>>   		iomi.flags |= IOMAP_ATOMIC_HW;
>>
>> I do admit that the checks are a bit uneven, i.e. check vs
>> IOMAP_DIO_ATOMIC_SW and IOCB_ATOMIC
>>
>> If we want a flag to set REQ_ATOMIC from the FS then we need
>> IOMAP_DIO_BIO_ATOMIC, and that would set IOMAP_BIO_ATOMIC. Is that better?
> 
> My expectation from a very cursory view is that iomap would be that
> there is a IOMAP_F_REQ_ATOMIC that is set in ->iomap_begin and which
> would make the core iomap code set REQ_ATOMIC on the bio for that
> iteration.

but we still need to tell ->iomap_begin about IOCB_ATOMIC, hence 
IOMAP_DIO_BIO_ATOMIC which sets IOMAP_BIO_ATOMIC.

We can't allow __iomap_dio_rw() check IOCB_ATOMIC only (and set 
IOMAP_BIO_ATOMIC), as this is the common path for COW and regular atomic 
write

> 
>>> Also how you ensure this -EAGAIN only happens on the first extent
>>> mapped and you doesn't cause double writes?
>>
>> When we find that a mapping does not suit REQ_ATOMIC-based atomic write,
>> then we immediately bail and retry with FS-based atomic write. And that
>> check should cover all requirements for a REQ_ATOMIC-based atomic write:
>> - aligned
>> - contiguous blocks, i.e. the mapping covers the full write
>>
>> And we also have the check in iomap_dio_bit_iter() to ensure that the
>> mapping covers the full write (for REQ_ATOMIC-based atomic write).
> 
> Ah, I guess that's the
> 
> 	if (bio_atomic && length != iter->len)
> 		return -EINVAL;
> 
> So yes, please adda comment there that this is about a single iteration
> covering the entire write.

ok, fine.

Thanks,
John

>