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