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

["Darrick J. Wong" <djwong@kernel.org>]

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