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! Maybe spell out the actual block layer rule, though? >> >> 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. */ > >> >>> + 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.
