Re: [PATCH v5 04/10] xfs: Reflink CoW-based atomic write support

[Christoph Hellwig <hch@infradead.org>]

On Mon, Mar 10, 2025 at 06:39:40PM +0000, John Garry wrote:
> Base SW-based atomic writes on CoW.
> 
> For SW-based atomic write support, always allocate a cow hole in
> xfs_reflink_allocate_cow() to write the new data.

What is a "COW hole"?

> The semantics is that if @atomic_sw is set, we will be passed a CoW fork
> extent mapping for no error returned.

This commit log feels extremely sparse for a brand new feature with
data integrity impact.  Can you expand on it a little?

> +	bool			atomic_sw = flags & XFS_REFLINK_ATOMIC_SW;

atomic_sw is not a very descriptive variable name.

>  
>  	resaligned = xfs_aligned_fsb_count(imap->br_startoff,
>  		imap->br_blockcount, xfs_get_cowextsz_hint(ip));
> @@ -466,7 +467,7 @@ xfs_reflink_fill_cow_hole(
>  	*lockmode = XFS_ILOCK_EXCL;
>  
>  	error = xfs_find_trim_cow_extent(ip, imap, cmap, shared, &found);
> -	if (error || !*shared)
> +	if (error || (!*shared && !atomic_sw))

And it's pnly used once.  Basically is is used to force COW, right?
Maybe use that fact as it describes the semantics at this level
instead of the very high level intent?

> @@ -10,6 +10,7 @@
>   * Flags for xfs_reflink_allocate_cow()
>   */
>  #define XFS_REFLINK_CONVERT	(1u << 0) /* convert unwritten extents now */
> +#define XFS_REFLINK_ATOMIC_SW	(1u << 1) /* alloc for SW-based atomic write */

Please expand what this actually means at the xfs_reflink_allocate_cow.
Of if it is just a force flag as I suspect speel that out.  And
move the comment up to avoid the overly long line as well as giving
you space to actually spell the semantics out.