Re: [PATCH v5 04/10] xfs: Reflink CoW-based atomic write support

[John Garry <john.g.garry@oracle.com>]

On 12/03/2025 07:27, Christoph Hellwig wrote:
> On Mon, Mar 10, 2025 at 06:39:40PM +0000, John Garry wrote:
>> Base SW-based atomic writes on CoW.
>>
>> For SW-based atomic write support, always allocate a cow hole in
>> xfs_reflink_allocate_cow() to write the new data.
> 
> What is a "COW hole"?

I really mean a cow mapping. I can reword that.

> 
>> The semantics is that if @atomic_sw is set, we will be passed a CoW fork
>> extent mapping for no error returned.
> 
> This commit log feels extremely sparse for a brand new feature with
> data integrity impact.  Can you expand on it a little?

Sure, will do

> 
>> +	bool			atomic_sw = flags & XFS_REFLINK_ATOMIC_SW;
> 
> atomic_sw is not a very descriptive variable name.

ack

> 
>>   
>>   	resaligned = xfs_aligned_fsb_count(imap->br_startoff,
>>   		imap->br_blockcount, xfs_get_cowextsz_hint(ip));
>> @@ -466,7 +467,7 @@ xfs_reflink_fill_cow_hole(
>>   	*lockmode = XFS_ILOCK_EXCL;
>>   
>>   	error = xfs_find_trim_cow_extent(ip, imap, cmap, shared, &found);
>> -	if (error || !*shared)
>> +	if (error || (!*shared && !atomic_sw))
> 
> And it's pnly used once.  Basically is is used to force COW, right?

Yes, we force it. Indeed, I think that is term you used a long time ago 
in your RFC for atomic file updates.

But that flag is being used to set XFS_BMAPI_EXTSZALIGN, so feels like a 
bit of a disconnect as why we would set XFS_BMAPI_EXTSZALIGN for "forced 
cow". I would need to spell that out.


> Maybe use that fact as it describes the semantics at this level
> instead of the very high level intent?

ok, fine

> 
>> @@ -10,6 +10,7 @@
>>    * Flags for xfs_reflink_allocate_cow()
>>    */
>>   #define XFS_REFLINK_CONVERT	(1u << 0) /* convert unwritten extents now */
>> +#define XFS_REFLINK_ATOMIC_SW	(1u << 1) /* alloc for SW-based atomic write */
> 
> Please expand what this actually means at the xfs_reflink_allocate_cow.
> Of if it is just a force flag as I suspect speel that out.  And
> move the comment up to avoid the overly long line as well as giving
> you space to actually spell the semantics out.

OK, I can do that, especially since XFS_REFLINK_CONVERT is going to be 
renamed.

Thanks,
John