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

John Garry
2025-03-18T08:23:44Z

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.

>