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

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

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