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.

The semantics is that if @atomic_sw is set, we will be passed a CoW fork
extent mapping for no error returned.

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 fs/xfs/xfs_reflink.c | 5 +++--
 fs/xfs/xfs_reflink.h | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index e3e594c65745..0949d6ba2b3b 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -444,6 +444,7 @@ xfs_reflink_fill_cow_hole(
 	int			nimaps;
 	int			error;
 	bool			found;
+	bool			atomic_sw = flags & XFS_REFLINK_ATOMIC_SW;
 
 	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))
 		goto out_trans_cancel;
 
 	if (found) {
@@ -580,7 +581,7 @@ xfs_reflink_allocate_cow(
 	}
 
 	error = xfs_find_trim_cow_extent(ip, imap, cmap, shared, &found);
-	if (error || !*shared)
+	if (error || (!*shared && !(flags & XFS_REFLINK_ATOMIC_SW)))
 		return error;
 
 	/* CoW fork has a real extent */
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index cdbd73d58822..dfd94e51e2b4 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -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 */
 
 /*
  * Check whether it is safe to free COW fork blocks from an inode. It is unsafe
-- 
2.31.1

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

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

> +	bool			atomic_sw = flags & XFS_REFLINK_ATOMIC_SW;

atomic_sw is not a very descriptive variable name.

>  
>  	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?
Maybe use that fact as it describes the semantics at this level
instead of the very high level intent?

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

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

On Wed, Mar 12, 2025 at 09:13:45AM +0000, John Garry wrote:
> > > +	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 two flags for that even if they currently are set together?
Note that this would go away if we'd always align extsize hinted
allocations, which I suspect is a good idea (even if I'm not 100% sure
about it).

On 12/03/2025 13:45, Christoph Hellwig wrote:
> On Wed, Mar 12, 2025 at 09:13:45AM +0000, John Garry wrote:
>>>> +	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 two flags for that even if they currently are set together?

ok, fine. Then it makes explaining why we set XFS_BMAPI_EXTSZALIGN for 
"force cow" a lot simpler in the code.

> Note that this would go away if we'd always align extsize hinted
> allocations, which I suspect is a good idea (even if I'm not 100% sure
> about it).
> 

Sure

Cheers,
John