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

[Christoph Hellwig <hch@infradead.org>]

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?

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?

Also how you ensure this -EAGAIN only happens on the first extent
mapped and you doesn't cause double writes?

> +	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.

> +		/*
> +		 * 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.