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

[John Garry]

On 17/03/2025 07:26, Christoph Hellwig wrote:
> > +static bool
> > +xfs_bmap_valid_for_atomic_write(
>
> This is misnamed.  It checks if the hardware offload an be used.

ok, so maybe:

xfs_bmap_atomic_write_hw_possible()?


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

Fine, so it will be something like "atomic writes are required to be 
naturally aligned for disk blocks, which is a block layer rule to ensure 
that we won't straddle any boundary or violate write alignment requirement".

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

nothing special

> If xfs_bmap_valid_for_atomic_write was properly named and documented
> this comment should probably just go away.

sure

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

I can add a comment, but please let me know of any name suggestion

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

We next call xfs_reflink_allocate_cow(), which uses the imap as the 
basis to carry the offset and count.

Are you hinting at statically declaring imap, like:

struct xfs_bmbt_irec imap = {
	.br_startoff		= offset_fsb,
	.br_startblock		= HOLESTARTBLOCK, //?
	.br_blockcount		= end_fsb - offset_fsb,
	.br_state		= XFS_EXT_UNWRITTEN,
};

Note I am not sure what problems which could be encountered in such an 
approach.