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(-)
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);
--
2.31.1On 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 .?
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
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.
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.
© 2016 - 2025 Red Hat, Inc.