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);
 		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)
 {
 	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)
 {
 	int			error;
 	bool			found;
@@ -578,7 +580,7 @@ xfs_reflink_allocate_cow(
 	}
 
 	error = xfs_find_trim_cow_extent(ip, imap, cmap, shared, &found);
-	if (error || !*shared)
+	if (error || (!*shared && !atomic))
 		return error;
 
 	/* CoW fork has a real extent */
@@ -592,7 +594,7 @@ xfs_reflink_allocate_cow(
 	 */
 	if (cmap->br_startoff > imap->br_startoff)
 		return xfs_reflink_fill_cow_hole(ip, imap, cmap, shared,
-				lockmode, convert_now);
+				lockmode, convert_now, atomic);
 
 	/*
 	 * CoW fork has a delalloc reservation. Replace it with a real extent.
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index cc4e92278279..754d2bb692d3 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -32,7 +32,7 @@ int xfs_bmap_trim_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap,
 
 int xfs_reflink_allocate_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap,
 		struct xfs_bmbt_irec *cmap, bool *shared, uint *lockmode,
-		bool convert_now);
+		bool convert_now, bool atomic);
 extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset,
 		xfs_off_t count);
 
-- 
2.31.1

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...

>  		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.  Also, is atomic==true only for the
ATOMIC_SW operation?  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.

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

--D

>  {
>  	int			error;
>  	bool			found;
> @@ -578,7 +580,7 @@ xfs_reflink_allocate_cow(
>  	}
>  
>  	error = xfs_find_trim_cow_extent(ip, imap, cmap, shared, &found);
> -	if (error || !*shared)
> +	if (error || (!*shared && !atomic))
>  		return error;
>  
>  	/* CoW fork has a real extent */
> @@ -592,7 +594,7 @@ xfs_reflink_allocate_cow(
>  	 */
>  	if (cmap->br_startoff > imap->br_startoff)
>  		return xfs_reflink_fill_cow_hole(ip, imap, cmap, shared,
> -				lockmode, convert_now);
> +				lockmode, convert_now, atomic);
>  
>  	/*
>  	 * CoW fork has a delalloc reservation. Replace it with a real extent.
> diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
> index cc4e92278279..754d2bb692d3 100644
> --- a/fs/xfs/xfs_reflink.h
> +++ b/fs/xfs/xfs_reflink.h
> @@ -32,7 +32,7 @@ int xfs_bmap_trim_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap,
>  
>  int xfs_reflink_allocate_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap,
>  		struct xfs_bmbt_irec *cmap, bool *shared, uint *lockmode,
> -		bool convert_now);
> +		bool convert_now, bool atomic);
>  extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset,
>  		xfs_off_t count);
>  
> -- 
> 2.31.1
> 
>

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

On Tue, Feb 25, 2025 at 10:58:56AM +0000, John Garry wrote:
> 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?

Sorry, I meant that the double booleans -> flags conversion could be a
cleanup patch at the end of the series.  But first we'd have to figure
out where we want the flags boundaries to be -- do we just pass the
IOMAP_{DIRECT,DAX,ATOMIC_*} flags directly to the reflink code and let
it figure out what to do?  Or do we make the xfs_iomap.c code translate
that into XFS_REFLINK_ALLOC_* flags?

Either way, that is not something that needs to be done in this patch.

> > 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
>

On 25/02/2025 17:37, Darrick J. Wong wrote:
> On Tue, Feb 25, 2025 at 10:58:56AM +0000, John Garry wrote:
>> 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?
> 
> Sorry, I meant that the double booleans -> flags conversion could be a
> cleanup patch at the end of the series.  But first we'd have to figure
> out where we want the flags boundaries to be -- do we just pass the
> IOMAP_{DIRECT,DAX,ATOMIC_*} flags directly to the reflink code and let
> it figure out what to do? 

We have the odd case of @convert_now being set from IS_DAX(inode) in 
xfs_direct_write_iomap_begin() -> xfs_reflink_allocate_cow(), so that 
thwarts the idea of passing the IOMAP flags directly. BTW, it may be 
possible to clear up that IS_DAX() usage - I'm not sure, so I'll check 
again.

> Or do we make the xfs_iomap.c code translate
> that into XFS_REFLINK_ALLOC_* flags?

That is what I was thinking of doing. But, as mentioned, it needs to be 
decided if we pass XFS_REFLINK_ALLOC_* to callees of 
xfs_reflink_allocate_cow(). I'm thinking 'no', as it will only create churn.

> 
> Either way, that is not something that needs to be done in this patch.

Sure


Thanks,
John