On 12/03/2025 07:37, Christoph Hellwig wrote:
On Mon, Mar 10, 2025 at 06:39:41PM +0000, John Garry wrote:
In cases of an atomic write occurs for misaligned or discontiguous disk
blocks, we will use a CoW-based method to issue the atomic write.
So, for that case, return -EAGAIN to request that the write be issued in
CoW atomic write mode. The dio write path should detect this, similar to
how misaligned regular DIO writes are handled.
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? 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.
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?
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).
+ bool atomic_hw = flags & IOMAP_ATOMIC_HW;
Again, atomic_hw is not a very useful variable name. But the
whole idea of using a non-descriptive bool variable for a flags
field feels like an antipattern to me.
- if (shared)
+ if (shared) {
+ if (atomic_hw &&
+ !xfs_bmap_valid_for_atomic_write(&cmap,
+ offset_fsb, end_fsb)) {
+ error = -EAGAIN;
+ goto out_unlock;
+ }
goto out_found_cow;
This needs a big fat comment explaining why bailing out here is
fine and how it works.
ok
+ /*
+ * Use CoW method for when we need to alloc > 1 block,
+ * otherwise we might allocate less than what we need here and
+ * have multiple mappings.
+ */
Describe why this is done, not just what is done.
I did say that we may get multiple mappings, which obvs is not useful
for REQ_ATOMIC-based atomic write. But I can add a bit more detail.
Thanks,
John
