Re: [PATCH v2 06/11] xfs: Reflink CoW-based atomic write support

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

On 24/02/2025 20:32, Darrick J. Wong wrote:
> On Thu, Feb 13, 2025 at 01:56:14PM +0000, John Garry wrote:
>> For CoW-based atomic write support, always allocate a cow hole in
>> xfs_reflink_allocate_cow() to write the new data.
>>
>> The semantics is that if @atomic is set, we will be passed a CoW fork
>> extent mapping for no error returned.
>>
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>> ---
>>   fs/xfs/xfs_iomap.c   |  2 +-
>>   fs/xfs/xfs_reflink.c | 12 +++++++-----
>>   fs/xfs/xfs_reflink.h |  2 +-
>>   3 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
>> index d61460309a78..ab79f0080288 100644
>> --- a/fs/xfs/xfs_iomap.c
>> +++ b/fs/xfs/xfs_iomap.c
>> @@ -865,7 +865,7 @@ xfs_direct_write_iomap_begin(
>>   		/* may drop and re-acquire the ilock */
>>   		error = xfs_reflink_allocate_cow(ip, &imap, &cmap, &shared,
>>   				&lockmode,
>> -				(flags & IOMAP_DIRECT) || IS_DAX(inode));
>> +				(flags & IOMAP_DIRECT) || IS_DAX(inode), false);
> 
> Now I'm /really/ think it's time for some reflink allocation flags,
> because the function signature now involves two booleans...

ok, but the @convert_now arg is passed to other functions from 
xfs_reflink_allocate_cow() - so would you prefer to create a bool 
@convert_now inside xfs_reflink_allocate_cow() and pass that bool as 
before? Or pass the flags all the way down to end users of @convert_now?

> 
>>   		if (error)
>>   			goto out_unlock;
>>   		if (shared)
>> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
>> index 8428f7b26ee6..3dab3ba900a3 100644
>> --- a/fs/xfs/xfs_reflink.c
>> +++ b/fs/xfs/xfs_reflink.c
>> @@ -435,7 +435,8 @@ xfs_reflink_fill_cow_hole(
>>   	struct xfs_bmbt_irec	*cmap,
>>   	bool			*shared,
>>   	uint			*lockmode,
>> -	bool			convert_now)
>> +	bool			convert_now,
>> +	bool			atomic)
> 
> ...but this can come later. 

Do you mean that this would just be a new flag to set?

 > Also, is atomic==true only for the> ATOMIC_SW operation?

Right, so I think that the variable (or new flag) can be renamed for that.

> I think so, but that's the unfortunate thing about
> booleans.
> 
>>   {
>>   	struct xfs_mount	*mp = ip->i_mount;
>>   	struct xfs_trans	*tp;
>> @@ -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))
>>   		goto out_trans_cancel;
>>   
>>   	if (found) {
>> @@ -566,7 +567,8 @@ xfs_reflink_allocate_cow(
>>   	struct xfs_bmbt_irec	*cmap,
>>   	bool			*shared,
>>   	uint			*lockmode,
>> -	bool			convert_now)
>> +	bool			convert_now,
>> +	bool 			atomic)
> 
> Nit:        ^ space before tab.

ok

> 
> If the answer to the question above is 'yes' then with that nit fixed,
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

Thanks, but I will wait for your feedback to decide whether to pick that up.

John