On 18/03/2025 05:39, Christoph Hellwig wrote:
On Mon, Mar 17, 2025 at 10:18:58AM +0000, John Garry wrote:
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()?
That does sound better.
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".
Much better!
good
Maybe spell out the actual block layer rule, though?
ok, fine, so that will be something like "writes need to be naturally
aligned to ensure that we don't straddle any bdev atomic boundary".
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
/*
* Handler for atomic writes implemented by writing out of place through
* the COW fork. If possible we try to use hardware provided atomicy
* instead, which is handled directly in xfs_direct_write_iomap_begin.
*/
ok, fine
+ 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.
Is xfs_reflink_allocate_cow even the right helper to use? We know we
absolutely want a a COW fork extent, we know there can't be delalloc
extent to convert as we flushed dirty data, so most of the logic in it
is pointless.
Well xfs_reflink_allocate_cow gives us what we want when we set some
flag (XFS_REFLINK_FORCE_COW).
Are you hinting at a dedicated helper? Note that
xfs_reflink_fill_cow_hole() also handles the XFS_REFLINK_FORCE_COW flag.
