diff mbox series

[v6,04/13] xfs: pass flags to xfs_reflink_allocate_cow()

Message ID 20250313171310.1886394-5-john.g.garry@oracle.com (mailing list archive)
State New
Headers show
Series large atomic writes for xfs with CoW | expand

Commit Message

John Garry March 13, 2025, 5:13 p.m. UTC 
In future we will want more boolean options for xfs_reflink_allocate_cow(),
so just prepare for this by passing a flags arg for @convert_now.

Suggested-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 fs/xfs/xfs_iomap.c   |  7 +++++--
 fs/xfs/xfs_reflink.c | 12 ++++++++----
 fs/xfs/xfs_reflink.h |  8 +++++++-
 3 files changed, 20 insertions(+), 7 deletions(-)

Comments

hch March 17, 2025, 6:15 a.m. UTC | #1 
On Thu, Mar 13, 2025 at 05:13:01PM +0000, John Garry wrote:
> @@ -823,6 +824,9 @@ xfs_direct_write_iomap_begin(
>  	if (xfs_is_shutdown(mp))
>  		return -EIO;
>  
> +	if (flags & IOMAP_DIRECT || IS_DAX(inode))
> +		reflink_flags |= XFS_REFLINK_CONVERT_UNWRITTEN;

Given that this is where the policy is implemented now, this comment:

	/*
	 * COW fork extents are supposed to remain unwritten until we're ready
         * to initiate a disk write.  For direct I/O we are going to write the
	 * data and need the conversion, but for buffered writes we're done.
         */        

from xfs_reflink_convert_unwritten should probably move here now.

> -	return xfs_reflink_convert_unwritten(ip, imap, cmap, convert_now);
> +	return xfs_reflink_convert_unwritten(ip, imap, cmap,
> +			flags & XFS_REFLINK_CONVERT_UNWRITTEN);

I'd probably thread the flags argument all the way through
xfs_reflink_convert_unwritten as that documents the intent better.

> +/*
> + * Flags for xfs_reflink_allocate_cow() and callees
> + */

And the full sentence with a .?
John Garry March 17, 2025, 9:17 a.m. UTC | #2 
On 17/03/2025 06:15, Christoph Hellwig wrote:
> On Thu, Mar 13, 2025 at 05:13:01PM +0000, John Garry wrote:
>> @@ -823,6 +824,9 @@ xfs_direct_write_iomap_begin(
>>   	if (xfs_is_shutdown(mp))
>>   		return -EIO;
>>   
>> +	if (flags & IOMAP_DIRECT || IS_DAX(inode))
>> +		reflink_flags |= XFS_REFLINK_CONVERT_UNWRITTEN;
> 
> Given that this is where the policy is implemented now, this comment:
> 
> 	/*
> 	 * COW fork extents are supposed to remain unwritten until we're ready
>           * to initiate a disk write.  For direct I/O we are going to write the
> 	 * data and need the conversion, but for buffered writes we're done.
>           */
> 
> from xfs_reflink_convert_unwritten should probably move here now.

ok, fine, I can relocate this comment to xfs_direct_write_iomap_begin(), 
but please let me know if you prefer an rewording.

> 
>> -	return xfs_reflink_convert_unwritten(ip, imap, cmap, convert_now);
>> +	return xfs_reflink_convert_unwritten(ip, imap, cmap,
>> +			flags & XFS_REFLINK_CONVERT_UNWRITTEN);
> 
> I'd probably thread the flags argument all the way through
> xfs_reflink_convert_unwritten as that documents the intent better.

ok


> 
>> +/*
>> + * Flags for xfs_reflink_allocate_cow() and callees
>> + */
> 
> And the full sentence with a .?
> 
ok
hch March 18, 2025, 5:33 a.m. UTC | #3 
On Mon, Mar 17, 2025 at 09:17:10AM +0000, John Garry wrote:
>>
>> Given that this is where the policy is implemented now, this comment:
>>
>> 	/*
>> 	 * COW fork extents are supposed to remain unwritten until we're ready
>>           * to initiate a disk write.  For direct I/O we are going to write the
>> 	 * data and need the conversion, but for buffered writes we're done.
>>           */
>>
>> from xfs_reflink_convert_unwritten should probably move here now.
>
> ok, fine, I can relocate this comment to xfs_direct_write_iomap_begin(), 
> but please let me know if you prefer an rewording.

I have to admit I found the wording a bit odd, but I failed to come up
with something significantly better.
John Garry March 18, 2025, 8:12 a.m. UTC | #4 
On 18/03/2025 05:33, Christoph Hellwig wrote:
> On Mon, Mar 17, 2025 at 09:17:10AM +0000, John Garry wrote:
>>>
>>> Given that this is where the policy is implemented now, this comment:
>>>
>>> 	/*
>>> 	 * COW fork extents are supposed to remain unwritten until we're ready
>>>            * to initiate a disk write.  For direct I/O we are going to write the
>>> 	 * data and need the conversion, but for buffered writes we're done.
>>>            */
>>>
>>> from xfs_reflink_convert_unwritten should probably move here now.
>>
>> ok, fine, I can relocate this comment to xfs_direct_write_iomap_begin(),
>> but please let me know if you prefer an rewording.
> 
> I have to admit I found the wording a bit odd, but I failed to come up
> with something significantly better.
> 
maybe when you see the code, you could have a better suggestion, but 
I'll keep the wording the same verbatim for now.
diff mbox series

Patch

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 9a22ecd794eb..8196e66b099b 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -813,6 +813,7 @@  xfs_direct_write_iomap_begin(
 	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
 	xfs_fileoff_t		end_fsb = xfs_iomap_end_fsb(mp, offset, length);
 	int			nimaps = 1, error = 0;
+	unsigned int		reflink_flags = 0;
 	bool			shared = false;
 	u16			iomap_flags = 0;
 	unsigned int		lockmode;
@@ -823,6 +824,9 @@  xfs_direct_write_iomap_begin(
 	if (xfs_is_shutdown(mp))
 		return -EIO;
 
+	if (flags & IOMAP_DIRECT || IS_DAX(inode))
+		reflink_flags |= XFS_REFLINK_CONVERT_UNWRITTEN;
+
 	/*
 	 * Writes that span EOF might trigger an IO size update on completion,
 	 * so consider them to be dirty for the purposes of O_DSYNC even if
@@ -870,8 +874,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));
+				&lockmode, reflink_flags);
 		if (error)
 			goto out_unlock;
 		if (shared)
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index cc3b4df88110..f8363c6b0f39 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -435,7 +435,7 @@  xfs_reflink_fill_cow_hole(
 	struct xfs_bmbt_irec	*cmap,
 	bool			*shared,
 	uint			*lockmode,
-	bool			convert_now)
+	unsigned int		flags)
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp;
@@ -488,7 +488,8 @@  xfs_reflink_fill_cow_hole(
 		return error;
 
 convert:
-	return xfs_reflink_convert_unwritten(ip, imap, cmap, convert_now);
+	return xfs_reflink_convert_unwritten(ip, imap, cmap,
+			flags & XFS_REFLINK_CONVERT_UNWRITTEN);
 
 out_trans_cancel:
 	xfs_trans_cancel(tp);
@@ -566,10 +567,13 @@  xfs_reflink_allocate_cow(
 	struct xfs_bmbt_irec	*cmap,
 	bool			*shared,
 	uint			*lockmode,
-	bool			convert_now)
+	unsigned int		flags)
 {
 	int			error;
 	bool			found;
+	bool			convert_now;
+
+	convert_now = flags & XFS_REFLINK_CONVERT_UNWRITTEN;
 
 	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
 	if (!ip->i_cowfp) {
@@ -592,7 +596,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, flags);
 
 	/*
 	 * 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..18f9624017cd 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -6,6 +6,12 @@ 
 #ifndef __XFS_REFLINK_H
 #define __XFS_REFLINK_H 1
 
+/*
+ * Flags for xfs_reflink_allocate_cow() and callees
+ */
+/* convert unwritten extents now */
+#define XFS_REFLINK_CONVERT_UNWRITTEN		(1u << 0)
+
 /*
  * Check whether it is safe to free COW fork blocks from an inode. It is unsafe
  * to do so when an inode has dirty cache or I/O in-flight, even if no shared
@@ -32,7 +38,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);
+		unsigned int flags);
 extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset,
 		xfs_off_t count);