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
