On 19/03/2025 07:30, Christoph Hellwig wrote:
On Tue, Mar 18, 2025 at 05:44:46PM +0000, John Garry wrote:
Please suggest any further modifications to the following attempt. I have
XFS_REFLINK_FORCE_COW still being passed to xfs_reflink_fill_cow_hole(),
but xfs_reflink_fill_cow_hole() is quite a large function and I am not sure
if I want to duplicate lots of it.
As said I'd do away with the helpers. Below is my completely
untested whiteboard coding attempt, based against the series you
sent out.
it seems to work ok, cheers
Just a query, below
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 88d86cabb8a1..06ece7070cfd 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1083,67 +1083,104 @@ xfs_atomic_write_cow_iomap_begin(
struct iomap *iomap,
struct iomap *srcmap)
{
- ASSERT(flags & IOMAP_WRITE);
- ASSERT(flags & IOMAP_DIRECT);
-
struct xfs_inode *ip = XFS_I(inode);
struct xfs_mount *mp = ip->i_mount;
- struct xfs_bmbt_irec imap, cmap;
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;
- bool shared = false;
- unsigned int lockmode = XFS_ILOCK_EXCL;
+ xfs_filblks_t count_fsb = end_fsb - offset_fsb;
+ int nmaps = 1;
+ xfs_filblks_t resaligned;
+ struct xfs_bmbt_irec cmap;
+ struct xfs_iext_cursor icur;
+ struct xfs_trans *tp;
+ int error;
u64 seq;
+ ASSERT(!XFS_IS_REALTIME_INODE(ip));
+ ASSERT(flags & IOMAP_WRITE);
+ ASSERT(flags & IOMAP_DIRECT);
+
if (xfs_is_shutdown(mp))
return -EIO;
- if (!xfs_has_reflink(mp))
+ if (WARN_ON_ONCE(!xfs_has_reflink(mp)))
return -EINVAL;
- error = xfs_ilock_for_iomap(ip, flags, &lockmode);
+ xfs_ilock(ip, XFS_ILOCK_EXCL);
+
+ if (!ip->i_cowfp) {
+ ASSERT(!xfs_is_reflink_inode(ip));
+ xfs_ifork_init_cow(ip);
+ }
+
+ /*
+ * If we don't find an overlapping extent, trim the range we need to
+ * allocate to fit the hole we found.
+ */
+ if (!xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, &icur, &cmap))
+ cmap.br_startoff = end_fsb;
+ if (cmap.br_startoff <= offset_fsb) {
+ xfs_trim_extent(&cmap, offset_fsb, count_fsb);
+ goto found;
+ }
+
+ end_fsb = cmap.br_startoff;
+ count_fsb = end_fsb - offset_fsb;
+ resaligned = xfs_aligned_fsb_count(offset_fsb, count_fsb,
+ xfs_get_cowextsz_hint(ip));
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
+
+ error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write,
+ XFS_DIOSTRAT_SPACE_RES(mp, resaligned), 0, false, &tp);
if (error)
return error;
- error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, &imap,
- &nimaps, 0);
- if (error)
- goto out_unlock;
+ if (!xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, &icur, &cmap))
+ cmap.br_startoff = end_fsb;
Do we really need this logic?
offset_fsb does not change, and logically cmap.br_startoff == end_fsb
already, right?
+ if (cmap.br_startoff <= offset_fsb) {
+ xfs_trim_extent(&cmap, offset_fsb, count_fsb);
+ xfs_trans_cancel(tp);
+ goto found;
+ }
- /*
- * Use XFS_REFLINK_ALLOC_EXTSZALIGN to hint at aligning new extents
- * according to extszhint, such that there will be a greater chance
- * that future atomic writes to that same range will be aligned (and
- * don't require this COW-based method).
- */
- error = xfs_reflink_allocate_cow(ip, &imap, &cmap, &shared,
- &lockmode, XFS_REFLINK_CONVERT_UNWRITTEN |
- XFS_REFLINK_FORCE_COW | XFS_REFLINK_ALLOC_EXTSZALIGN);
/*
- * Don't check @shared. For atomic writes, we should error when
- * we don't get a COW fork extent mapping.
+ * Allocate the entire reservation as unwritten blocks.
+ *
+ * Use XFS_BMAPI_EXTSZALIGN to hint at aligning new extents according to
+ * extszhint, such that there will be a greater chance that future
+ * atomic writes to that same range will be aligned (and don't require
+ * this COW-based method).
*/
- if (error)
+ error = xfs_bmapi_write(tp, ip, offset_fsb, count_fsb,
+ XFS_BMAPI_COWFORK | XFS_BMAPI_PREALLOC |
+ XFS_BMAPI_EXTSZALIGN, 0, &cmap, &nmaps);
