Re: [PATCH v6 10/13] xfs: iomap COW-based atomic write support

[Christoph Hellwig]

> +static bool
> +xfs_bmap_valid_for_atomic_write(

This is misnamed.  It checks if the hardware offload an be used.

> +	/* Misaligned start block wrt size */
> +	if (!IS_ALIGNED(imap->br_startblock, imap->br_blockcount))
> +		return false;

It is very obvious that this checks for a natural alignment of the
block number.  But it fails to explain why it checks for that.

> +
> +	/* Discontiguous extents */
> +	if (!imap_spans_range(imap, offset_fsb, end_fsb))

Same here.

> +		if (shared) {
> +			/*
> +			 * Since we got a CoW fork extent mapping, ensure that
> +			 * the mapping is actually suitable for an
> +			 * REQ_ATOMIC-based atomic write, i.e. properly aligned
> +			 * and covers the full range of the write. Otherwise,
> +			 * we need to use the COW-based atomic write mode.
> +			 */
> +			if ((flags & IOMAP_ATOMIC) &&
> +			    !xfs_bmap_valid_for_atomic_write(&cmap,

The "Since.." implies there is something special about COW fork mappings.
But I don't think there is, or am I missing something?
If xfs_bmap_valid_for_atomic_write was properly named and documented
this comment should probably just go away.

> +static int
> +xfs_atomic_write_cow_iomap_begin(

Should the atomic and cow be together for coherent naming?
But even if the naming is coherent it isn't really
self-explanatory, so please add a little top of the function
comment introducing it.

> +	error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, &imap,
> +			&nimaps, 0);
> +	if (error)
> +		goto out_unlock;

Why does this need to read the existing data for mapping?  You'll
overwrite everything through the COW fork anyway.