[RFC PATCH 00/33] XFS: Atomic multi-extent operations via rolling transactions

[RFC PATCH 00/33] XFS: Atomic multi-extent operations via rolling transactions

[Dave Chinner]

Hi folks,

This is a patchset intended to address long-standing races in extent
manipulation operations throughput the IO path. This series
addresses the underlying cause of the problem Darrick recently fixed
in commit 2f4acd0fcd86 ("xfs: resample the data fork mapping after
cycling ILOCK") in that extent manipulations are not atomic w.r.t.
each other and so can race in unexpected and unpredictable ways.

This is not a new issue - these code paths have been a source of
data corruption bugs for a long time. We have several outstanding
bug reports for intermittent test failures caused by warnings or
assert failures due to unexpected mismatches in extent state
(delalloc, data/cow fork inconsistencies, state not matching cached
imap state, etc) that have resisted attempts to reproduce reliably
and/or find a root cause.

Hence I decided that it would probably be a good idea to do the work
necessary to try to address the underlying problems to try to get
rid of this entire class of bug from the code.

I haven't been working on XFS for a year now; I've been working a
completely unrelated cloud-based project, and so I haven't written
any kernel code (nor C code!) for some time. Any mistakes I've made
in the code, commit messages, etc are all my own and stem from being
a bit rusty when it comes to writing kernel XFS code. Point them
out, I'll fix them.

I'll also be up front about the fact that this patchset is based
entirely on LLM predicted code. I review it all and correct any
errors, issues, concerns, etc immediately as each new hunk of code
is predicted by the LLM to prevent errors from compounding and
multiplying. Thus the code ends up the way I'd write it manually,
not the way an unchecked LLM would vibe code it. Mistakes in the
code are mine, not a result of an LLM being used to predict the code
I'd write faster than I can write it myself.

The first problematic pattern this patchset addresses is this:

	- lock ILOCK
	- map extent
	- decide what to do
	- unlock ILOCK
	- allocate transaction
	- lock ILOCK
	- do what was decided
	- commit transaction
	- unlock ILOCK

The issue is that the decision on what to do and actually doing it
is occurring in separate ILOCK contexts - the decision and action
are not atomic, leading to incorrect actions being taken.

This is further exacerbated by the second problematic pattern -
extent manipulations being executed in piece-wise manner and hence
not being atomic with respect to the range over which the action is
being performed. And example of this is unwritten IO completion. It
performs a loop like:

	for the range {offset, len} {
		- allocate transaction
		- lock ILOCK
		- convert next extent in range
		- update i_disk_size
		- commit transaction
		- unlock ILOCK
		- move offset forwards for next extent in range
	}

Hence we can have unwritten extent conversion interact with incoming
writes mapping the same range using the first pattern, and they
lock-step through the extents in the range and the extent map within
the range changes with each ILOCK cycle on both sides. The inherent
raciness of these operations means it is difficult to reason about
them and the races have caused data corruption issues in the past.

There are similar (but more complex) interactions with COW
operations as conversion and fork updates are split across incoming
writes, IO submission and IO completion.

What this patch set is doing is making both patterns atomic with
respect to the ILOCK. Code that uses the first pattern is adjusted
to return all the way back to the start if a transaction is
required. We then allocate a transaction and run the entire
map-decision-modify operation under a single ILOCK context.

For the second pattern, these are converted to use a rolling
transaction so the ILOCK is held across the entire set of
modifications whilst still maintaining the "one extent modification
per transaction" structure of the operation. Whilst this was always
possible with some operations, the operations that required a
physical block reservation for every extent modification (e.g. for
BMBT block allocation) require renewing the block reservation and
hence can receive ENOSPC from those reservations.

Historically speaking, handling errors from within rolling
transactions has been problematic and usually results in a
filesystem shutdown as we can't cancel a dirty transaction cleanly.

Hence to turn these into rolling transactions we require the
addition of a new transaction block reservation regrant mechanism
that can safely return ENOSPC from within a rolling transaction
context. This means we can only regrant block reservations when the
transaction is clean.

When we roll a transaction, we have a new transaction that is clean
after commiting the old transaction, and so this provides a location
for regranting block reservations whilst being able to handle an
ENOSPC error safely. However, if we are in the middle of a defer-ops
chain, we cannot safely unwind even though we have a clean
transaction. i.e. the "atomic modification" of the extent
modification has to complete fully before we can regrant block
reservation space and handle ENOSPC.

We only want to do this block reservation regrant when committing
the final transaction that finishes deferops processing - the
original reservation should already be sufficient for the entire
deferops chain, and so by avoiding regrants in the middle of an
intent processing chain we avoid seeing ENOSPC errors that would
cause a shutdown. Hence we only process regrants at the top level
commit in xfs_defer_finish().

With this mechanism, we can convert the individual transactions in a
loop like unwritten conversion to a rolling transaction that handles
ENOSPC in exactly the same way that the individual transaction
method does. This is how the series makes these multi-extent
modifications atomic with respect to the ILOCK and hence other
extent manipulations.

The overall series addresses the most frequently hit instances of
both patterns. There are other code paths that have similar patterns
that could be converted, but it's not immediately clear that any of
them matter in any material way. If there are such instances, they
can be addressed in future series.

The series passed multiple iterations of the fstests auto group over
mulitple fs configurations without regressions, and it has survived
several hundred iterations of the recoveryloop group without an
unexpected failure. Lots more testing is necessary, but given it
appears to be working it is time to get feedback on the approach and
code changes.

Comments and thoughts welcome.

-Dave.

----

LLM generated summary of the series:

This series of 33 patches fixes several race conditions and bugs in the XFS
extent manipulation code, then reworks the transaction handling to make
multi-extent operations atomic with respect to the ILOCK.

Bug fixes (patches 1-3):

Three pre-existing bugs are fixed. A dirty transaction cancellation in
xfs_bmapi_convert_one_delalloc() where xfs_iext_count_extend() could dirty a
transaction before discovering there was nothing to convert, leading to a
filesystem shutdown on the -EAGAIN cancel path. An isize update bug in
xfs_iomap_write_unwritten() where i_disk_size was set to the end of the entire
write range rather than the end of the actually converted extent, potentially
exposing zeroes after a crash. A block reservation bug in xfs_zoned_end_io()
where the reservation only covered bmbt splits but not the deferred rmap and
refcount btree operations, papered over by the fragile XFS_TRANS_RES_FDBLKS
workaround.

COW direct write atomicity (patches 4-14):

The COW extent allocation path in xfs_direct_write_iomap_begin() is restructured
so that the extent lookup and COW allocation are performed atomically under a
single ILOCK hold. Previously, the ILOCK was cycled between the extent lookup
and the allocation, leaving a window where racing DIO writes could modify the
extent tree and cause stale state bugs. Transaction allocation is lifted to the
caller with a -EAGAIN retry mechanism, and xfs_reflink_fill_cow_hole() and
xfs_reflink_fill_delalloc() are converted to use rolling transactions via
xfs_defer_finish() to keep the ILOCK held across their entire operation.

Delalloc conversion atomicity (patches 15-17):

The xfs_bmapi_convert_delalloc() loop, which converts delalloc extents to real
allocations, is converted from per-iteration transaction allocation to a single
rolling transaction using xfs_defer_finish(). This makes multi-extent delalloc
conversion atomic with respect to other concurrent extent operations.

Block reservation renewal infrastructure (patch 18):

New XFS_TRANS_RENEW_BLKRES transaction flag and xfs_trans_regrant_blkres()
mechanism that automatically restores the block reservation to its original
level after xfs_defer_finish() processes deferred operations. This is needed for
rolling transaction loops where each iteration requires a block reservation for
btree splits.

Unwritten extent conversion atomicity (patches 19-20):

The xfs_iomap_write_unwritten() loop, used for buffered writeback and DIO
completion, is converted from per-iteration transaction allocation to a single
rolling transaction with XFS_TRANS_RENEW_BLKRES for automatic block reservation
renewal.

COW IO completion atomicity (patches 21-23):

The xfs_reflink_end_cow() loop is converted to a rolling transaction, making COW
extent remapping atomic. The xfs_reflink_end_cow_extent() wrapper is removed and
the locked variant renamed since there is no longer an unlocked path.

Zoned IO completion atomicity (patch 24):

The xfs_zoned_end_io() loop is converted to a rolling transaction with the
corrected XFS_RTEXTENTADD_SPACE_RES() block reservation.

Direct write iomap restructure (patches 25-33):

The xfs_direct_write_iomap_begin() function is restructured with a unified
do/while retry loop that handles both COW and non-COW allocation paths
atomically. A new xfs_direct_write_args struct replaces the long parameter lists
throughout the call chain. The pNFS block allocation path is reworked to perform
extent allocation and inode update in a single synchronous transaction. Dead
internal transaction paths are removed from xfs_iomap_write_direct() and
xfs_direct_write_cow_iomap_begin() now that all callers supply transactions.

----

 fs/xfs/libxfs/xfs_bmap.c        |  80 ++++++++-------
 fs/xfs/libxfs/xfs_defer.c       |  38 ++++---
 fs/xfs/libxfs/xfs_shared.h      |   3 +
 fs/xfs/libxfs/xfs_trans_space.h |  12 +++
 fs/xfs/xfs_iomap.c              | 548 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------
 fs/xfs/xfs_iomap.h              |  23 ++++-
 fs/xfs/xfs_pnfs.c               | 117 +++++++++++-----------
 fs/xfs/xfs_reflink.c            | 313 +++++++++++++++++++++-------------------------------------
 fs/xfs/xfs_reflink.h            |   6 +-
 fs/xfs/xfs_trans.c              |  40 +++++++-
 fs/xfs/xfs_trans.h              |   3 +
 fs/xfs/xfs_zone_alloc.c         |  43 +++++---
 12 files changed, 675 insertions(+), 551 deletions(-)

[PATCH 01/33] xfs: fix dirty transaction cancellation in xfs_bmapi_convert_one_delalloc

[Dave Chinner]

xfs_bmapi_convert_one_delalloc() calls xfs_iext_count_extend() before
checking whether there is actually a delalloc extent to convert. If
xfs_iext_count_extend() modifies the inode's extent count fields it
will dirty the transaction. If the subsequent extent lookup finds
nothing to convert (e.g. a racing COW completion moved the extent to
the data fork), the function returns -EAGAIN and the caller cancels
the transaction. Cancelling a dirty transaction triggers a filesystem
shutdown.

Fix this by moving the extent lookup and validation ahead of the
xfs_iext_count_extend() call. This ensures that if there is no extent
to convert, or the extent has already been converted by a racing
thread, the function returns before the transaction is dirtied and
can be safely cancelled.

Fixes: 4f86bb4b66c9 ("xfs: Conditionally upgrade existing inodes to use large extent counters")
Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/libxfs/xfs_bmap.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index d64defeda645..47c4d5c52b95 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4380,11 +4380,11 @@ xfs_bmapi_convert_one_delalloc(
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, 0);
 
-	error = xfs_iext_count_extend(tp, ip, whichfork,
-			XFS_IEXT_ADD_NOSPLIT_CNT);
-	if (error)
-		goto out_trans_cancel;
-
+	/*
+	 * Look up the extent before extending the extent count so that we
+	 * don't dirty the transaction if there is nothing to convert. A
+	 * dirty transaction cancellation on -EAGAIN would shutdown the fs.
+	 */
 	if (!xfs_iext_lookup_extent(ip, ifp, offset_fsb, &bma.icur, &bma.got) ||
 	    bma.got.br_startoff > offset_fsb) {
 		/*
@@ -4409,6 +4409,11 @@ xfs_bmapi_convert_one_delalloc(
 		goto out_trans_cancel;
 	}
 
+	error = xfs_iext_count_extend(tp, ip, whichfork,
+			XFS_IEXT_ADD_NOSPLIT_CNT);
+	if (error)
+		goto out_trans_cancel;
+
 	bma.tp = tp;
 	bma.ip = ip;
 	bma.wasdel = true;
-- 
2.55.0

[PATCH 02/33] xfs: fix isize update in xfs_iomap_write_unwritten to track conversion progress

[Dave Chinner]

xfs_iomap_write_unwritten() updates i_disk_size using the end of
the entire unwritten range (offset_fsb + count_fsb) rather than the
end of the extent that was actually converted in each iteration
(imap.br_startoff + imap.br_blockcount).

If the conversion requires multiple iterations and a crash occurs
partway through, recovery would replay the first transaction which
set i_disk_size to the end of the full range. This exposes
unwritten extents to userspace reads as zeroes rather than the
data that was written, because those extents have not yet been
converted from unwritten to written state.

Fix this by computing i_size from the extent that was actually
converted (imap), so i_disk_size advances incrementally as each
extent is converted. On crash, recovery only exposes data in
extents that have been both written and converted.

Fixes: 84803fb78237 ("xfs: log file size updates as part of unwritten extent conversion")
Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_iomap.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 225c3de88d03..1437ea93563c 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -682,11 +682,15 @@ xfs_iomap_write_unwritten(
 			goto error_on_bmapi_transaction;
 
 		/*
-		 * Log the updated inode size as we go.  We have to be careful
-		 * to only log it up to the actual write offset if it is
-		 * halfway into a block.
+		 * Update the inode size to reflect the extent that was
+		 * converted in this iteration. We must not advance isize
+		 * beyond the extent we just converted, otherwise a crash
+		 * before the next conversion exposes unwritten extents
+		 * (zeroes) to userspace instead of the written data.
+		 * Clamp to the byte-level write end in case the converted
+		 * extent extends past the write boundary.
 		 */
-		i_size = XFS_FSB_TO_B(mp, offset_fsb + count_fsb);
+		i_size = XFS_FSB_TO_B(mp, imap.br_startoff + imap.br_blockcount);
 		if (i_size > offset + count)
 			i_size = offset + count;
 		if (update_isize && i_size > i_size_read(inode))
-- 
2.55.0

[PATCH 03/33] xfs: fix block reservation for zoned RT extent remapping

[Dave Chinner]

xfs_zoned_end_io() uses XFS_EXTENTADD_SPACE_RES() for its block
reservation, which only covers bmbt splits. However, the remap
operation in xfs_zoned_map_extent() generates deferred rmap and
refcount btree updates that also need blocks for btree splits
when they are processed.

The code used XFS_TRANS_RES_FDBLKS as a workaround to recycle
freed data blocks back into the transaction's block reservation.
This is fragile — it depends on freed blocks being large enough
to cover the metadata btree needs, and conflates data block
recycling with metadata reservation.

Fix this by introducing XFS_RTEXTENTADD_SPACE_RES() which computes
the correct reservation for any RT data extent modification: the
bmbt split cost plus the rt rmap btree split cost plus the rt
refcount btree cost. This covers all the deferred operations that
are generated when an RT extent is mapped or unmapped.

Drop XFS_TRANS_RES_FDBLKS from xfs_zoned_end_io() since the
reservation now correctly covers all btree costs.

Note: XFS_RTEXTENTADD_SPACE_RES() should eventually be folded
into XFS_EXTENTADD_SPACE_RES() so that all callers operating on
RT inodes automatically get the correct reservation. There are
approximately 11 sites that use XFS_EXTENTADD_SPACE_RES either
directly or via XFS_DIOSTRAT_SPACE_RES that have the same
under-reservation issue for RT inodes.

Fixes: 4e4d52075577 ("xfs: add the zoned space allocator")
Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/libxfs/xfs_trans_space.h | 12 ++++++++++++
 fs/xfs/xfs_zone_alloc.c         |  5 ++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_trans_space.h b/fs/xfs/libxfs/xfs_trans_space.h
index d89b570aafcc..4cfc29ac764f 100644
--- a/fs/xfs/libxfs/xfs_trans_space.h
+++ b/fs/xfs/libxfs/xfs_trans_space.h
@@ -55,6 +55,18 @@
 	  XFS_MAX_CONTIG_EXTENTS_PER_BLOCK(mp)) * \
 	  XFS_EXTENTADD_SPACE_RES(mp,w))
 
+/*
+ * Blocks needed to add or remove a realtime data extent: the bmbt split plus
+ * rt rmap btree and rt refcount btree updates deferred from the bmbt operation.
+ *
+ * TODO: this should be folded into XFS_EXTENTADD_SPACE_RES() so that all
+ * callers that operate on RT inodes automatically get the correct reservation.
+ */
+#define XFS_RTEXTENTADD_SPACE_RES(mp)	\
+	(XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK) + \
+	 (xfs_has_rmapbt(mp) ? XFS_RTRMAPADD_SPACE_RES(mp) : 0) + \
+	 (xfs_has_reflink(mp) ? 2 * (mp)->m_rtrefc_maxlevels - 1 : 0))
+
 /* Blocks we might need to add "b" mappings & rmappings to a file. */
 #define XFS_SWAP_RMAP_SPACE_RES(mp,b,w)\
 	(XFS_NEXTENTADD_SPACE_RES((mp), (b), (w)) + \
diff --git a/fs/xfs/xfs_zone_alloc.c b/fs/xfs/xfs_zone_alloc.c
index 7d13fa7ab30a..7e3456ca28ac 100644
--- a/fs/xfs/xfs_zone_alloc.c
+++ b/fs/xfs/xfs_zone_alloc.c
@@ -330,8 +330,7 @@ xfs_zoned_end_io(
 		.br_startblock	= xfs_daddr_to_rtb(mp, daddr),
 		.br_state	= XFS_EXT_NORM,
 	};
-	unsigned int		resblks =
-		XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK);
+	unsigned int		resblks = XFS_RTEXTENTADD_SPACE_RES(mp);
 	struct xfs_trans	*tp;
 	int			error;
 
@@ -342,7 +341,7 @@ xfs_zoned_end_io(
 		new.br_blockcount = end_fsb - new.br_startoff;
 
 		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0,
-				XFS_TRANS_RESERVE | XFS_TRANS_RES_FDBLKS, &tp);
+				XFS_TRANS_RESERVE, &tp);
 		if (error)
 			return error;
 		xfs_ilock(ip, XFS_ILOCK_EXCL);
-- 
2.55.0

[PATCH 04/33] xfs: factor out COW iomap handling from xfs_direct_write_iomap_begin()

[Dave Chinner]

Extract the COW extent allocation and iomap setup logic from
xfs_direct_write_iomap_begin() into a new helper function
xfs_direct_write_cow_iomap_begin().

When the inode is a COW inode, the new function handles the entire
COW path: acquiring the ILOCK exclusively, reading the data fork
extent mapping, checking if COW is needed, allocating COW extents
via xfs_reflink_allocate_cow(), and setting up the iomap/srcmap for
the COW write.

The *nimaps output parameter tells the caller what to do next:
  - *nimaps == 0: COW was fully handled, iomap/srcmap are filled in,
    and the ILOCK has been released. The caller returns immediately.
  - *nimaps > 0: The extent is not shared. The imap is valid and the
    ILOCK is still held, so the caller continues with the normal
    allocation or overwrite IO path.

When the inode is not a COW inode, xfs_direct_write_iomap_begin()
handles the locking and extent lookup itself as before, taking
only a shared ILOCK.

This is a pure refactoring with no functional change, done to prepare
for reworking the COW allocation to handle transaction allocation and
retry logic at the xfs_direct_write_cow_iomap_begin() level.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_iomap.c | 184 ++++++++++++++++++++++++++++++---------------
 1 file changed, 123 insertions(+), 61 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 1437ea93563c..df9fc7c6a4b9 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -848,6 +848,111 @@ xfs_bmap_hw_atomic_write_possible(
 	return len <= xfs_inode_buftarg(ip)->bt_awu_max;
 }
 
+/*
+ * Handle COW extent allocation and iomap setup for direct writes to reflinked
+ * files.
+ *
+ * The caller passes in an imap and nimaps that the COW allocation will fill
+ * with the data fork extent mapping. On return, *nimaps indicates whether the
+ * caller needs to continue with the normal IO path:
+ *
+ *   *nimaps == 0: COW was handled, iomap/srcmap are filled in, ILOCK released.
+ *                 Caller should return 0 immediately.
+ *   *nimaps > 0:  Extent is not shared, imap is valid, ILOCK is still held.
+ *                 Caller should continue with the normal IO path.
+ */
+static int
+xfs_direct_write_cow_iomap_begin(
+	struct xfs_inode	*ip,
+	loff_t			offset,
+	loff_t			length,
+	unsigned		flags,
+	struct iomap		*iomap,
+	struct iomap		*srcmap,
+	struct xfs_bmbt_irec	*imap,
+	int			*nimaps,
+	unsigned int		*lockmode,
+	u16			iomap_flags)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_bmbt_irec	cmap;
+	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
+	xfs_fileoff_t		end_fsb = xfs_iomap_end_fsb(mp, offset, length);
+	bool			shared = false;
+	int			error;
+	u64			seq;
+
+	*lockmode = XFS_ILOCK_EXCL;
+
+relock:
+	error = xfs_ilock_for_iomap(ip, flags, lockmode);
+	if (error)
+		return error;
+
+	/*
+	 * The reflink iflag could have changed since the earlier unlocked
+	 * check, check if it again and relock if needed.
+	 */
+	if (xfs_is_cow_inode(ip) && *lockmode == XFS_ILOCK_SHARED) {
+		xfs_iunlock(ip, *lockmode);
+		*lockmode = XFS_ILOCK_EXCL;
+		goto relock;
+	}
+
+	*nimaps = 1;
+	error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, imap,
+			       nimaps, 0);
+	if (error)
+		goto out_unlock;
+
+	if (!imap_needs_cow(ip, flags, imap, *nimaps))
+		return 0;
+
+	error = -EAGAIN;
+	if (flags & IOMAP_NOWAIT)
+		goto out_unlock;
+
+	/* may drop and re-acquire the ilock */
+	error = xfs_reflink_allocate_cow(ip, imap, &cmap, &shared,
+			lockmode,
+			(flags & IOMAP_DIRECT) || IS_DAX(VFS_I(ip)));
+	if (error)
+		goto out_unlock;
+
+	if (!shared)
+		return 0;
+
+	if ((flags & IOMAP_ATOMIC) &&
+	    !xfs_bmap_hw_atomic_write_possible(ip, &cmap,
+			offset_fsb, end_fsb)) {
+		error = -ENOPROTOOPT;
+		goto out_unlock;
+	}
+
+	/*
+	 * COW extent found and allocated. Set up iomap/srcmap and return
+	 * with *nimaps = 0 to tell the caller the COW path is complete.
+	 */
+	*nimaps = 0;
+	length = XFS_FSB_TO_B(mp, cmap.br_startoff + cmap.br_blockcount);
+	trace_xfs_iomap_found(ip, offset, length - offset, XFS_COW_FORK,
+			&cmap);
+	if (imap->br_startblock != HOLESTARTBLOCK) {
+		seq = xfs_iomap_inode_sequence(ip, 0);
+		error = xfs_bmbt_to_iomap(ip, srcmap, imap, flags, 0, seq);
+		if (error)
+			goto out_unlock;
+	}
+	seq = xfs_iomap_inode_sequence(ip, IOMAP_F_SHARED);
+	xfs_iunlock(ip, *lockmode);
+	return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, IOMAP_F_SHARED, seq);
+
+out_unlock:
+	if (*lockmode)
+		xfs_iunlock(ip, *lockmode);
+	return error;
+}
+
 static int
 xfs_direct_write_iomap_begin(
 	struct inode		*inode,
@@ -859,12 +964,11 @@ xfs_direct_write_iomap_begin(
 {
 	struct xfs_inode	*ip = XFS_I(inode);
 	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_bmbt_irec	imap, cmap;
+	struct xfs_bmbt_irec	imap;
 	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
 	xfs_fileoff_t		end_fsb = xfs_iomap_end_fsb(mp, offset, length);
 	xfs_fileoff_t		orig_end_fsb = end_fsb;
 	int			nimaps = 1, error = 0;
-	bool			shared = false;
 	u16			iomap_flags = 0;
 	bool			needs_alloc;
 	unsigned int		lockmode;
@@ -887,57 +991,28 @@ xfs_direct_write_iomap_begin(
 	if (flags & IOMAP_ATOMIC)
 		iomap_flags |= IOMAP_F_ATOMIC_BIO;
 
-	/*
-	 * COW writes may allocate delalloc space or convert unwritten COW
-	 * extents, so we need to make sure to take the lock exclusively here.
-	 */
-	if (xfs_is_cow_inode(ip))
-		lockmode = XFS_ILOCK_EXCL;
-	else
-		lockmode = XFS_ILOCK_SHARED;
-
-relock:
-	error = xfs_ilock_for_iomap(ip, flags, &lockmode);
-	if (error)
-		return error;
-
-	/*
-	 * The reflink iflag could have changed since the earlier unlocked
-	 * check, check if it again and relock if needed.
-	 */
-	if (xfs_is_cow_inode(ip) && lockmode == XFS_ILOCK_SHARED) {
-		xfs_iunlock(ip, lockmode);
-		lockmode = XFS_ILOCK_EXCL;
-		goto relock;
-	}
+	if (xfs_is_cow_inode(ip)) {
+		error = xfs_direct_write_cow_iomap_begin(ip, offset, length,
+				flags, iomap, srcmap, &imap, &nimaps,
+				&lockmode, iomap_flags);
+		if (error)
+			return error;
+		if (!nimaps)
+			return 0;
 
-	error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, &imap,
-			       &nimaps, 0);
-	if (error)
-		goto out_unlock;
+		end_fsb = imap.br_startoff + imap.br_blockcount;
+		length = XFS_FSB_TO_B(mp, end_fsb) - offset;
+	} else {
+		lockmode = XFS_ILOCK_SHARED;
 
-	if (imap_needs_cow(ip, flags, &imap, nimaps)) {
-		error = -EAGAIN;
-		if (flags & IOMAP_NOWAIT)
-			goto out_unlock;
+		error = xfs_ilock_for_iomap(ip, flags, &lockmode);
+		if (error)
+			return error;
 
-		/* may drop and re-acquire the ilock */
-		error = xfs_reflink_allocate_cow(ip, &imap, &cmap, &shared,
-				&lockmode,
-				(flags & IOMAP_DIRECT) || IS_DAX(inode));
+		error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb,
+				&imap, &nimaps, 0);
 		if (error)
 			goto out_unlock;
-		if (shared) {
-			if ((flags & IOMAP_ATOMIC) &&
-			    !xfs_bmap_hw_atomic_write_possible(ip, &cmap,
-					offset_fsb, end_fsb)) {
-				error = -ENOPROTOOPT;
-				goto out_unlock;
-			}
-			goto out_found_cow;
-		}
-		end_fsb = imap.br_startoff + imap.br_blockcount;
-		length = XFS_FSB_TO_B(mp, end_fsb) - offset;
 	}
 
 	needs_alloc = imap_needs_alloc(inode, flags, &imap, nimaps);
@@ -1022,19 +1097,6 @@ xfs_direct_write_iomap_begin(
 	return xfs_bmbt_to_iomap(ip, iomap, &imap, flags,
 				 iomap_flags | IOMAP_F_NEW, seq);
 
-out_found_cow:
-	length = XFS_FSB_TO_B(mp, cmap.br_startoff + cmap.br_blockcount);
-	trace_xfs_iomap_found(ip, offset, length - offset, XFS_COW_FORK, &cmap);
-	if (imap.br_startblock != HOLESTARTBLOCK) {
-		seq = xfs_iomap_inode_sequence(ip, 0);
-		error = xfs_bmbt_to_iomap(ip, srcmap, &imap, flags, 0, seq);
-		if (error)
-			goto out_unlock;
-	}
-	seq = xfs_iomap_inode_sequence(ip, IOMAP_F_SHARED);
-	xfs_iunlock(ip, lockmode);
-	return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, IOMAP_F_SHARED, seq);
-
 out_unlock:
 	if (lockmode)
 		xfs_iunlock(ip, lockmode);
-- 
2.55.0

[PATCH 05/33] xfs: plumb xfs_trans through xfs_reflink_allocate_cow and fill_cow_hole

[Dave Chinner]

Add a struct xfs_trans pointer as the first parameter to
xfs_reflink_allocate_cow() and xfs_reflink_fill_cow_hole().
All callers currently pass NULL.

This is preparation for having the caller pre-allocate a transaction
and pass it down to avoid needing to cycle the ILOCK inside
fill_cow_hole().

The local transaction variable in fill_cow_hole() is renamed to
local_tp to avoid shadowing the new parameter.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_iomap.c   |  2 +-
 fs/xfs/xfs_reflink.c | 17 ++++++++++-------
 fs/xfs/xfs_reflink.h |  6 +++---
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index df9fc7c6a4b9..e4e64412b431 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -913,7 +913,7 @@ xfs_direct_write_cow_iomap_begin(
 		goto out_unlock;
 
 	/* may drop and re-acquire the ilock */
-	error = xfs_reflink_allocate_cow(ip, imap, &cmap, &shared,
+	error = xfs_reflink_allocate_cow(NULL, ip, imap, &cmap, &shared,
 			lockmode,
 			(flags & IOMAP_DIRECT) || IS_DAX(VFS_I(ip)));
 	if (error)
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 480136136635..1fa899048b24 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -430,6 +430,7 @@ xfs_reflink_convert_unwritten(
 
 static int
 xfs_reflink_fill_cow_hole(
+	struct xfs_trans	*tp,
 	struct xfs_inode	*ip,
 	struct xfs_bmbt_irec	*imap,
 	struct xfs_bmbt_irec	*cmap,
@@ -438,7 +439,7 @@ xfs_reflink_fill_cow_hole(
 	bool			convert_now)
 {
 	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_trans	*tp;
+	struct xfs_trans	*local_tp;
 	xfs_filblks_t		resaligned;
 	unsigned int		seq_before = READ_ONCE(ip->i_df.if_seq);
 	unsigned int		dblocks = 0, rblocks = 0;
@@ -460,7 +461,7 @@ xfs_reflink_fill_cow_hole(
 	*lockmode = 0;
 
 	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, dblocks,
-			rblocks, false, &tp);
+			rblocks, false, &local_tp);
 	if (error)
 		return error;
 
@@ -487,20 +488,21 @@ xfs_reflink_fill_cow_hole(
 		goto out_trans_cancel;
 
 	if (found) {
-		xfs_trans_cancel(tp);
+		xfs_trans_cancel(local_tp);
 		goto convert;
 	}
 
 	/* Allocate the entire reservation as unwritten blocks. */
 	nimaps = 1;
-	error = xfs_bmapi_write(tp, ip, imap->br_startoff, imap->br_blockcount,
+	error = xfs_bmapi_write(local_tp, ip, imap->br_startoff,
+			imap->br_blockcount,
 			XFS_BMAPI_COWFORK | XFS_BMAPI_PREALLOC, 0, cmap,
 			&nimaps);
 	if (error)
 		goto out_trans_cancel;
 
 	xfs_inode_set_cowblocks_tag(ip);
-	error = xfs_trans_commit(tp);
+	error = xfs_trans_commit(local_tp);
 	if (error)
 		return error;
 
@@ -508,7 +510,7 @@ xfs_reflink_fill_cow_hole(
 	return xfs_reflink_convert_unwritten(ip, imap, cmap, convert_now);
 
 out_trans_cancel:
-	xfs_trans_cancel(tp);
+	xfs_trans_cancel(local_tp);
 	return error;
 }
 
@@ -597,6 +599,7 @@ xfs_reflink_fill_delalloc(
 /* Allocate all CoW reservations covering a range of blocks in a file. */
 int
 xfs_reflink_allocate_cow(
+	struct xfs_trans	*tp,
 	struct xfs_inode	*ip,
 	struct xfs_bmbt_irec	*imap,
 	struct xfs_bmbt_irec	*cmap,
@@ -627,7 +630,7 @@ xfs_reflink_allocate_cow(
 	 * Allocate a real extent in the CoW fork.
 	 */
 	if (cmap->br_startoff > imap->br_startoff)
-		return xfs_reflink_fill_cow_hole(ip, imap, cmap, shared,
+		return xfs_reflink_fill_cow_hole(NULL, ip, imap, cmap, shared,
 				lockmode, convert_now);
 
 	/*
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index 9d1ed9bb0bee..6a984363edbc 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -30,9 +30,9 @@ int xfs_reflink_trim_around_shared(struct xfs_inode *ip,
 int xfs_bmap_trim_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap,
 		bool *shared);
 
-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);
+int xfs_reflink_allocate_cow(struct xfs_trans *tp, struct xfs_inode *ip,
+		struct xfs_bmbt_irec *imap, struct xfs_bmbt_irec *cmap,
+		bool *shared, uint *lockmode, bool convert_now);
 extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset,
 		xfs_off_t count);
 int xfs_reflink_convert_cow_locked(struct xfs_inode *ip,
-- 
2.55.0

[PATCH 06/33] xfs: teach xfs_reflink_fill_cow_hole() to use a caller-supplied transaction

[Dave Chinner]

When xfs_reflink_fill_cow_hole() is called with a non-NULL transaction
pointer, use it directly for the COW extent allocation instead of
dropping the ILOCK and allocating a new transaction internally.

When a caller-supplied transaction is used:
- The ILOCK drop/reacquire cycle is skipped entirely
- The caller is responsible for committing or cancelling the transaction
- xfs_find_trim_cow_extent() is still called to re-validate the COW
  fork state

When tp is NULL, the existing behaviour is preserved: the function
drops the ILOCK, allocates a transaction (which re-acquires the ILOCK),
and handles commit/cancel internally.

No callers pass a non-NULL transaction yet, so there is no functional
change.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_reflink.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 1fa899048b24..1ad0569ecdd1 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -439,7 +439,7 @@ xfs_reflink_fill_cow_hole(
 	bool			convert_now)
 {
 	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_trans	*local_tp;
+	struct xfs_trans	*local_tp = NULL;
 	xfs_filblks_t		resaligned;
 	unsigned int		seq_before = READ_ONCE(ip->i_df.if_seq);
 	unsigned int		dblocks = 0, rblocks = 0;
@@ -447,6 +447,16 @@ xfs_reflink_fill_cow_hole(
 	int			error;
 	bool			found;
 
+	/*
+	 * If the caller supplied a transaction, use it directly. The caller
+	 * is responsible for commit/cancel and holds the ILOCK.
+	 *
+	 * Otherwise, we need to drop the ILOCK and allocate a transaction
+	 * ourselves, which will re-acquire the ILOCK.
+	 */
+	if (tp)
+		goto allocate;
+
 	resaligned = xfs_aligned_fsb_count(imap->br_startoff,
 		imap->br_blockcount, xfs_get_cowextsz_hint(ip));
 	if (XFS_IS_REALTIME_INODE(ip)) {
@@ -466,6 +476,7 @@ xfs_reflink_fill_cow_hole(
 		return error;
 
 	*lockmode = XFS_ILOCK_EXCL;
+	tp = local_tp;
 
 	/*
 	 * The data fork mapping may have changed while we dropped the ILOCK
@@ -483,18 +494,20 @@ xfs_reflink_fill_cow_hole(
 			goto out_trans_cancel;
 	}
 
+allocate:
 	error = xfs_find_trim_cow_extent(ip, imap, cmap, shared, &found);
 	if (error || !*shared)
 		goto out_trans_cancel;
 
 	if (found) {
-		xfs_trans_cancel(local_tp);
+		if (local_tp)
+			xfs_trans_cancel(local_tp);
 		goto convert;
 	}
 
 	/* Allocate the entire reservation as unwritten blocks. */
 	nimaps = 1;
-	error = xfs_bmapi_write(local_tp, ip, imap->br_startoff,
+	error = xfs_bmapi_write(tp, ip, imap->br_startoff,
 			imap->br_blockcount,
 			XFS_BMAPI_COWFORK | XFS_BMAPI_PREALLOC, 0, cmap,
 			&nimaps);
@@ -502,15 +515,18 @@ xfs_reflink_fill_cow_hole(
 		goto out_trans_cancel;
 
 	xfs_inode_set_cowblocks_tag(ip);
-	error = xfs_trans_commit(local_tp);
-	if (error)
-		return error;
+	if (local_tp) {
+		error = xfs_trans_commit(local_tp);
+		if (error)
+			return error;
+	}
 
 convert:
 	return xfs_reflink_convert_unwritten(ip, imap, cmap, convert_now);
 
 out_trans_cancel:
-	xfs_trans_cancel(local_tp);
+	if (local_tp)
+		xfs_trans_cancel(local_tp);
 	return error;
 }
 
-- 
2.55.0

[PATCH 07/33] xfs: add transaction retry infrastructure to xfs_direct_write_cow_iomap_begin

[Dave Chinner]

Add the infrastructure for handling COW extent allocation retries with
a pre-allocated transaction in xfs_direct_write_cow_iomap_begin().

When xfs_reflink_allocate_cow() returns -EAGAIN (indicating it needs
a transaction to proceed), the function drops the ILOCK, allocates a
zero-block transaction via xfs_trans_alloc_inode() (which reacquires
the ILOCK), and jumps to retry: to re-read the imap via
xfs_bmapi_read() since the extent tree may have changed while the
ILOCK was not held.

The initial xfs_ilock_for_iomap() call is done once before the retry
label since we always enter with ILOCK_EXCL and xfs_trans_alloc_inode()
re-acquires ILOCK_EXCL on the retry path.

The old relock label and shared-to-exclusive lock upgrade logic is
removed as it is unnecessary - this function always takes ILOCK_EXCL.

Nothing returns -EAGAIN from xfs_reflink_allocate_cow() yet, so there
is no functional change.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_iomap.c | 66 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 53 insertions(+), 13 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index e4e64412b431..017355372a44 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -876,6 +876,7 @@ xfs_direct_write_cow_iomap_begin(
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_bmbt_irec	cmap;
+	struct xfs_trans	*tp = NULL;
 	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
 	xfs_fileoff_t		end_fsb = xfs_iomap_end_fsb(mp, offset, length);
 	bool			shared = false;
@@ -884,41 +885,78 @@ xfs_direct_write_cow_iomap_begin(
 
 	*lockmode = XFS_ILOCK_EXCL;
 
-relock:
 	error = xfs_ilock_for_iomap(ip, flags, lockmode);
 	if (error)
 		return error;
 
-	/*
-	 * The reflink iflag could have changed since the earlier unlocked
-	 * check, check if it again and relock if needed.
-	 */
-	if (xfs_is_cow_inode(ip) && *lockmode == XFS_ILOCK_SHARED) {
-		xfs_iunlock(ip, *lockmode);
-		*lockmode = XFS_ILOCK_EXCL;
-		goto relock;
-	}
-
+retry:
 	*nimaps = 1;
 	error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, imap,
 			       nimaps, 0);
 	if (error)
 		goto out_unlock;
 
-	if (!imap_needs_cow(ip, flags, imap, *nimaps))
+	if (!imap_needs_cow(ip, flags, imap, *nimaps)) {
+		/*
+		 * Extent is not shared - return the imap and ILOCK to the
+		 * caller for normal IO path processing.
+		 */
+		if (tp) {
+			xfs_trans_cancel(tp);
+			tp = NULL;
+		}
 		return 0;
+	}
 
 	error = -EAGAIN;
 	if (flags & IOMAP_NOWAIT)
 		goto out_unlock;
 
 	/* may drop and re-acquire the ilock */
-	error = xfs_reflink_allocate_cow(NULL, ip, imap, &cmap, &shared,
+	error = xfs_reflink_allocate_cow(tp, ip, imap, &cmap, &shared,
 			lockmode,
 			(flags & IOMAP_DIRECT) || IS_DAX(VFS_I(ip)));
+	if (error == -EAGAIN) {
+		/*
+		 * COW allocation needs a transaction. Drop the ILOCK and
+		 * allocate a transaction, which will re-acquire the ILOCK.
+		 * Then retry the imap lookup since the extent tree may have
+		 * changed while the ILOCK was not held.
+		 */
+		xfs_filblks_t		resaligned;
+		unsigned int		dblocks, rblocks;
+
+		ASSERT(!tp);
+
+		xfs_iunlock(ip, *lockmode);
+
+		resaligned = xfs_aligned_fsb_count(offset_fsb,
+				end_fsb - offset_fsb, xfs_get_cowextsz_hint(ip));
+		if (XFS_IS_REALTIME_INODE(ip)) {
+			dblocks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
+			rblocks = resaligned;
+		} else {
+			dblocks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
+			rblocks = 0;
+		}
+
+		error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write,
+				dblocks, rblocks, false, &tp);
+		if (error)
+			return error;
+
+		goto retry;
+	}
 	if (error)
 		goto out_unlock;
 
+	if (tp) {
+		error = xfs_trans_commit(tp);
+		tp = NULL;
+		if (error)
+			goto out_unlock;
+	}
+
 	if (!shared)
 		return 0;
 
@@ -948,6 +986,8 @@ xfs_direct_write_cow_iomap_begin(
 	return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, IOMAP_F_SHARED, seq);
 
 out_unlock:
+	if (tp)
+		xfs_trans_cancel(tp);
 	if (*lockmode)
 		xfs_iunlock(ip, *lockmode);
 	return error;
-- 
2.55.0

[PATCH 08/33] xfs: return -EAGAIN from xfs_reflink_allocate_cow for COW hole without transaction

[Dave Chinner]

When xfs_reflink_allocate_cow() encounters a COW fork hole that needs
a real extent allocated and no transaction has been provided, return
-EAGAIN to tell the caller to allocate a transaction and retry.

When a transaction is provided, pass it through to
xfs_reflink_fill_cow_hole() which will use it directly for the COW
extent allocation without needing to cycle the ILOCK.

This activates the retry loop added in the previous patch: on the
first call (tp == NULL), allocate_cow returns -EAGAIN, the caller
drops the ILOCK, allocates a transaction, and retries. On the
second call (tp != NULL), the transaction is passed down to
fill_cow_hole() and the COW allocation completes atomically
under the ILOCK without cycling it.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_reflink.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 1ad0569ecdd1..56ad76b0f6b9 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -643,16 +643,22 @@ xfs_reflink_allocate_cow(
 
 	/*
 	 * CoW fork does not have an extent and data extent is shared.
-	 * Allocate a real extent in the CoW fork.
+	 * Allocate a real extent in the CoW fork. If the caller has not
+	 * provided a transaction for the allocation, return -EAGAIN to
+	 * tell the caller to allocate a transaction and retry.
 	 */
-	if (cmap->br_startoff > imap->br_startoff)
-		return xfs_reflink_fill_cow_hole(NULL, ip, imap, cmap, shared,
+	if (cmap->br_startoff > imap->br_startoff) {
+		if (!tp)
+			return -EAGAIN;
+		return xfs_reflink_fill_cow_hole(tp, ip, imap, cmap, shared,
 				lockmode, convert_now);
+	}
 
 	/*
 	 * CoW fork has a delalloc reservation. Replace it with a real extent.
 	 * There may or may not be a data fork mapping.
 	 */
+	ASSERT(!tp);
 	if (isnullstartblock(cmap->br_startblock) ||
 	    cmap->br_startblock == DELAYSTARTBLOCK)
 		return xfs_reflink_fill_delalloc(ip, imap, cmap, shared,
-- 
2.55.0

[PATCH 09/33] xfs: remove internal transaction allocation from xfs_reflink_fill_cow_hole

[Dave Chinner]

Now that xfs_reflink_fill_cow_hole() is always called with a valid
caller-supplied transaction, remove the dead internal transaction
allocation path.

This removes the ILOCK drop/reacquire cycle that was previously
needed to allocate a transaction, as the caller now handles
transaction allocation before calling into this function. The
COW extent allocation via xfs_bmapi_write() is now performed
entirely under the caller's ILOCK hold, eliminating the window
where the extent tree could change due to the lock being cycled.

The lockmode parameter is also removed as the function no longer
modifies the lock state.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_reflink.c | 75 +++-----------------------------------------
 1 file changed, 5 insertions(+), 70 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 56ad76b0f6b9..a3b4343fd882 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -435,75 +435,20 @@ xfs_reflink_fill_cow_hole(
 	struct xfs_bmbt_irec	*imap,
 	struct xfs_bmbt_irec	*cmap,
 	bool			*shared,
-	uint			*lockmode,
 	bool			convert_now)
 {
-	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_trans	*local_tp = NULL;
-	xfs_filblks_t		resaligned;
-	unsigned int		seq_before = READ_ONCE(ip->i_df.if_seq);
-	unsigned int		dblocks = 0, rblocks = 0;
 	int			nimaps;
 	int			error;
 	bool			found;
 
-	/*
-	 * If the caller supplied a transaction, use it directly. The caller
-	 * is responsible for commit/cancel and holds the ILOCK.
-	 *
-	 * Otherwise, we need to drop the ILOCK and allocate a transaction
-	 * ourselves, which will re-acquire the ILOCK.
-	 */
-	if (tp)
-		goto allocate;
-
-	resaligned = xfs_aligned_fsb_count(imap->br_startoff,
-		imap->br_blockcount, xfs_get_cowextsz_hint(ip));
-	if (XFS_IS_REALTIME_INODE(ip)) {
-		dblocks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
-		rblocks = resaligned;
-	} else {
-		dblocks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
-		rblocks = 0;
-	}
-
-	xfs_iunlock(ip, *lockmode);
-	*lockmode = 0;
-
-	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, dblocks,
-			rblocks, false, &local_tp);
-	if (error)
-		return error;
-
-	*lockmode = XFS_ILOCK_EXCL;
-	tp = local_tp;
-
-	/*
-	 * The data fork mapping may have changed while we dropped the ILOCK
-	 * (a racing O_DIRECT writer under IOLOCK_SHARED can complete a full
-	 * CoW cycle including xfs_reflink_end_cow(), which remaps this offset
-	 * and drops the refcount of the old shared block).  Re-read it so the
-	 * shared-status recheck below and the caller's in-place iomap both
-	 * operate on the current mapping rather than a stale physical block.
-	 */
-	if (seq_before != READ_ONCE(ip->i_df.if_seq)) {
-		nimaps = 1;
-		error = xfs_bmapi_read(ip, imap->br_startoff,
-				imap->br_blockcount, imap, &nimaps, 0);
-		if (error)
-			goto out_trans_cancel;
-	}
+	ASSERT(tp);
 
-allocate:
 	error = xfs_find_trim_cow_extent(ip, imap, cmap, shared, &found);
 	if (error || !*shared)
-		goto out_trans_cancel;
+		return error;
 
-	if (found) {
-		if (local_tp)
-			xfs_trans_cancel(local_tp);
+	if (found)
 		goto convert;
-	}
 
 	/* Allocate the entire reservation as unwritten blocks. */
 	nimaps = 1;
@@ -512,22 +457,12 @@ xfs_reflink_fill_cow_hole(
 			XFS_BMAPI_COWFORK | XFS_BMAPI_PREALLOC, 0, cmap,
 			&nimaps);
 	if (error)
-		goto out_trans_cancel;
+		return error;
 
 	xfs_inode_set_cowblocks_tag(ip);
-	if (local_tp) {
-		error = xfs_trans_commit(local_tp);
-		if (error)
-			return error;
-	}
 
 convert:
 	return xfs_reflink_convert_unwritten(ip, imap, cmap, convert_now);
-
-out_trans_cancel:
-	if (local_tp)
-		xfs_trans_cancel(local_tp);
-	return error;
 }
 
 static int
@@ -651,7 +586,7 @@ xfs_reflink_allocate_cow(
 		if (!tp)
 			return -EAGAIN;
 		return xfs_reflink_fill_cow_hole(tp, ip, imap, cmap, shared,
-				lockmode, convert_now);
+				convert_now);
 	}
 
 	/*
-- 
2.55.0

[PATCH 10/33] xfs: use zero-block transaction with xfs_trans_reserve_more_inode for COW holes

[Dave Chinner]

Change the COW transaction allocation strategy to use a zero-block
reservation at the top level and defer block reservation to the
callee that knows the actual extent state.

When xfs_reflink_allocate_cow() returns -EAGAIN, the ILOCK must be
dropped to allocate a transaction. Because the extent tree can change
while the ILOCK is not held, we cannot determine what extent type
will be found once we've regained the ILOCK. The callees will use
xfs_trans_reserve_more_inode() directly to reserve any blocks they
require before they start modifications. This allows ENOSPC to be
returned and the transaction cancelled safely if the block reservation
cannot be made.

In xfs_reflink_fill_cow_hole(), add an xfs_trans_reserve_more_inode()
call before xfs_bmapi_write() to reserve the blocks needed for the
COW extent allocation. The reservation is computed from the current
imap which is stable under the ILOCK. An ASSERT verifies the
transaction has not been dirtied, confirming it is safe to cancel
on ENOSPC.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_iomap.c   | 28 +++++++++++-----------------
 fs/xfs/xfs_reflink.c | 25 +++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 017355372a44..777048e6a2ca 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -919,29 +919,23 @@ xfs_direct_write_cow_iomap_begin(
 	if (error == -EAGAIN) {
 		/*
 		 * COW allocation needs a transaction. Drop the ILOCK and
-		 * allocate a transaction, which will re-acquire the ILOCK.
-		 * Then retry the imap lookup since the extent tree may have
-		 * changed while the ILOCK was not held.
+		 * allocate a zero-block reservation transaction, which will
+		 * re-acquire the ILOCK. We cannot determine what extent type
+		 * will be found once we've regained the ILOCK, so the callees
+		 * will use xfs_trans_reserve_more_inode() directly to reserve
+		 * any blocks they require before they start modifications.
+		 * This allows ENOSPC to be returned and the transaction
+		 * cancelled safely if the block reservation cannot be made.
+		 *
+		 * Retry the imap lookup since the extent tree may have changed
+		 * while the ILOCK was not held.
 		 */
-		xfs_filblks_t		resaligned;
-		unsigned int		dblocks, rblocks;
-
 		ASSERT(!tp);
 
 		xfs_iunlock(ip, *lockmode);
 
-		resaligned = xfs_aligned_fsb_count(offset_fsb,
-				end_fsb - offset_fsb, xfs_get_cowextsz_hint(ip));
-		if (XFS_IS_REALTIME_INODE(ip)) {
-			dblocks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
-			rblocks = resaligned;
-		} else {
-			dblocks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
-			rblocks = 0;
-		}
-
 		error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write,
-				dblocks, rblocks, false, &tp);
+				0, 0, false, &tp);
 		if (error)
 			return error;
 
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index a3b4343fd882..373ce9fea2a8 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -437,6 +437,9 @@ xfs_reflink_fill_cow_hole(
 	bool			*shared,
 	bool			convert_now)
 {
+	struct xfs_mount	*mp = ip->i_mount;
+	xfs_filblks_t		resaligned;
+	unsigned int		dblocks = 0, rblocks = 0;
 	int			nimaps;
 	int			error;
 	bool			found;
@@ -450,6 +453,28 @@ xfs_reflink_fill_cow_hole(
 	if (found)
 		goto convert;
 
+	/*
+	 * Reserve blocks for the COW extent allocation. The transaction was
+	 * allocated with a zero-block reservation because the caller could
+	 * not determine the block reservation required until the extent
+	 * state was known under the ILOCK. The transaction has not been
+	 * dirtied yet, so on ENOSPC it can safely be cancelled by the caller.
+	 */
+	ASSERT(!(tp->t_flags & XFS_TRANS_DIRTY));
+
+	resaligned = xfs_aligned_fsb_count(imap->br_startoff,
+			imap->br_blockcount, xfs_get_cowextsz_hint(ip));
+	if (XFS_IS_REALTIME_INODE(ip)) {
+		dblocks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
+		rblocks = resaligned;
+	} else {
+		dblocks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
+	}
+
+	error = xfs_trans_reserve_more_inode(tp, ip, dblocks, rblocks, false);
+	if (error)
+		return error;
+
 	/* Allocate the entire reservation as unwritten blocks. */
 	nimaps = 1;
 	error = xfs_bmapi_write(tp, ip, imap->br_startoff,
-- 
2.55.0

[PATCH 11/33] xfs: change *tp to **tpp in COW allocation call chain

[Dave Chinner]

Change the transaction parameter from struct xfs_trans *tp to
struct xfs_trans **tpp in xfs_reflink_fill_cow_hole(),
xfs_reflink_allocate_cow(), and the header declaration. Update
the caller in xfs_direct_write_cow_iomap_begin() to pass &tp.

This is preparation for xfs_reflink_fill_delalloc() using
xfs_trans_roll_inode() to implement rolling transactions, which
modifies the transaction pointer and requires **tpp semantics.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_iomap.c   |  2 +-
 fs/xfs/xfs_reflink.c | 19 ++++++++++---------
 fs/xfs/xfs_reflink.h |  2 +-
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 777048e6a2ca..2f18a8f62e39 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -913,7 +913,7 @@ xfs_direct_write_cow_iomap_begin(
 		goto out_unlock;
 
 	/* may drop and re-acquire the ilock */
-	error = xfs_reflink_allocate_cow(tp, ip, imap, &cmap, &shared,
+	error = xfs_reflink_allocate_cow(&tp, ip, imap, &cmap, &shared,
 			lockmode,
 			(flags & IOMAP_DIRECT) || IS_DAX(VFS_I(ip)));
 	if (error == -EAGAIN) {
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 373ce9fea2a8..6ca99ddf9976 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -430,7 +430,7 @@ xfs_reflink_convert_unwritten(
 
 static int
 xfs_reflink_fill_cow_hole(
-	struct xfs_trans	*tp,
+	struct xfs_trans	**tpp,
 	struct xfs_inode	*ip,
 	struct xfs_bmbt_irec	*imap,
 	struct xfs_bmbt_irec	*cmap,
@@ -444,7 +444,7 @@ xfs_reflink_fill_cow_hole(
 	int			error;
 	bool			found;
 
-	ASSERT(tp);
+	ASSERT(*tpp);
 
 	error = xfs_find_trim_cow_extent(ip, imap, cmap, shared, &found);
 	if (error || !*shared)
@@ -460,7 +460,7 @@ xfs_reflink_fill_cow_hole(
 	 * state was known under the ILOCK. The transaction has not been
 	 * dirtied yet, so on ENOSPC it can safely be cancelled by the caller.
 	 */
-	ASSERT(!(tp->t_flags & XFS_TRANS_DIRTY));
+	ASSERT(!((*tpp)->t_flags & XFS_TRANS_DIRTY));
 
 	resaligned = xfs_aligned_fsb_count(imap->br_startoff,
 			imap->br_blockcount, xfs_get_cowextsz_hint(ip));
@@ -471,13 +471,14 @@ xfs_reflink_fill_cow_hole(
 		dblocks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
 	}
 
-	error = xfs_trans_reserve_more_inode(tp, ip, dblocks, rblocks, false);
+	error = xfs_trans_reserve_more_inode(*tpp, ip, dblocks, rblocks,
+			false);
 	if (error)
 		return error;
 
 	/* Allocate the entire reservation as unwritten blocks. */
 	nimaps = 1;
-	error = xfs_bmapi_write(tp, ip, imap->br_startoff,
+	error = xfs_bmapi_write(*tpp, ip, imap->br_startoff,
 			imap->br_blockcount,
 			XFS_BMAPI_COWFORK | XFS_BMAPI_PREALLOC, 0, cmap,
 			&nimaps);
@@ -575,7 +576,7 @@ xfs_reflink_fill_delalloc(
 /* Allocate all CoW reservations covering a range of blocks in a file. */
 int
 xfs_reflink_allocate_cow(
-	struct xfs_trans	*tp,
+	struct xfs_trans	**tpp,
 	struct xfs_inode	*ip,
 	struct xfs_bmbt_irec	*imap,
 	struct xfs_bmbt_irec	*cmap,
@@ -608,9 +609,9 @@ xfs_reflink_allocate_cow(
 	 * tell the caller to allocate a transaction and retry.
 	 */
 	if (cmap->br_startoff > imap->br_startoff) {
-		if (!tp)
+		if (!*tpp)
 			return -EAGAIN;
-		return xfs_reflink_fill_cow_hole(tp, ip, imap, cmap, shared,
+		return xfs_reflink_fill_cow_hole(tpp, ip, imap, cmap, shared,
 				convert_now);
 	}
 
@@ -618,7 +619,7 @@ xfs_reflink_allocate_cow(
 	 * CoW fork has a delalloc reservation. Replace it with a real extent.
 	 * There may or may not be a data fork mapping.
 	 */
-	ASSERT(!tp);
+	ASSERT(!*tpp);
 	if (isnullstartblock(cmap->br_startblock) ||
 	    cmap->br_startblock == DELAYSTARTBLOCK)
 		return xfs_reflink_fill_delalloc(ip, imap, cmap, shared,
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index 6a984363edbc..3da8374829c3 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -30,7 +30,7 @@ int xfs_reflink_trim_around_shared(struct xfs_inode *ip,
 int xfs_bmap_trim_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap,
 		bool *shared);
 
-int xfs_reflink_allocate_cow(struct xfs_trans *tp, struct xfs_inode *ip,
+int xfs_reflink_allocate_cow(struct xfs_trans **tpp, struct xfs_inode *ip,
 		struct xfs_bmbt_irec *imap, struct xfs_bmbt_irec *cmap,
 		bool *shared, uint *lockmode, bool convert_now);
 extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset,
-- 
2.55.0

[PATCH 12/33] xfs: convert xfs_reflink_fill_delalloc to use rolling transactions

[Dave Chinner]

Convert xfs_reflink_fill_delalloc() to use xfs_defer_finish() to
process deferred operations and roll the transaction instead of
cycling the ILOCK with separate commit/alloc pairs for each delalloc
extent conversion.

The previous code dropped and reacquired the ILOCK for every
iteration of the delalloc conversion loop. This left a window where
a racing DIO write to the same range could complete its COW, have IO
completion move the extent from the COW fork to the data fork, and
leave a hole in the COW fork that the original writer would then
trip over.

Using xfs_defer_finish() keeps the ILOCK held across the entire
conversion loop and correctly processes deferred operations (such as
rmap updates) that are generated by xfs_bmapi_write() during each
iteration.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_reflink.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 6ca99ddf9976..3e8eb16ce076 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -493,6 +493,7 @@ xfs_reflink_fill_cow_hole(
 
 static int
 xfs_reflink_fill_delalloc(
+	struct xfs_trans	**tpp,
 	struct xfs_inode	*ip,
 	struct xfs_bmbt_irec	*imap,
 	struct xfs_bmbt_irec	*cmap,
@@ -501,12 +502,12 @@ xfs_reflink_fill_delalloc(
 	bool			convert_now)
 {
 	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_trans	*tp;
+	struct xfs_trans	*tp = *tpp;
 	int			nimaps;
 	int			error;
 	bool			found;
 
-	do {
+	if (!tp) {
 		unsigned int	seq_before = READ_ONCE(ip->i_df.if_seq);
 
 		xfs_iunlock(ip, *lockmode);
@@ -535,16 +536,16 @@ xfs_reflink_fill_delalloc(
 			if (error)
 				goto out_trans_cancel;
 		}
+	}
 
+	do {
 		error = xfs_find_trim_cow_extent(ip, imap, cmap, shared,
 				&found);
 		if (error || !*shared)
 			goto out_trans_cancel;
 
-		if (found) {
-			xfs_trans_cancel(tp);
+		if (found)
 			break;
-		}
 
 		ASSERT(isnullstartblock(cmap->br_startblock) ||
 		       cmap->br_startblock == DELAYSTARTBLOCK);
@@ -561,15 +562,27 @@ xfs_reflink_fill_delalloc(
 			goto out_trans_cancel;
 
 		xfs_inode_set_cowblocks_tag(ip);
+
+		error = xfs_defer_finish(&tp);
+		if (error)
+			goto out_trans_cancel;
+	} while (cmap->br_startoff + cmap->br_blockcount <= imap->br_startoff);
+
+	if (*tpp) {
+		*tpp = tp;
+	} else {
 		error = xfs_trans_commit(tp);
 		if (error)
 			return error;
-	} while (cmap->br_startoff + cmap->br_blockcount <= imap->br_startoff);
+	}
 
 	return xfs_reflink_convert_unwritten(ip, imap, cmap, convert_now);
 
 out_trans_cancel:
-	xfs_trans_cancel(tp);
+	if (*tpp)
+		*tpp = tp;
+	else
+		xfs_trans_cancel(tp);
 	return error;
 }
 
@@ -622,7 +635,7 @@ xfs_reflink_allocate_cow(
 	ASSERT(!*tpp);
 	if (isnullstartblock(cmap->br_startblock) ||
 	    cmap->br_startblock == DELAYSTARTBLOCK)
-		return xfs_reflink_fill_delalloc(ip, imap, cmap, shared,
+		return xfs_reflink_fill_delalloc(tpp, ip, imap, cmap, shared,
 				lockmode, convert_now);
 
 	/* Shouldn't get here. */
-- 
2.55.0

[PATCH 13/33] xfs: return -EAGAIN from xfs_reflink_allocate_cow for all allocation cases

[Dave Chinner]

Consolidate the -EAGAIN transaction check for both COW hole and
delalloc allocation paths. Both cases require a transaction to
proceed, so lift the *tpp == NULL check above both branches and
return -EAGAIN from a single point.

This activates the retry loop for the delalloc case, which was
previously handled by cycling the ILOCK internally in
xfs_reflink_fill_delalloc().

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_reflink.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 3e8eb16ce076..cc3c1caee2d8 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -616,23 +616,20 @@ xfs_reflink_allocate_cow(
 				convert_now);
 
 	/*
-	 * CoW fork does not have an extent and data extent is shared.
-	 * Allocate a real extent in the CoW fork. If the caller has not
-	 * provided a transaction for the allocation, return -EAGAIN to
-	 * tell the caller to allocate a transaction and retry.
+	 * Allocation is now required, so we need a transaction context from
+	 * the caller if it hasn't already supplied one.
 	 */
-	if (cmap->br_startoff > imap->br_startoff) {
-		if (!*tpp)
-			return -EAGAIN;
+	if (!*tpp)
+		return -EAGAIN;
+
+	if (cmap->br_startoff > imap->br_startoff)
 		return xfs_reflink_fill_cow_hole(tpp, ip, imap, cmap, shared,
 				convert_now);
-	}
 
 	/*
 	 * CoW fork has a delalloc reservation. Replace it with a real extent.
 	 * There may or may not be a data fork mapping.
 	 */
-	ASSERT(!*tpp);
 	if (isnullstartblock(cmap->br_startblock) ||
 	    cmap->br_startblock == DELAYSTARTBLOCK)
 		return xfs_reflink_fill_delalloc(tpp, ip, imap, cmap, shared,
-- 
2.55.0

[PATCH 14/33] xfs: remove dead internal transaction allocation from xfs_reflink_fill_delalloc

[Dave Chinner]

Now that xfs_reflink_fill_delalloc() is always called with a valid
caller-supplied transaction via the -EAGAIN retry mechanism, remove
the dead internal transaction allocation path.

This removes the ILOCK drop/reacquire cycle that was previously
needed to allocate a transaction. The delalloc conversion loop runs
entirely under the caller's ILOCK hold using rolling transactions,
eliminating the window where racing DIO writes could cause stale
extent state.

The lockmode parameter is also removed as the function no longer
modifies the lock state.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_reflink.c | 59 ++++++--------------------------------------
 1 file changed, 8 insertions(+), 51 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index cc3c1caee2d8..e40f7afc2c7b 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -498,51 +498,20 @@ xfs_reflink_fill_delalloc(
 	struct xfs_bmbt_irec	*imap,
 	struct xfs_bmbt_irec	*cmap,
 	bool			*shared,
-	uint			*lockmode,
 	bool			convert_now)
 {
-	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp = *tpp;
 	int			nimaps;
 	int			error;
 	bool			found;
 
-	if (!tp) {
-		unsigned int	seq_before = READ_ONCE(ip->i_df.if_seq);
-
-		xfs_iunlock(ip, *lockmode);
-		*lockmode = 0;
-
-		error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, 0, 0,
-				false, &tp);
-		if (error)
-			return error;
-
-		*lockmode = XFS_ILOCK_EXCL;
-
-		/*
-		 * The data fork mapping may have changed while we dropped the
-		 * ILOCK (a racing O_DIRECT writer under IOLOCK_SHARED can
-		 * complete a full CoW cycle including xfs_reflink_end_cow(),
-		 * which remaps this offset and drops the refcount of the old
-		 * shared block).  Re-read it so the shared-status recheck
-		 * below and the caller's in-place iomap both operate on the
-		 * current mapping rather than a stale physical block.
-		 */
-		if (seq_before != READ_ONCE(ip->i_df.if_seq)) {
-			nimaps = 1;
-			error = xfs_bmapi_read(ip, imap->br_startoff,
-					imap->br_blockcount, imap, &nimaps, 0);
-			if (error)
-				goto out_trans_cancel;
-		}
-	}
+	ASSERT(tp);
 
 	do {
 		error = xfs_find_trim_cow_extent(ip, imap, cmap, shared,
 				&found);
 		if (error || !*shared)
-			goto out_trans_cancel;
+			goto out_error;
 
 		if (found)
 			break;
@@ -559,30 +528,18 @@ xfs_reflink_fill_delalloc(
 				XFS_BMAPI_COWFORK | XFS_BMAPI_PREALLOC, 0,
 				cmap, &nimaps);
 		if (error)
-			goto out_trans_cancel;
+			goto out_error;
 
 		xfs_inode_set_cowblocks_tag(ip);
 
 		error = xfs_defer_finish(&tp);
 		if (error)
-			goto out_trans_cancel;
+			goto out_error;
 	} while (cmap->br_startoff + cmap->br_blockcount <= imap->br_startoff);
 
-	if (*tpp) {
-		*tpp = tp;
-	} else {
-		error = xfs_trans_commit(tp);
-		if (error)
-			return error;
-	}
-
-	return xfs_reflink_convert_unwritten(ip, imap, cmap, convert_now);
-
-out_trans_cancel:
-	if (*tpp)
-		*tpp = tp;
-	else
-		xfs_trans_cancel(tp);
+	error = xfs_reflink_convert_unwritten(ip, imap, cmap, convert_now);
+out_error:
+	*tpp = tp;
 	return error;
 }
 
@@ -633,7 +590,7 @@ xfs_reflink_allocate_cow(
 	if (isnullstartblock(cmap->br_startblock) ||
 	    cmap->br_startblock == DELAYSTARTBLOCK)
 		return xfs_reflink_fill_delalloc(tpp, ip, imap, cmap, shared,
-				lockmode, convert_now);
+				convert_now);
 
 	/* Shouldn't get here. */
 	ASSERT(0);
-- 
2.55.0

[PATCH 15/33] xfs: plumb struct xfs_trans *tp into xfs_bmapi_convert_one_delalloc

[Dave Chinner]

Add a struct xfs_trans *tp parameter to xfs_bmapi_convert_one_delalloc().
When a non-NULL transaction is provided, use it directly instead of
allocating one internally, and skip the ILOCK acquisition and
xfs_trans_ijoin() since the caller already holds the ILOCK with the
inode joined to the transaction.

On success with a caller-supplied transaction, the function returns
without committing or unlocking. On error, it returns the error
without cancelling or unlocking.

When tp is NULL, the existing behaviour is preserved.

All callers currently pass NULL, so there is no functional change.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/libxfs/xfs_bmap.c | 43 ++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 47c4d5c52b95..7f3a3a02a035 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4351,6 +4351,7 @@ xfs_bmapi_write(
  */
 static int
 xfs_bmapi_convert_one_delalloc(
+	struct xfs_trans	*tp,
 	struct xfs_inode	*ip,
 	int			whichfork,
 	xfs_off_t		offset,
@@ -4362,23 +4363,27 @@ xfs_bmapi_convert_one_delalloc(
 	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
 	struct xfs_bmalloca	bma = { NULL };
 	uint16_t		flags = 0;
-	struct xfs_trans	*tp;
+	struct xfs_trans	*local_tp = NULL;
 	int			error;
 
 	if (whichfork == XFS_COW_FORK)
 		flags |= IOMAP_F_SHARED;
 
-	/*
-	 * Space for the extent and indirect blocks was reserved when the
-	 * delalloc extent was created so there's no need to do so here.
-	 */
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, 0, 0,
-				XFS_TRANS_RESERVE, &tp);
-	if (error)
-		return error;
+	if (!tp) {
+		/*
+		 * Space for the extent and indirect blocks was reserved when
+		 * the delalloc extent was created so there's no need to do so
+		 * here.
+		 */
+		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, 0, 0,
+					XFS_TRANS_RESERVE, &local_tp);
+		if (error)
+			return error;
 
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	xfs_trans_ijoin(tp, ip, 0);
+		xfs_ilock(ip, XFS_ILOCK_EXCL);
+		xfs_trans_ijoin(local_tp, ip, 0);
+		tp = local_tp;
+	}
 
 	/*
 	 * Look up the extent before extending the extent count so that we
@@ -4470,15 +4475,19 @@ xfs_bmapi_convert_one_delalloc(
 		goto out_finish;
 
 	xfs_bmapi_finish(&bma, whichfork, 0);
-	error = xfs_trans_commit(tp);
-	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	if (local_tp) {
+		error = xfs_trans_commit(local_tp);
+		xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	}
 	return error;
 
 out_finish:
 	xfs_bmapi_finish(&bma, whichfork, error);
 out_trans_cancel:
-	xfs_trans_cancel(tp);
-	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	if (local_tp) {
+		xfs_trans_cancel(local_tp);
+		xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	}
 	return error;
 }
 
@@ -4503,8 +4512,8 @@ xfs_bmapi_convert_delalloc(
 	 * delalloc extent if free space is sufficiently fragmented.
 	 */
 	do {
-		error = xfs_bmapi_convert_one_delalloc(ip, whichfork, offset,
-					iomap, seq);
+		error = xfs_bmapi_convert_one_delalloc(NULL, ip, whichfork,
+					offset, iomap, seq);
 		if (error)
 			return error;
 	} while (iomap->offset + iomap->length <= offset);
-- 
2.55.0

[PATCH 16/33] xfs: use rolling transaction in xfs_bmapi_convert_delalloc

[Dave Chinner]

Move the transaction allocation, ILOCK acquisition and inode join
up from xfs_bmapi_convert_one_delalloc() into
xfs_bmapi_convert_delalloc(). Pass the transaction down to each
call to xfs_bmapi_convert_one_delalloc(), and use xfs_defer_finish()
after each iteration to process deferred operations and roll the
transaction while keeping the ILOCK held.

The rolling transaction keeps the ILOCK held across the entire
conversion loop, making this multi-extent conversion operation
atomic with respect to other concurrent extent operations.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/libxfs/xfs_bmap.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 7f3a3a02a035..4eaf67b51c8d 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4503,8 +4503,26 @@ xfs_bmapi_convert_delalloc(
 	struct iomap		*iomap,
 	unsigned int		*seq)
 {
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_trans	*tp;
 	int			error;
 
+	/*
+	 * Allocate the transaction, take the ILOCK and join the inode to it.
+	 * Space for the extent and indirect blocks was reserved when the
+	 * delalloc extent was created so there's no need to reserve blocks.
+	 * The rolling transaction keeps the ILOCK held across the entire
+	 * conversion loop, making this multi-extent conversion operation
+	 * atomic with respect to other concurrent extent operations.
+	 */
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, 0, 0,
+				XFS_TRANS_RESERVE, &tp);
+	if (error)
+		return error;
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, ip, 0);
+
 	/*
 	 * Attempt to allocate whatever delalloc extent currently backs offset
 	 * and put the result into iomap.  Allocate in a loop because it may
@@ -4512,13 +4530,24 @@ xfs_bmapi_convert_delalloc(
 	 * delalloc extent if free space is sufficiently fragmented.
 	 */
 	do {
-		error = xfs_bmapi_convert_one_delalloc(NULL, ip, whichfork,
+		error = xfs_bmapi_convert_one_delalloc(tp, ip, whichfork,
 					offset, iomap, seq);
 		if (error)
-			return error;
+			goto out_trans_cancel;
+
+		error = xfs_defer_finish(&tp);
+		if (error)
+			goto out_trans_cancel;
 	} while (iomap->offset + iomap->length <= offset);
 
-	return 0;
+	error = xfs_trans_commit(tp);
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	return error;
+
+out_trans_cancel:
+	xfs_trans_cancel(tp);
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	return error;
 }
 
 int
-- 
2.55.0

[PATCH 17/33] xfs: remove dead internal transaction path from xfs_bmapi_convert_one_delalloc

[Dave Chinner]

Now that xfs_bmapi_convert_one_delalloc() is always called with a
valid caller-supplied transaction from xfs_bmapi_convert_delalloc(),
remove the dead internal transaction allocation path.

The function is now a pure helper that converts one delalloc extent
using the caller's transaction and returns errors directly without
managing the transaction or lock lifecycle.

The success and error finish paths are collapsed into a single
out_finish label since both call xfs_bmapi_finish() and return error.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/libxfs/xfs_bmap.c | 41 +++++-----------------------------------
 1 file changed, 5 insertions(+), 36 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 4eaf67b51c8d..181333d3dee6 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4363,28 +4363,13 @@ xfs_bmapi_convert_one_delalloc(
 	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
 	struct xfs_bmalloca	bma = { NULL };
 	uint16_t		flags = 0;
-	struct xfs_trans	*local_tp = NULL;
 	int			error;
 
+	ASSERT(tp);
+
 	if (whichfork == XFS_COW_FORK)
 		flags |= IOMAP_F_SHARED;
 
-	if (!tp) {
-		/*
-		 * Space for the extent and indirect blocks was reserved when
-		 * the delalloc extent was created so there's no need to do so
-		 * here.
-		 */
-		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, 0, 0,
-					XFS_TRANS_RESERVE, &local_tp);
-		if (error)
-			return error;
-
-		xfs_ilock(ip, XFS_ILOCK_EXCL);
-		xfs_trans_ijoin(local_tp, ip, 0);
-		tp = local_tp;
-	}
-
 	/*
 	 * Look up the extent before extending the extent count so that we
 	 * don't dirty the transaction if there is nothing to convert. A
@@ -4398,8 +4383,7 @@ xfs_bmapi_convert_one_delalloc(
 		 * might have moved the extent to the data fork in the meantime.
 		 */
 		WARN_ON_ONCE(whichfork != XFS_COW_FORK);
-		error = -EAGAIN;
-		goto out_trans_cancel;
+		return -EAGAIN;
 	}
 
 	/*
@@ -4411,13 +4395,13 @@ xfs_bmapi_convert_one_delalloc(
 				xfs_iomap_inode_sequence(ip, flags));
 		if (seq)
 			*seq = READ_ONCE(ifp->if_seq);
-		goto out_trans_cancel;
+		return 0;
 	}
 
 	error = xfs_iext_count_extend(tp, ip, whichfork,
 			XFS_IEXT_ADD_NOSPLIT_CNT);
 	if (error)
-		goto out_trans_cancel;
+		return error;
 
 	bma.tp = tp;
 	bma.ip = ip;
@@ -4471,23 +4455,8 @@ xfs_bmapi_convert_one_delalloc(
 
 	error = xfs_bmap_btree_to_extents(tp, ip, bma.cur, &bma.logflags,
 			whichfork);
-	if (error)
-		goto out_finish;
-
-	xfs_bmapi_finish(&bma, whichfork, 0);
-	if (local_tp) {
-		error = xfs_trans_commit(local_tp);
-		xfs_iunlock(ip, XFS_ILOCK_EXCL);
-	}
-	return error;
-
 out_finish:
 	xfs_bmapi_finish(&bma, whichfork, error);
-out_trans_cancel:
-	if (local_tp) {
-		xfs_trans_cancel(local_tp);
-		xfs_iunlock(ip, XFS_ILOCK_EXCL);
-	}
 	return error;
 }
 
-- 
2.55.0

[PATCH 18/33] xfs: add block reservation renewal to xfs_defer_finish

[Dave Chinner]

Add infrastructure to automatically renew block and RT extent
reservations after deferred operations have been processed by
xfs_defer_finish().

Unlike log reservations which are renewed by xfs_log_regrant() on
every roll, block reservations are carried forward by
xfs_trans_dup() with consumed blocks subtracted. Over multiple
rolls, the reservation can be depleted even though each iteration
of the caller needs the same reservation.

Add a new XFS_TRANS_RENEW_BLKRES flag. When set, the original
block reservation is recorded in t_blk_res_orig (and t_rtx_res_orig
for RT extents) at transaction allocation time and propagated
through xfs_trans_dup(). After xfs_defer_finish() completes all
deferred operations and performs the final roll,
xfs_trans_regrant_blkres() reserves the deficit between
t_blk_res and t_blk_res_orig from the free space pool, restoring
the original reservation for the next iteration.

The regrant is not performed during the internal rolls in
xfs_defer_finish_noroll() because the original reservation already
contains all the space needed for the deferred op chain. The regrant
after the final roll operates on a clean transaction, so the caller
can safely cancel on ENOSPC without causing a filesystem shutdown.

xfs_defer_finish() is changed to unconditionally roll the transaction
after deferred op processing so that the regrant always occurs.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/libxfs/xfs_defer.c  | 38 +++++++++++++++++++++++++-----------
 fs/xfs/libxfs/xfs_shared.h |  3 +++
 fs/xfs/xfs_trans.c         | 40 +++++++++++++++++++++++++++++++++++++-
 fs/xfs/xfs_trans.h         |  3 +++
 4 files changed, 72 insertions(+), 12 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 89501e8bd2f8..f1807dadafb2 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -725,6 +725,24 @@ xfs_defer_finish_noroll(
 	return error;
 }
 
+/*
+ * Finish all deferred ops and roll the transaction. The transaction is
+ * always rolled unconditionally so that the block reservation can be
+ * regranted after all deferred ops have completed.
+ *
+ * The block reservation regrant is not performed during the internal
+ * transaction rolls in xfs_defer_finish_noroll() because the original
+ * reservation already contains all the space needed for the deferred op
+ * chain. Regranting during internal rolls would risk unnecessary ENOSPC
+ * errors in the middle of a deferred op chain that cannot be safely
+ * aborted.
+ *
+ * The regrant is done after the final roll when the transaction is clean,
+ * replenishing whatever blocks were consumed by both the caller's
+ * modifications and the deferred operations. If the regrant fails with
+ * ENOSPC, the caller can safely cancel the clean transaction without
+ * causing a filesystem shutdown.
+ */
 int
 xfs_defer_finish(
 	struct xfs_trans	**tp)
@@ -734,22 +752,20 @@ xfs_defer_finish(
 #endif
 	int			error;
 
-	/*
-	 * Finish and roll the transaction once more to avoid returning to the
-	 * caller with a dirty transaction.
-	 */
 	error = xfs_defer_finish_noroll(tp);
 	if (error)
 		return error;
-	if ((*tp)->t_flags & XFS_TRANS_DIRTY) {
-		error = xfs_defer_trans_roll(tp);
-		if (error) {
-			xfs_force_shutdown((*tp)->t_mountp,
-					   SHUTDOWN_CORRUPT_INCORE);
-			return error;
-		}
+
+	error = xfs_defer_trans_roll(tp);
+	if (error) {
+		xfs_force_shutdown((*tp)->t_mountp, SHUTDOWN_CORRUPT_INCORE);
+		return error;
 	}
 
+	error = xfs_trans_regrant_blkres(*tp);
+	if (error)
+		return error;
+
 	/* Reset LOWMODE now that we've finished all the dfops. */
 #ifdef DEBUG
 	list_for_each_entry(dfp, &(*tp)->t_dfops, dfp_list)
diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
index b1e0d9bc1f7d..e3909d46f3cd 100644
--- a/fs/xfs/libxfs/xfs_shared.h
+++ b/fs/xfs/libxfs/xfs_shared.h
@@ -164,6 +164,9 @@ void	xfs_log_get_max_trans_res(struct xfs_mount *mp,
 /* Transaction has locked the rtbitmap and rtsum inodes */
 #define XFS_TRANS_RTBITMAP_LOCKED	(1u << 9)
 
+/* Renew block reservation on transaction roll */
+#define XFS_TRANS_RENEW_BLKRES		(1u << 10)
+
 /*
  * Field values for xfs_trans_mod_sb.
  */
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 7bfbd9f6f0df..c267a174cbe1 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -112,16 +112,19 @@ xfs_trans_dup(
 	ntp->t_flags = XFS_TRANS_PERM_LOG_RES |
 		       (tp->t_flags & XFS_TRANS_RESERVE) |
 		       (tp->t_flags & XFS_TRANS_NO_WRITECOUNT) |
-		       (tp->t_flags & XFS_TRANS_RES_FDBLKS);
+		       (tp->t_flags & XFS_TRANS_RES_FDBLKS) |
+		       (tp->t_flags & XFS_TRANS_RENEW_BLKRES);
 	/* We gave our writer reference to the new transaction */
 	tp->t_flags |= XFS_TRANS_NO_WRITECOUNT;
 	ntp->t_ticket = xfs_log_ticket_get(tp->t_ticket);
 
 	ASSERT(tp->t_blk_res >= tp->t_blk_res_used);
 	ntp->t_blk_res = tp->t_blk_res - tp->t_blk_res_used;
+	ntp->t_blk_res_orig = tp->t_blk_res_orig;
 	tp->t_blk_res = tp->t_blk_res_used;
 
 	ntp->t_rtx_res = tp->t_rtx_res - tp->t_rtx_res_used;
+	ntp->t_rtx_res_orig = tp->t_rtx_res_orig;
 	tp->t_rtx_res = tp->t_rtx_res_used;
 
 	/* move deferred ops over to the new tp */
@@ -164,6 +167,7 @@ xfs_trans_reserve(
 		if (error != 0)
 			return -ENOSPC;
 		tp->t_blk_res += blocks;
+		tp->t_blk_res_orig = tp->t_blk_res;
 	}
 
 	/*
@@ -191,6 +195,7 @@ xfs_trans_reserve(
 			goto undo_log;
 		}
 		tp->t_rtx_res += rtextents;
+		tp->t_rtx_res_orig = tp->t_rtx_res;
 	}
 
 	return 0;
@@ -995,6 +1000,39 @@ xfs_trans_cancel(
 	xfs_trans_free(tp);
 }
 
+/*
+ * Renew the block and RT extent reservations from the free space pool.
+ * The consumed counts were carried forward by xfs_trans_dup() so we know
+ * exactly how many blocks need to be reserved to restore the original
+ * reservation.
+ */
+int
+xfs_trans_regrant_blkres(
+	struct xfs_trans	*tp)
+{
+	int			error;
+
+	if (!(tp->t_flags & XFS_TRANS_RENEW_BLKRES))
+		return 0;
+
+	if (tp->t_blk_res != tp->t_blk_res_orig) {
+		error = xfs_dec_fdblocks(tp->t_mountp,
+				tp->t_blk_res_orig - tp->t_blk_res,
+				tp->t_flags & XFS_TRANS_RESERVE);
+		if (error)
+			return error;
+		tp->t_blk_res = tp->t_blk_res_orig;
+	}
+	if (tp->t_rtx_res != tp->t_rtx_res_orig) {
+		error = xfs_dec_frextents(tp->t_mountp,
+				tp->t_rtx_res_orig - tp->t_rtx_res);
+		if (error)
+			return error;
+		tp->t_rtx_res = tp->t_rtx_res_orig;
+	}
+	return 0;
+}
+
 /*
  * Roll from one trans in the sequence of PERMANENT transactions to the next:
  * permanent transactions are only flushed out when committed with
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index eb83c5dac032..3d1ddc2a90f7 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -127,8 +127,10 @@ typedef struct xfs_trans {
 	unsigned int		t_log_count;	/* count for perm log res */
 	unsigned int		t_blk_res;	/* # of blocks resvd */
 	unsigned int		t_blk_res_used;	/* # of resvd blocks used */
+	unsigned int		t_blk_res_orig;	/* used with XFS_TRANS_RENEW_BLKRES */
 	unsigned int		t_rtx_res;	/* # of rt extents resvd */
 	unsigned int		t_rtx_res_used;	/* # of resvd rt extents used */
+	unsigned int		t_rtx_res_orig;	/* used with XFS_TRANS_RENEW_BLKRES */
 	unsigned int		t_flags;	/* misc flags */
 	xfs_agnumber_t		t_highest_agno;	/* highest AGF locked */
 	struct xlog_ticket	*t_ticket;	/* log mgr ticket */
@@ -237,6 +239,7 @@ void		xfs_trans_log_inode(xfs_trans_t *, struct xfs_inode *, uint);
 int		xfs_trans_commit(struct xfs_trans *);
 int		xfs_trans_roll(struct xfs_trans **);
 int		xfs_trans_roll_inode(struct xfs_trans **, struct xfs_inode *);
+int		xfs_trans_regrant_blkres(struct xfs_trans *);
 void		xfs_trans_cancel(xfs_trans_t *);
 int		xfs_trans_ail_init(struct xfs_mount *);
 void		xfs_trans_ail_destroy(struct xfs_mount *);
-- 
2.55.0

[PATCH 19/33] xfs: factor out xfs_iomap_write_unwritten_one helper

[Dave Chinner]

Extract the per-iteration unwritten extent conversion work from
xfs_iomap_write_unwritten() into a new helper function
xfs_iomap_write_unwritten_one().

The helper takes a caller-supplied transaction and performs the
extent count extension, unwritten-to-written conversion via
xfs_bmapi_write(), and inode size update for a single extent.

The startblock validation check remains in the caller after the
transaction commit, as it is a corruption detection check that
should warn and return an error without causing a filesystem
shutdown from cancelling a dirty transaction.

This is a pure refactoring with no functional change, done to
prepare for converting the loop to use rolling transactions.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_iomap.c | 103 +++++++++++++++++++++++++++------------------
 1 file changed, 61 insertions(+), 42 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 2f18a8f62e39..16feac3ad3bd 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -609,6 +609,59 @@ xfs_iomap_prealloc_size(
 	return alloc_blocks;
 }
 
+/*
+ * Convert a single unwritten extent to a real extent using the supplied
+ * transaction. Returns the converted extent via @imap.
+ */
+static int
+xfs_iomap_write_unwritten_one(
+	struct xfs_trans	*tp,
+	struct xfs_inode	*ip,
+	xfs_fileoff_t		offset_fsb,
+	xfs_filblks_t		count_fsb,
+	xfs_off_t		end,
+	bool			update_isize,
+	uint			resblks,
+	struct xfs_bmbt_irec	*imap)
+{
+	xfs_fsize_t		i_size;
+	int			nimaps;
+	int			error;
+
+	error = xfs_iext_count_extend(tp, ip, XFS_DATA_FORK,
+			XFS_IEXT_WRITE_UNWRITTEN_CNT);
+	if (error)
+		return error;
+
+	nimaps = 1;
+	error = xfs_bmapi_write(tp, ip, offset_fsb, count_fsb,
+				XFS_BMAPI_CONVERT, resblks, imap, &nimaps);
+	if (error)
+		return error;
+
+	/*
+	 * Update the inode size to reflect the extent that was converted in
+	 * this iteration. We must not advance isize beyond the extent we just
+	 * converted, otherwise a crash before the next conversion exposes
+	 * unwritten extents (zeroes) to userspace instead of the written data.
+	 * Clamp to the byte-level write end in case the converted extent
+	 * extends past the write boundary.
+	 */
+	i_size = XFS_FSB_TO_B(ip->i_mount,
+			imap->br_startoff + imap->br_blockcount);
+	if (i_size > end)
+		i_size = end;
+	if (update_isize && i_size > i_size_read(VFS_I(ip)))
+		i_size_write(VFS_I(ip), i_size);
+	i_size = xfs_new_eof(ip, i_size);
+	if (i_size) {
+		ip->i_disk_size = i_size;
+		xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+	}
+
+	return 0;
+}
+
 int
 xfs_iomap_write_unwritten(
 	xfs_inode_t	*ip,
@@ -617,21 +670,19 @@ xfs_iomap_write_unwritten(
 	bool		update_isize)
 {
 	xfs_mount_t	*mp = ip->i_mount;
+	xfs_off_t	end = offset + count;
 	xfs_fileoff_t	offset_fsb;
 	xfs_filblks_t	count_fsb;
 	xfs_filblks_t	numblks_fsb;
-	int		nimaps;
 	xfs_trans_t	*tp;
 	xfs_bmbt_irec_t imap;
-	struct inode	*inode = VFS_I(ip);
-	xfs_fsize_t	i_size;
 	uint		resblks;
 	int		error;
 
 	trace_xfs_unwritten_convert(ip, offset, count);
 
 	offset_fsb = XFS_B_TO_FSBT(mp, offset);
-	count_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)offset + count);
+	count_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)end);
 	count_fsb = (xfs_filblks_t)(count_fsb - offset_fsb);
 
 	/*
@@ -666,39 +717,12 @@ xfs_iomap_write_unwritten(
 		if (error)
 			return error;
 
-		error = xfs_iext_count_extend(tp, ip, XFS_DATA_FORK,
-				XFS_IEXT_WRITE_UNWRITTEN_CNT);
-		if (error)
-			goto error_on_bmapi_transaction;
-
-		/*
-		 * Modify the unwritten extent state of the buffer.
-		 */
-		nimaps = 1;
-		error = xfs_bmapi_write(tp, ip, offset_fsb, count_fsb,
-					XFS_BMAPI_CONVERT, resblks, &imap,
-					&nimaps);
-		if (error)
-			goto error_on_bmapi_transaction;
-
-		/*
-		 * Update the inode size to reflect the extent that was
-		 * converted in this iteration. We must not advance isize
-		 * beyond the extent we just converted, otherwise a crash
-		 * before the next conversion exposes unwritten extents
-		 * (zeroes) to userspace instead of the written data.
-		 * Clamp to the byte-level write end in case the converted
-		 * extent extends past the write boundary.
-		 */
-		i_size = XFS_FSB_TO_B(mp, imap.br_startoff + imap.br_blockcount);
-		if (i_size > offset + count)
-			i_size = offset + count;
-		if (update_isize && i_size > i_size_read(inode))
-			i_size_write(inode, i_size);
-		i_size = xfs_new_eof(ip, i_size);
-		if (i_size) {
-			ip->i_disk_size = i_size;
-			xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+		error = xfs_iomap_write_unwritten_one(tp, ip, offset_fsb,
+				count_fsb, end, update_isize, resblks, &imap);
+		if (error) {
+			xfs_trans_cancel(tp);
+			xfs_iunlock(ip, XFS_ILOCK_EXCL);
+			return error;
 		}
 
 		error = xfs_trans_commit(tp);
@@ -724,11 +748,6 @@ xfs_iomap_write_unwritten(
 	} while (count_fsb > 0);
 
 	return 0;
-
-error_on_bmapi_transaction:
-	xfs_trans_cancel(tp);
-	xfs_iunlock(ip, XFS_ILOCK_EXCL);
-	return error;
 }
 
 static inline bool
-- 
2.55.0

[PATCH 20/33] xfs: convert xfs_iomap_write_unwritten to rolling transactions

[Dave Chinner]

Convert xfs_iomap_write_unwritten() from per-iteration transaction
allocation to a single rolling transaction that keeps the ILOCK
held across the entire unwritten extent conversion loop.

Previously each iteration allocated a new transaction, acquired the
ILOCK, converted one extent, committed, and released the ILOCK.
This left a window where the extent tree could change between
iterations.

The rolling transaction keeps the ILOCK held throughout, making the
multi-extent conversion atomic with respect to other concurrent
extent operations. XFS_TRANS_RENEW_BLKRES is set so that the btree
split block reservation is automatically renewed after each
xfs_defer_finish() call.

The startblock corruption check is performed after xfs_defer_finish()
commits the dirty transaction, so that corruption detection warns and
returns an error without causing a filesystem shutdown from cancelling
a dirty transaction.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_iomap.c | 71 +++++++++++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 29 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 16feac3ad3bd..e43ce4811283 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -19,6 +19,7 @@
 #include "xfs_errortag.h"
 #include "xfs_error.h"
 #include "xfs_trans.h"
+#include "xfs_defer.h"
 #include "xfs_trans_space.h"
 #include "xfs_inode_item.h"
 #include "xfs_iomap.h"
@@ -702,44 +703,49 @@ xfs_iomap_write_unwritten(
 	if (error)
 		return error;
 
-	do {
-		/*
-		 * Set up a transaction to convert the range of extents
-		 * from unwritten to real. Do allocations in a loop until
-		 * we have covered the range passed in.
-		 *
-		 * Note that we can't risk to recursing back into the filesystem
-		 * here as we might be asked to write out the same inode that we
-		 * complete here and might deadlock on the iolock.
-		 */
-		error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, resblks,
-				0, true, &tp);
-		if (error)
-			return error;
+	/*
+	 * Allocate the transaction once and use a rolling transaction to
+	 * convert the range of extents from unwritten to real. The rolling
+	 * transaction keeps the ILOCK held across the entire conversion,
+	 * making it atomic with respect to other concurrent extent
+	 * operations. The block reservation is automatically renewed on
+	 * each roll by XFS_TRANS_RENEW_BLKRES.
+	 *
+	 * Note that we can't risk recursing back into the filesystem here
+	 * as we might be asked to write out the same inode that we complete
+	 * here and might deadlock on the iolock.
+	 */
+	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, resblks,
+			0, true, &tp);
+	if (error)
+		return error;
+	tp->t_flags |= XFS_TRANS_RENEW_BLKRES;
 
+	do {
 		error = xfs_iomap_write_unwritten_one(tp, ip, offset_fsb,
 				count_fsb, end, update_isize, resblks, &imap);
-		if (error) {
-			xfs_trans_cancel(tp);
-			xfs_iunlock(ip, XFS_ILOCK_EXCL);
-			return error;
-		}
+		if (error)
+			goto out_cancel;
 
-		error = xfs_trans_commit(tp);
-		xfs_iunlock(ip, XFS_ILOCK_EXCL);
+		/*
+		 * Roll the transaction to commit the conversion and renew
+		 * the block reservation. The startblock validation must be
+		 * done after the roll commits the transaction, so that a
+		 * corruption detection does not cancel a dirty transaction
+		 * and shut down the filesystem.
+		 */
+		error = xfs_defer_finish(&tp);
 		if (error)
-			return error;
+			goto out_cancel;
 
 		if (unlikely(!xfs_valid_startblock(ip, imap.br_startblock))) {
 			xfs_bmap_mark_sick(ip, XFS_DATA_FORK);
-			return xfs_alert_fsblock_zero(ip, &imap);
+			error = xfs_alert_fsblock_zero(ip, &imap);
+			goto out_cancel;
 		}
 
-		if ((numblks_fsb = imap.br_blockcount) == 0) {
-			/*
-			 * The numblks_fsb value should always get
-			 * smaller, otherwise the loop is stuck.
-			 */
+		numblks_fsb = imap.br_blockcount;
+		if (numblks_fsb == 0) {
 			ASSERT(imap.br_blockcount);
 			break;
 		}
@@ -747,7 +753,14 @@ xfs_iomap_write_unwritten(
 		count_fsb -= numblks_fsb;
 	} while (count_fsb > 0);
 
-	return 0;
+	error = xfs_trans_commit(tp);
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	return error;
+
+out_cancel:
+	xfs_trans_cancel(tp);
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	return error;
 }
 
 static inline bool
-- 
2.55.0

[PATCH 21/33] xfs: plumb struct xfs_trans *tp into xfs_reflink_end_cow_extent

[Dave Chinner]

Add a struct xfs_trans *tp parameter to xfs_reflink_end_cow_extent().
When a non-NULL transaction is provided, use it directly for the
COW extent remapping and return the error to the caller without
managing the transaction or lock lifecycle.

When tp is NULL, the existing behaviour is preserved.

All callers currently pass NULL, so there is no functional change.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_reflink.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index e40f7afc2c7b..b175a549ee55 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -889,15 +889,19 @@ xfs_reflink_end_cow_extent_locked(
  */
 STATIC int
 xfs_reflink_end_cow_extent(
+	struct xfs_trans	*tp,
 	struct xfs_inode	*ip,
 	xfs_fileoff_t		*offset_fsb,
 	xfs_fileoff_t		end_fsb)
 {
 	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_trans	*tp;
 	unsigned int		resblks;
+	bool			local_tp = false;
 	int			error;
 
+	if (tp)
+		goto end_cow;
+
 	resblks = XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK);
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0,
 			XFS_TRANS_RESERVE, &tp);
@@ -905,8 +909,12 @@ xfs_reflink_end_cow_extent(
 		return error;
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, 0);
+	local_tp = true;
 
+end_cow:
 	error = xfs_reflink_end_cow_extent_locked(tp, ip, offset_fsb, end_fsb);
+	if (!local_tp)
+		return error;
 	if (error)
 		xfs_trans_cancel(tp);
 	else
@@ -966,7 +974,8 @@ xfs_reflink_end_cow(
 	 * blocks will be remapped.
 	 */
 	while (end_fsb > offset_fsb && !error)
-		error = xfs_reflink_end_cow_extent(ip, &offset_fsb, end_fsb);
+		error = xfs_reflink_end_cow_extent(NULL, ip, &offset_fsb,
+				end_fsb);
 
 	if (error)
 		trace_xfs_reflink_end_cow_error(ip, error, _RET_IP_);
-- 
2.55.0

[PATCH 22/33] xfs: convert xfs_reflink_end_cow to rolling transactions

[Dave Chinner]

Convert xfs_reflink_end_cow() from per-extent transaction allocation
to a single rolling transaction that keeps the ILOCK held across the
entire COW remapping loop.

The rolling transaction keeps the ILOCK held throughout, making the
COW remapping operation atomic with respect to other ILOCK-protected
extent manipulations such as truncate, reflink remapping, and other
concurrent end_cow operations on overlapping regions.

XFS_TRANS_RENEW_BLKRES is set so that the btree split block
reservation is automatically renewed by xfs_defer_finish() after
each iteration's deferred operations are processed.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_reflink.c | 75 ++++++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 37 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index b175a549ee55..7e62f05499be 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -925,6 +925,13 @@ xfs_reflink_end_cow_extent(
 
 /*
  * Remap parts of a file's data fork after a successful CoW.
+ *
+ * The rolling transaction keeps the ILOCK held across the entire remapping
+ * loop, making the operation atomic with respect to other ILOCK-protected
+ * extent manipulations such as truncate, reflink remapping, and other
+ * concurrent end_cow operations on overlapping regions.
+ * XFS_TRANS_RENEW_BLKRES ensures the btree split block reservation is
+ * renewed after each xfs_defer_finish() call.
  */
 int
 xfs_reflink_end_cow(
@@ -932,54 +939,48 @@ xfs_reflink_end_cow(
 	xfs_off_t			offset,
 	xfs_off_t			count)
 {
+	struct xfs_mount		*mp = ip->i_mount;
 	xfs_fileoff_t			offset_fsb;
 	xfs_fileoff_t			end_fsb;
-	int				error = 0;
+	struct xfs_trans		*tp;
+	unsigned int			resblks;
+	int				error;
 
 	trace_xfs_reflink_end_cow(ip, offset, count);
 
-	offset_fsb = XFS_B_TO_FSBT(ip->i_mount, offset);
-	end_fsb = XFS_B_TO_FSB(ip->i_mount, offset + count);
+	offset_fsb = XFS_B_TO_FSBT(mp, offset);
+	end_fsb = XFS_B_TO_FSB(mp, offset + count);
 
-	/*
-	 * Walk forwards until we've remapped the I/O range.  The loop function
-	 * repeatedly cycles the ILOCK to allocate one transaction per remapped
-	 * extent.
-	 *
-	 * If we're being called by writeback then the folios will still
-	 * have the writeback flag set, which prevents races with reflink
-	 * remapping and truncate.  Reflink remapping prevents races with
-	 * writeback by taking the iolock and mmaplock before flushing
-	 * the folios and remapping, which means there won't be any further
-	 * writeback or page cache dirtying until the reflink completes.
-	 *
-	 * We should never have two threads issuing writeback for the same file
-	 * region.  There are also have post-eof checks in the writeback
-	 * preparation code so that we don't bother writing out folios that are
-	 * about to be truncated.
-	 *
-	 * If we're being called as part of directio write completion, the dio
-	 * count is still elevated, which reflink and truncate will wait for.
-	 * Reflink remapping takes the iolock and mmaplock and waits for
-	 * pending dio to finish, which should prevent any directio until the
-	 * remap completes.  Multiple concurrent directio writes to the same
-	 * region are handled by end_cow processing only occurring for the
-	 * threads which succeed; the outcome of multiple overlapping direct
-	 * writes is not well defined anyway.
-	 *
-	 * It's possible that a buffered write and a direct write could collide
-	 * here (the buffered write stumbles in after the dio flushes and
-	 * invalidates the page cache and immediately queues writeback), but we
-	 * have never supported this 100%.  If either disk write succeeds the
-	 * blocks will be remapped.
-	 */
-	while (end_fsb > offset_fsb && !error)
-		error = xfs_reflink_end_cow_extent(NULL, ip, &offset_fsb,
+	resblks = XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK);
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0,
+			XFS_TRANS_RESERVE | XFS_TRANS_RENEW_BLKRES, &tp);
+	if (error)
+		return error;
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, ip, 0);
+
+	while (end_fsb > offset_fsb) {
+		error = xfs_reflink_end_cow_extent(tp, ip, &offset_fsb,
 				end_fsb);
+		if (error)
+			goto out_cancel;
+
+		error = xfs_defer_finish(&tp);
+		if (error)
+			goto out_cancel;
+	}
 
+	error = xfs_trans_commit(tp);
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	if (error)
 		trace_xfs_reflink_end_cow_error(ip, error, _RET_IP_);
 	return error;
+
+out_cancel:
+	xfs_trans_cancel(tp);
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	trace_xfs_reflink_end_cow_error(ip, error, _RET_IP_);
+	return error;
 }
 
 /*
-- 
2.55.0

[PATCH 23/33] xfs: remove xfs_reflink_end_cow_extent wrapper and rename locked variant

[Dave Chinner]

Now that xfs_reflink_end_cow() calls xfs_reflink_end_cow_extent_locked()
directly with its rolling transaction, the xfs_reflink_end_cow_extent()
wrapper function is no longer needed. Remove it entirely and rename
xfs_reflink_end_cow_extent_locked() to xfs_reflink_end_cow_extent()
since there is no longer an unlocked variant.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_reflink.c | 40 ++--------------------------------------
 1 file changed, 2 insertions(+), 38 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 7e62f05499be..597300f39611 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -775,7 +775,7 @@ xfs_reflink_update_quota(
  * requirements as low as possible.
  */
 STATIC int
-xfs_reflink_end_cow_extent_locked(
+xfs_reflink_end_cow_extent(
 	struct xfs_trans	*tp,
 	struct xfs_inode	*ip,
 	xfs_fileoff_t		*offset_fsb,
@@ -887,42 +887,6 @@ xfs_reflink_end_cow_extent_locked(
  * every remap operation and we'd like to keep the block reservation
  * requirements as low as possible.
  */
-STATIC int
-xfs_reflink_end_cow_extent(
-	struct xfs_trans	*tp,
-	struct xfs_inode	*ip,
-	xfs_fileoff_t		*offset_fsb,
-	xfs_fileoff_t		end_fsb)
-{
-	struct xfs_mount	*mp = ip->i_mount;
-	unsigned int		resblks;
-	bool			local_tp = false;
-	int			error;
-
-	if (tp)
-		goto end_cow;
-
-	resblks = XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK);
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0,
-			XFS_TRANS_RESERVE, &tp);
-	if (error)
-		return error;
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	xfs_trans_ijoin(tp, ip, 0);
-	local_tp = true;
-
-end_cow:
-	error = xfs_reflink_end_cow_extent_locked(tp, ip, offset_fsb, end_fsb);
-	if (!local_tp)
-		return error;
-	if (error)
-		xfs_trans_cancel(tp);
-	else
-		error = xfs_trans_commit(tp);
-	xfs_iunlock(ip, XFS_ILOCK_EXCL);
-	return error;
-}
-
 /*
  * Remap parts of a file's data fork after a successful CoW.
  *
@@ -1023,7 +987,7 @@ xfs_reflink_end_atomic_cow(
 	xfs_trans_ijoin(tp, ip, 0);
 
 	while (end_fsb > offset_fsb && !error) {
-		error = xfs_reflink_end_cow_extent_locked(tp, ip, &offset_fsb,
+		error = xfs_reflink_end_cow_extent(tp, ip, &offset_fsb,
 				end_fsb);
 	}
 	if (error) {
-- 
2.55.0

[PATCH 24/33] xfs: convert xfs_zoned_end_io to rolling transactions

[Dave Chinner]

Convert xfs_zoned_end_io() from per-extent transaction allocation
to a single rolling transaction that keeps the ILOCK held across
the entire extent mapping loop.

The rolling transaction keeps the ILOCK held throughout, making the
zoned IO completion operation atomic with respect to other concurrent
extent manipulations. XFS_TRANS_RENEW_BLKRES is set so that the
block reservation is automatically renewed by xfs_defer_finish()
after each iteration's deferred operations are processed.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_zone_alloc.c | 40 +++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/fs/xfs/xfs_zone_alloc.c b/fs/xfs/xfs_zone_alloc.c
index 7e3456ca28ac..d24afffe255f 100644
--- a/fs/xfs/xfs_zone_alloc.c
+++ b/fs/xfs/xfs_zone_alloc.c
@@ -13,6 +13,7 @@
 #include "xfs_inode.h"
 #include "xfs_iomap.h"
 #include "xfs_trans.h"
+#include "xfs_defer.h"
 #include "xfs_alloc.h"
 #include "xfs_bmap.h"
 #include "xfs_bmap_btree.h"
@@ -314,6 +315,13 @@ xfs_zoned_map_extent(
 	return 0;
 }
 
+/*
+ * Map written extents into the data fork after zoned IO completion.
+ *
+ * The rolling transaction keeps the ILOCK held across the entire mapping
+ * loop, making the operation atomic with respect to other concurrent
+ * extent manipulations.
+ */
 int
 xfs_zoned_end_io(
 	struct xfs_inode	*ip,
@@ -337,24 +345,23 @@ xfs_zoned_end_io(
 	if (xfs_is_shutdown(mp))
 		return -EIO;
 
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0,
+			XFS_TRANS_RESERVE | XFS_TRANS_RENEW_BLKRES, &tp);
+	if (error)
+		return error;
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, ip, 0);
+
 	while (new.br_startoff < end_fsb) {
 		new.br_blockcount = end_fsb - new.br_startoff;
 
-		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0,
-				XFS_TRANS_RESERVE, &tp);
-		if (error)
-			return error;
-		xfs_ilock(ip, XFS_ILOCK_EXCL);
-		xfs_trans_ijoin(tp, ip, 0);
-
 		error = xfs_zoned_map_extent(tp, ip, &new, oz, old_startblock);
 		if (error)
-			xfs_trans_cancel(tp);
-		else
-			error = xfs_trans_commit(tp);
-		xfs_iunlock(ip, XFS_ILOCK_EXCL);
+			goto out_cancel;
+
+		error = xfs_defer_finish(&tp);
 		if (error)
-			return error;
+			goto out_cancel;
 
 		new.br_startoff += new.br_blockcount;
 		new.br_startblock += new.br_blockcount;
@@ -362,7 +369,14 @@ xfs_zoned_end_io(
 			old_startblock += new.br_blockcount;
 	}
 
-	return 0;
+	error = xfs_trans_commit(tp);
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	return error;
+
+out_cancel:
+	xfs_trans_cancel(tp);
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	return error;
 }
 
 /*
-- 
2.55.0

[PATCH 25/33] xfs: plumb struct xfs_trans *tp into xfs_iomap_write_direct

[Dave Chinner]

Add a struct xfs_trans *tp parameter to xfs_iomap_write_direct().
When a non-NULL transaction is provided, use it directly for the
allocation instead of allocating one internally. The block
reservation is added to the caller's transaction via
xfs_trans_reserve_more_inode().

On success with a caller-supplied transaction, the function returns
without committing or unlocking. On error, it returns the error
without cancelling.

When tp is NULL, the existing behaviour is preserved.

All callers currently pass NULL, so there is no functional change.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_iomap.c | 41 ++++++++++++++++++++++++++++-------------
 fs/xfs/xfs_iomap.h |  7 ++++---
 fs/xfs/xfs_pnfs.c  |  2 +-
 3 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index e43ce4811283..f3e4f243e877 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -267,6 +267,7 @@ xfs_iomap_eof_align_last_fsb(
 
 int
 xfs_iomap_write_direct(
+	struct xfs_trans	*tp,
 	struct xfs_inode	*ip,
 	xfs_fileoff_t		offset_fsb,
 	xfs_fileoff_t		count_fsb,
@@ -275,7 +276,7 @@ xfs_iomap_write_direct(
 	u64			*seq)
 {
 	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_trans	*tp;
+	struct xfs_trans	*local_tp = NULL;
 	xfs_filblks_t		resaligned;
 	int			nimaps;
 	unsigned int		dblocks, rblocks;
@@ -296,7 +297,10 @@ xfs_iomap_write_direct(
 		rblocks = 0;
 	}
 
-	error = xfs_qm_dqattach(ip);
+	if (tp)
+		error = xfs_qm_dqattach_locked(ip, false);
+	else
+		error = xfs_qm_dqattach(ip);
 	if (error)
 		return error;
 
@@ -322,10 +326,18 @@ xfs_iomap_write_direct(
 		}
 	}
 
-	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, dblocks,
-			rblocks, force, &tp);
-	if (error)
-		return error;
+	if (tp) {
+		error = xfs_trans_reserve_more_inode(tp, ip, dblocks, rblocks,
+				force);
+		if (error)
+			return error;
+	} else {
+		error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, dblocks,
+				rblocks, force, &tp);
+		if (error)
+			return error;
+		local_tp = tp;
+	}
 
 	error = xfs_iext_count_extend(tp, ip, XFS_DATA_FORK, nr_exts);
 	if (error)
@@ -341,9 +353,9 @@ xfs_iomap_write_direct(
 	if (error)
 		goto out_trans_cancel;
 
-	/*
-	 * Complete the transaction
-	 */
+	if (!local_tp)
+		return 0;
+
 	error = xfs_trans_commit(tp);
 	if (error)
 		goto out_unlock;
@@ -359,8 +371,11 @@ xfs_iomap_write_direct(
 	return error;
 
 out_trans_cancel:
-	xfs_trans_cancel(tp);
-	goto out_unlock;
+	if (local_tp) {
+		xfs_trans_cancel(tp);
+		goto out_unlock;
+	}
+	return error;
 }
 
 STATIC bool
@@ -1154,8 +1169,8 @@ xfs_direct_write_iomap_begin(
 		end_fsb = min(end_fsb, imap.br_startoff + imap.br_blockcount);
 	xfs_iunlock(ip, lockmode);
 
-	error = xfs_iomap_write_direct(ip, offset_fsb, end_fsb - offset_fsb,
-			flags, &imap, &seq);
+	error = xfs_iomap_write_direct(NULL, ip, offset_fsb,
+			end_fsb - offset_fsb, flags, &imap, &seq);
 	if (error)
 		return error;
 
diff --git a/fs/xfs/xfs_iomap.h b/fs/xfs/xfs_iomap.h
index ebcce7d49446..937a61c7a610 100644
--- a/fs/xfs/xfs_iomap.h
+++ b/fs/xfs/xfs_iomap.h
@@ -12,9 +12,10 @@ struct xfs_inode;
 struct xfs_bmbt_irec;
 struct xfs_zone_alloc_ctx;
 
-int xfs_iomap_write_direct(struct xfs_inode *ip, xfs_fileoff_t offset_fsb,
-		xfs_fileoff_t count_fsb, unsigned int flags,
-		struct xfs_bmbt_irec *imap, u64 *sequence);
+int xfs_iomap_write_direct(struct xfs_trans *tp, struct xfs_inode *ip,
+		xfs_fileoff_t offset_fsb, xfs_fileoff_t count_fsb,
+		unsigned int flags, struct xfs_bmbt_irec *imap,
+		u64 *sequence);
 int xfs_iomap_write_unwritten(struct xfs_inode *, xfs_off_t, xfs_off_t, bool);
 xfs_fileoff_t xfs_iomap_eof_align_last_fsb(struct xfs_inode *ip,
 		xfs_fileoff_t end_fsb);
diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
index f8535ecde21a..442564967b39 100644
--- a/fs/xfs/xfs_pnfs.c
+++ b/fs/xfs/xfs_pnfs.c
@@ -202,7 +202,7 @@ xfs_fs_map_blocks(
 					       imap.br_blockcount);
 		xfs_iunlock(ip, lock_flags);
 
-		error = xfs_iomap_write_direct(ip, offset_fsb,
+		error = xfs_iomap_write_direct(NULL, ip, offset_fsb,
 				end_fsb - offset_fsb, 0, &imap, &seq);
 		if (error)
 			goto out_unlock;
-- 
2.55.0

[PATCH 26/33] xfs: make xfs_iomap_write_direct fill in the iomap directly

[Dave Chinner]

Move the xfs_bmbt_to_iomap() call and the xfs_iomap_alloc tracing
into xfs_iomap_write_direct() so that callers do not need to
convert the returned imap to an iomap themselves.

Both callers performed the same conversion after the function
returned. By moving this inside the function, the callers are
simplified and the iomap sequence cookie is correctly captured
after the allocation completes under the ILOCK.

Add iomap, iomap_flags, offset and length parameters so the
function has everything it needs for tracing and the iomap
conversion. Replace the seq output parameter with an internal
variable.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_iomap.c | 46 ++++++++++++++++++++++------------------------
 fs/xfs/xfs_iomap.h |  3 ++-
 fs/xfs/xfs_pnfs.c  |  9 ++++-----
 3 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index f3e4f243e877..347a451ec7cb 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -273,7 +273,10 @@ xfs_iomap_write_direct(
 	xfs_fileoff_t		count_fsb,
 	unsigned int		flags,
 	struct xfs_bmbt_irec	*imap,
-	u64			*seq)
+	loff_t			offset,
+	loff_t			length,
+	struct iomap		*iomap,
+	u16			iomap_flags)
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*local_tp = NULL;
@@ -284,6 +287,7 @@ xfs_iomap_write_direct(
 	int			error;
 	int			bmapi_flags = XFS_BMAPI_PREALLOC;
 	int			nr_exts = XFS_IEXT_ADD_NOSPLIT_CNT;
+	u64			seq;
 
 	ASSERT(count_fsb > 0);
 
@@ -353,27 +357,26 @@ xfs_iomap_write_direct(
 	if (error)
 		goto out_trans_cancel;
 
-	if (!local_tp)
-		return 0;
-
-	error = xfs_trans_commit(tp);
-	if (error)
-		goto out_unlock;
-
-	if (unlikely(!xfs_valid_startblock(ip, imap->br_startblock))) {
-		xfs_bmap_mark_sick(ip, XFS_DATA_FORK);
-		error = xfs_alert_fsblock_zero(ip, imap);
+	seq = xfs_iomap_inode_sequence(ip, iomap_flags);
+	if (local_tp) {
+		error = xfs_trans_commit(tp);
+		xfs_iunlock(ip, XFS_ILOCK_EXCL);
+		if (error)
+			return error;
+		if (unlikely(!xfs_valid_startblock(ip, imap->br_startblock))) {
+			xfs_bmap_mark_sick(ip, XFS_DATA_FORK);
+			return xfs_alert_fsblock_zero(ip, imap);
+		}
 	}
 
-out_unlock:
-	*seq = xfs_iomap_inode_sequence(ip, 0);
-	xfs_iunlock(ip, XFS_ILOCK_EXCL);
-	return error;
+	trace_xfs_iomap_alloc(ip, offset, length, XFS_DATA_FORK, imap);
+	return xfs_bmbt_to_iomap(ip, iomap, imap, flags,
+			iomap_flags | IOMAP_F_NEW, seq);
 
 out_trans_cancel:
 	if (local_tp) {
 		xfs_trans_cancel(tp);
-		goto out_unlock;
+		xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	}
 	return error;
 }
@@ -1169,14 +1172,9 @@ xfs_direct_write_iomap_begin(
 		end_fsb = min(end_fsb, imap.br_startoff + imap.br_blockcount);
 	xfs_iunlock(ip, lockmode);
 
-	error = xfs_iomap_write_direct(NULL, ip, offset_fsb,
-			end_fsb - offset_fsb, flags, &imap, &seq);
-	if (error)
-		return error;
-
-	trace_xfs_iomap_alloc(ip, offset, length, XFS_DATA_FORK, &imap);
-	return xfs_bmbt_to_iomap(ip, iomap, &imap, flags,
-				 iomap_flags | IOMAP_F_NEW, seq);
+	return xfs_iomap_write_direct(NULL, ip, offset_fsb,
+			end_fsb - offset_fsb, flags, &imap,
+			offset, length, iomap, iomap_flags);
 
 out_unlock:
 	if (lockmode)
diff --git a/fs/xfs/xfs_iomap.h b/fs/xfs/xfs_iomap.h
index 937a61c7a610..c883a7175cd0 100644
--- a/fs/xfs/xfs_iomap.h
+++ b/fs/xfs/xfs_iomap.h
@@ -15,7 +15,8 @@ struct xfs_zone_alloc_ctx;
 int xfs_iomap_write_direct(struct xfs_trans *tp, struct xfs_inode *ip,
 		xfs_fileoff_t offset_fsb, xfs_fileoff_t count_fsb,
 		unsigned int flags, struct xfs_bmbt_irec *imap,
-		u64 *sequence);
+		loff_t offset, loff_t length, struct iomap *iomap,
+		u16 iomap_flags);
 int xfs_iomap_write_unwritten(struct xfs_inode *, xfs_off_t, xfs_off_t, bool);
 xfs_fileoff_t xfs_iomap_eof_align_last_fsb(struct xfs_inode *ip,
 		xfs_fileoff_t end_fsb);
diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
index 442564967b39..f9c0e748448e 100644
--- a/fs/xfs/xfs_pnfs.c
+++ b/fs/xfs/xfs_pnfs.c
@@ -190,8 +190,6 @@ xfs_fs_map_blocks(
 		xfs_iunlock(ip, lock_flags);
 		goto out_unlock;
 	}
-	seq = xfs_iomap_inode_sequence(ip, 0);
-
 	ASSERT(!nimaps || imap.br_startblock != DELAYSTARTBLOCK);
 
 	if (write && (!nimaps || imap.br_startblock == HOLESTARTBLOCK)) {
@@ -203,7 +201,8 @@ xfs_fs_map_blocks(
 		xfs_iunlock(ip, lock_flags);
 
 		error = xfs_iomap_write_direct(NULL, ip, offset_fsb,
-				end_fsb - offset_fsb, 0, &imap, &seq);
+				end_fsb - offset_fsb, 0, &imap,
+				offset, length, iomap, 0);
 		if (error)
 			goto out_unlock;
 
@@ -219,11 +218,11 @@ xfs_fs_map_blocks(
 			goto out_unlock;
 
 	} else {
+		seq = xfs_iomap_inode_sequence(ip, 0);
 		xfs_iunlock(ip, lock_flags);
+		error = xfs_bmbt_to_iomap(ip, iomap, &imap, 0, 0, seq);
 	}
 	xfs_iunlock(ip, XFS_IOLOCK_EXCL);
-
-	error = xfs_bmbt_to_iomap(ip, iomap, &imap, 0, 0, seq);
 	*device_generation = mp->m_generation;
 	return error;
 out_unlock:
-- 
2.55.0

[PATCH 27/33] xfs: plumb struct xfs_trans **tpp into xfs_direct_write_cow_iomap_begin

[Dave Chinner]

Add a struct xfs_trans **tpp parameter to
xfs_direct_write_cow_iomap_begin(). When tpp is provided and
xfs_reflink_allocate_cow() returns -EAGAIN (needing a transaction),
return -EAGAIN to the caller so they can allocate a transaction
and retry at a higher level. When tpp is NULL, handle the retry
internally as before using a local transaction.

The caller currently passes NULL, so the function continues to
manage its own transaction internally. No functional change.

This is preparation for moving the transaction allocation and
retry logic up to xfs_direct_write_iomap_begin() so that a single
unified retry loop handles both COW and non-COW allocation paths.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_iomap.c | 70 +++++++++++++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 26 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 347a451ec7cb..314a885bf9f3 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -902,6 +902,11 @@ xfs_bmap_hw_atomic_write_possible(
  * Handle COW extent allocation and iomap setup for direct writes to reflinked
  * files.
  *
+ * Transitional tpp handling:
+ *	!tpp = do everything internally using local_tp
+ *	tpp & !*tpp = caller did locking, wants -EAGAIN if transaction required
+ *	tpp && *tpp = caller did locking and transaction allocation
+ *
  * The caller passes in an imap and nimaps that the COW allocation will fill
  * with the data fork extent mapping. On return, *nimaps indicates whether the
  * caller needs to continue with the normal IO path:
@@ -913,6 +918,7 @@ xfs_bmap_hw_atomic_write_possible(
  */
 static int
 xfs_direct_write_cow_iomap_begin(
+	struct xfs_trans	**tpp,
 	struct xfs_inode	*ip,
 	loff_t			offset,
 	loff_t			length,
@@ -927,34 +933,37 @@ xfs_direct_write_cow_iomap_begin(
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_bmbt_irec	cmap;
 	struct xfs_trans	*tp = NULL;
+	struct xfs_trans	*local_tp = NULL;
 	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
 	xfs_fileoff_t		end_fsb = xfs_iomap_end_fsb(mp, offset, length);
 	bool			shared = false;
 	int			error;
 	u64			seq;
 
-	*lockmode = XFS_ILOCK_EXCL;
+	if (!tpp) {
+		*lockmode = XFS_ILOCK_EXCL;
 
-	error = xfs_ilock_for_iomap(ip, flags, lockmode);
-	if (error)
-		return error;
+		error = xfs_ilock_for_iomap(ip, flags, lockmode);
+		if (error)
+			return error;
 
 retry:
-	*nimaps = 1;
-	error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, imap,
-			       nimaps, 0);
-	if (error)
-		goto out_unlock;
+		*nimaps = 1;
+		error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, imap,
+				       nimaps, 0);
+		if (error)
+			goto out_unlock;
+	} else if (*tpp) {
+		tp = *tpp;
+	}
 
 	if (!imap_needs_cow(ip, flags, imap, *nimaps)) {
 		/*
 		 * Extent is not shared - return the imap and ILOCK to the
 		 * caller for normal IO path processing.
 		 */
-		if (tp) {
-			xfs_trans_cancel(tp);
-			tp = NULL;
-		}
+		if (local_tp)
+			xfs_trans_cancel(local_tp);
 		return 0;
 	}
 
@@ -962,13 +971,21 @@ xfs_direct_write_cow_iomap_begin(
 	if (flags & IOMAP_NOWAIT)
 		goto out_unlock;
 
-	/* may drop and re-acquire the ilock */
 	error = xfs_reflink_allocate_cow(&tp, ip, imap, &cmap, &shared,
 			lockmode,
 			(flags & IOMAP_DIRECT) || IS_DAX(VFS_I(ip)));
 	if (error == -EAGAIN) {
+		ASSERT(!tp);
+
 		/*
-		 * COW allocation needs a transaction. Drop the ILOCK and
+		 * If the caller can handle the retry, return -EAGAIN so
+		 * they can allocate a transaction and call again.
+		 */
+		if (tpp)
+			goto out_unlock;
+
+		/*
+		 * Otherwise handle the retry internally. Drop the ILOCK and
 		 * allocate a zero-block reservation transaction, which will
 		 * re-acquire the ILOCK. We cannot determine what extent type
 		 * will be found once we've regained the ILOCK, so the callees
@@ -980,22 +997,21 @@ xfs_direct_write_cow_iomap_begin(
 		 * Retry the imap lookup since the extent tree may have changed
 		 * while the ILOCK was not held.
 		 */
-		ASSERT(!tp);
-
 		xfs_iunlock(ip, *lockmode);
 
 		error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write,
 				0, 0, false, &tp);
 		if (error)
 			return error;
+		local_tp = tp;
 
 		goto retry;
 	}
 	if (error)
 		goto out_unlock;
 
-	if (tp) {
-		error = xfs_trans_commit(tp);
+	if (local_tp) {
+		error = xfs_trans_commit(local_tp);
 		tp = NULL;
 		if (error)
 			goto out_unlock;
@@ -1026,14 +1042,16 @@ xfs_direct_write_cow_iomap_begin(
 			goto out_unlock;
 	}
 	seq = xfs_iomap_inode_sequence(ip, IOMAP_F_SHARED);
-	xfs_iunlock(ip, *lockmode);
+	if (!tpp)
+		xfs_iunlock(ip, *lockmode);
 	return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, IOMAP_F_SHARED, seq);
 
 out_unlock:
-	if (tp)
-		xfs_trans_cancel(tp);
-	if (*lockmode)
+	if (local_tp) {
+		if (tp)
+			xfs_trans_cancel(local_tp);
 		xfs_iunlock(ip, *lockmode);
+	}
 	return error;
 }
 
@@ -1076,9 +1094,9 @@ xfs_direct_write_iomap_begin(
 		iomap_flags |= IOMAP_F_ATOMIC_BIO;
 
 	if (xfs_is_cow_inode(ip)) {
-		error = xfs_direct_write_cow_iomap_begin(ip, offset, length,
-				flags, iomap, srcmap, &imap, &nimaps,
-				&lockmode, iomap_flags);
+		error = xfs_direct_write_cow_iomap_begin(NULL, ip, offset,
+				length, flags, iomap, srcmap, &imap,
+				&nimaps, &lockmode, iomap_flags);
 		if (error)
 			return error;
 		if (!nimaps)
-- 
2.55.0

[PATCH 28/33] xfs: introduce struct xfs_direct_write_args for direct write call chain

[Dave Chinner]

Introduce struct xfs_direct_write_args to pass context through the
direct write extent allocation call chain instead of long lists of
individual parameters. The struct carries the transaction pointer,
inode, block allocation range (offset_fsb/count_fsb), IO byte range
(offset/length for tracing), iomap flags, data and COW fork extent
mappings, shared state, convert_now flag, and iomap/srcmap output
pointers.

Convert all functions in the direct write allocation chain:
xfs_iomap_write_direct(), xfs_direct_write_cow_iomap_begin(),
xfs_reflink_allocate_cow(), xfs_reflink_fill_cow_hole(), and
xfs_reflink_fill_delalloc().

xfs_direct_write_cow_iomap_begin() takes an additional bool
needs_tp parameter that controls whether the function manages
locking and transaction allocation internally (true for current
callers) or expects the caller to have already set up the
transaction and ILOCK (false, for future restructured callers).

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_iomap.c   | 217 ++++++++++++++++++++++---------------------
 fs/xfs/xfs_iomap.h   |  25 ++++-
 fs/xfs/xfs_pnfs.c    |  16 +++-
 fs/xfs/xfs_reflink.c | 100 +++++++++-----------
 fs/xfs/xfs_reflink.h |   6 +-
 5 files changed, 190 insertions(+), 174 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 314a885bf9f3..ed80b23115bf 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -267,17 +267,9 @@ xfs_iomap_eof_align_last_fsb(
 
 int
 xfs_iomap_write_direct(
-	struct xfs_trans	*tp,
-	struct xfs_inode	*ip,
-	xfs_fileoff_t		offset_fsb,
-	xfs_fileoff_t		count_fsb,
-	unsigned int		flags,
-	struct xfs_bmbt_irec	*imap,
-	loff_t			offset,
-	loff_t			length,
-	struct iomap		*iomap,
-	u16			iomap_flags)
+	struct xfs_direct_write_args *args)
 {
+	struct xfs_inode	*ip = args->ip;
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*local_tp = NULL;
 	xfs_filblks_t		resaligned;
@@ -289,9 +281,9 @@ xfs_iomap_write_direct(
 	int			nr_exts = XFS_IEXT_ADD_NOSPLIT_CNT;
 	u64			seq;
 
-	ASSERT(count_fsb > 0);
+	ASSERT(args->count_fsb > 0);
 
-	resaligned = xfs_aligned_fsb_count(offset_fsb, count_fsb,
+	resaligned = xfs_aligned_fsb_count(args->offset_fsb, args->count_fsb,
 					   xfs_get_extsz_hint(ip));
 	if (unlikely(XFS_IS_REALTIME_INODE(ip))) {
 		dblocks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
@@ -301,7 +293,7 @@ xfs_iomap_write_direct(
 		rblocks = 0;
 	}
 
-	if (tp)
+	if (args->tp)
 		error = xfs_qm_dqattach_locked(ip, false);
 	else
 		error = xfs_qm_dqattach(ip);
@@ -321,61 +313,61 @@ xfs_iomap_write_direct(
 	 * the reserve block pool for bmbt block allocation if there is no space
 	 * left but we need to do unwritten extent conversion.
 	 */
-	if (flags & IOMAP_DAX) {
+	if (args->flags & IOMAP_DAX) {
 		bmapi_flags = XFS_BMAPI_CONVERT | XFS_BMAPI_ZERO;
-		if (imap->br_state == XFS_EXT_UNWRITTEN) {
+		if (args->imap.br_state == XFS_EXT_UNWRITTEN) {
 			force = true;
 			nr_exts = XFS_IEXT_WRITE_UNWRITTEN_CNT;
 			dblocks = XFS_DIOSTRAT_SPACE_RES(mp, 0) << 1;
 		}
 	}
 
-	if (tp) {
-		error = xfs_trans_reserve_more_inode(tp, ip, dblocks, rblocks,
-				force);
+	if (args->tp) {
+		error = xfs_trans_reserve_more_inode(args->tp, ip, dblocks,
+				rblocks, force);
 		if (error)
 			return error;
 	} else {
 		error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, dblocks,
-				rblocks, force, &tp);
+				rblocks, force, &args->tp);
 		if (error)
 			return error;
-		local_tp = tp;
+		local_tp = args->tp;
 	}
 
-	error = xfs_iext_count_extend(tp, ip, XFS_DATA_FORK, nr_exts);
+	error = xfs_iext_count_extend(args->tp, ip, XFS_DATA_FORK, nr_exts);
 	if (error)
 		goto out_trans_cancel;
 
-	/*
-	 * From this point onwards we overwrite the imap pointer that the
-	 * caller gave to us.
-	 */
 	nimaps = 1;
-	error = xfs_bmapi_write(tp, ip, offset_fsb, count_fsb, bmapi_flags, 0,
-				imap, &nimaps);
+	error = xfs_bmapi_write(args->tp, ip, args->offset_fsb, args->count_fsb,
+				bmapi_flags, 0, &args->imap, &nimaps);
 	if (error)
 		goto out_trans_cancel;
 
-	seq = xfs_iomap_inode_sequence(ip, iomap_flags);
+	seq = xfs_iomap_inode_sequence(ip, args->iomap_flags);
 	if (local_tp) {
-		error = xfs_trans_commit(tp);
+		error = xfs_trans_commit(args->tp);
+		args->tp = NULL;
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
 		if (error)
 			return error;
-		if (unlikely(!xfs_valid_startblock(ip, imap->br_startblock))) {
+		if (unlikely(!xfs_valid_startblock(ip,
+				args->imap.br_startblock))) {
 			xfs_bmap_mark_sick(ip, XFS_DATA_FORK);
-			return xfs_alert_fsblock_zero(ip, imap);
+			return xfs_alert_fsblock_zero(ip, &args->imap);
 		}
 	}
 
-	trace_xfs_iomap_alloc(ip, offset, length, XFS_DATA_FORK, imap);
-	return xfs_bmbt_to_iomap(ip, iomap, imap, flags,
-			iomap_flags | IOMAP_F_NEW, seq);
+	trace_xfs_iomap_alloc(ip, args->offset, args->length, XFS_DATA_FORK,
+			&args->imap);
+	return xfs_bmbt_to_iomap(ip, args->iomap, &args->imap, args->flags,
+			args->iomap_flags | IOMAP_F_NEW, seq);
 
 out_trans_cancel:
 	if (local_tp) {
-		xfs_trans_cancel(tp);
+		xfs_trans_cancel(args->tp);
+		args->tp = NULL;
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	}
 	return error;
@@ -918,92 +910,72 @@ xfs_bmap_hw_atomic_write_possible(
  */
 static int
 xfs_direct_write_cow_iomap_begin(
-	struct xfs_trans	**tpp,
-	struct xfs_inode	*ip,
-	loff_t			offset,
-	loff_t			length,
-	unsigned		flags,
-	struct iomap		*iomap,
-	struct iomap		*srcmap,
-	struct xfs_bmbt_irec	*imap,
-	int			*nimaps,
-	unsigned int		*lockmode,
-	u16			iomap_flags)
+	struct xfs_direct_write_args *args,
+	bool			needs_tp)
 {
+	struct xfs_inode	*ip = args->ip;
 	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_bmbt_irec	cmap;
-	struct xfs_trans	*tp = NULL;
 	struct xfs_trans	*local_tp = NULL;
-	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
-	xfs_fileoff_t		end_fsb = xfs_iomap_end_fsb(mp, offset, length);
-	bool			shared = false;
+	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, args->offset);
+	xfs_fileoff_t		end_fsb = xfs_iomap_end_fsb(mp, args->offset,
+					args->length);
 	int			error;
 	u64			seq;
 
-	if (!tpp) {
-		*lockmode = XFS_ILOCK_EXCL;
+	if (needs_tp) {
+		args->lockmode = XFS_ILOCK_EXCL;
 
-		error = xfs_ilock_for_iomap(ip, flags, lockmode);
+		error = xfs_ilock_for_iomap(ip, args->flags, &args->lockmode);
 		if (error)
 			return error;
 
 retry:
-		*nimaps = 1;
-		error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, imap,
-				       nimaps, 0);
+		args->nimaps = 1;
+		error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb,
+				&args->imap, &args->nimaps, 0);
 		if (error)
 			goto out_unlock;
-	} else if (*tpp) {
-		tp = *tpp;
 	}
 
-	if (!imap_needs_cow(ip, flags, imap, *nimaps)) {
-		/*
-		 * Extent is not shared - return the imap and ILOCK to the
-		 * caller for normal IO path processing.
-		 */
-		if (local_tp)
+	if (!imap_needs_cow(ip, args->flags, &args->imap, args->nimaps)) {
+		if (local_tp) {
 			xfs_trans_cancel(local_tp);
+			args->tp = NULL;
+		}
 		return 0;
 	}
 
 	error = -EAGAIN;
-	if (flags & IOMAP_NOWAIT)
+	if (args->flags & IOMAP_NOWAIT)
 		goto out_unlock;
 
-	error = xfs_reflink_allocate_cow(&tp, ip, imap, &cmap, &shared,
-			lockmode,
-			(flags & IOMAP_DIRECT) || IS_DAX(VFS_I(ip)));
+	error = xfs_reflink_allocate_cow(args);
 	if (error == -EAGAIN) {
-		ASSERT(!tp);
+		ASSERT(!args->tp);
 
-		/*
-		 * If the caller can handle the retry, return -EAGAIN so
-		 * they can allocate a transaction and call again.
-		 */
-		if (tpp)
+		if (!needs_tp)
 			goto out_unlock;
 
 		/*
-		 * Otherwise handle the retry internally. Drop the ILOCK and
-		 * allocate a zero-block reservation transaction, which will
-		 * re-acquire the ILOCK. We cannot determine what extent type
-		 * will be found once we've regained the ILOCK, so the callees
-		 * will use xfs_trans_reserve_more_inode() directly to reserve
-		 * any blocks they require before they start modifications.
-		 * This allows ENOSPC to be returned and the transaction
-		 * cancelled safely if the block reservation cannot be made.
+		 * Handle the retry internally. Drop the ILOCK and allocate a
+		 * zero-block reservation transaction, which will re-acquire
+		 * the ILOCK. We cannot determine what extent type will be
+		 * found once we've regained the ILOCK, so the callees will
+		 * use xfs_trans_reserve_more_inode() directly to reserve any
+		 * blocks they require before they start modifications. This
+		 * allows ENOSPC to be returned and the transaction cancelled
+		 * safely if the block reservation cannot be made.
 		 *
 		 * Retry the imap lookup since the extent tree may have changed
 		 * while the ILOCK was not held.
 		 */
-		xfs_iunlock(ip, *lockmode);
+		xfs_iunlock(ip, args->lockmode);
 
 		error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write,
-				0, 0, false, &tp);
+				0, 0, false, &args->tp);
 		if (error)
 			return error;
-		local_tp = tp;
+		local_tp = args->tp;
 
 		goto retry;
 	}
@@ -1012,16 +984,16 @@ xfs_direct_write_cow_iomap_begin(
 
 	if (local_tp) {
 		error = xfs_trans_commit(local_tp);
-		tp = NULL;
+		args->tp = NULL;
 		if (error)
 			goto out_unlock;
 	}
 
-	if (!shared)
+	if (!args->shared)
 		return 0;
 
-	if ((flags & IOMAP_ATOMIC) &&
-	    !xfs_bmap_hw_atomic_write_possible(ip, &cmap,
+	if ((args->flags & IOMAP_ATOMIC) &&
+	    !xfs_bmap_hw_atomic_write_possible(ip, &args->cmap,
 			offset_fsb, end_fsb)) {
 		error = -ENOPROTOOPT;
 		goto out_unlock;
@@ -1031,26 +1003,31 @@ xfs_direct_write_cow_iomap_begin(
 	 * COW extent found and allocated. Set up iomap/srcmap and return
 	 * with *nimaps = 0 to tell the caller the COW path is complete.
 	 */
-	*nimaps = 0;
-	length = XFS_FSB_TO_B(mp, cmap.br_startoff + cmap.br_blockcount);
-	trace_xfs_iomap_found(ip, offset, length - offset, XFS_COW_FORK,
-			&cmap);
-	if (imap->br_startblock != HOLESTARTBLOCK) {
+	args->nimaps = 0;
+	args->length = XFS_FSB_TO_B(mp,
+			args->cmap.br_startoff + args->cmap.br_blockcount);
+	trace_xfs_iomap_found(ip, args->offset,
+			args->length - args->offset, XFS_COW_FORK,
+			&args->cmap);
+	if (args->imap.br_startblock != HOLESTARTBLOCK) {
 		seq = xfs_iomap_inode_sequence(ip, 0);
-		error = xfs_bmbt_to_iomap(ip, srcmap, imap, flags, 0, seq);
+		error = xfs_bmbt_to_iomap(ip, args->srcmap, &args->imap,
+				args->flags, 0, seq);
 		if (error)
 			goto out_unlock;
 	}
 	seq = xfs_iomap_inode_sequence(ip, IOMAP_F_SHARED);
-	if (!tpp)
-		xfs_iunlock(ip, *lockmode);
-	return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, IOMAP_F_SHARED, seq);
+	if (needs_tp)
+		xfs_iunlock(ip, args->lockmode);
+	return xfs_bmbt_to_iomap(ip, args->iomap, &args->cmap, args->flags,
+			IOMAP_F_SHARED, seq);
 
 out_unlock:
 	if (local_tp) {
-		if (tp)
+		if (args->tp)
 			xfs_trans_cancel(local_tp);
-		xfs_iunlock(ip, *lockmode);
+		args->tp = NULL;
+		xfs_iunlock(ip, args->lockmode);
 	}
 	return error;
 }
@@ -1094,14 +1071,26 @@ xfs_direct_write_iomap_begin(
 		iomap_flags |= IOMAP_F_ATOMIC_BIO;
 
 	if (xfs_is_cow_inode(ip)) {
-		error = xfs_direct_write_cow_iomap_begin(NULL, ip, offset,
-				length, flags, iomap, srcmap, &imap,
-				&nimaps, &lockmode, iomap_flags);
+		struct xfs_direct_write_args cow_args = {
+			.ip		= ip,
+			.offset		= offset,
+			.length		= length,
+			.flags		= flags,
+			.iomap_flags	= iomap_flags,
+			.convert_now	= (flags & IOMAP_DIRECT) ||
+					  IS_DAX(inode),
+			.iomap		= iomap,
+			.srcmap		= srcmap,
+		};
+
+		error = xfs_direct_write_cow_iomap_begin(&cow_args, true);
 		if (error)
 			return error;
-		if (!nimaps)
+		if (!cow_args.nimaps)
 			return 0;
 
+		imap = cow_args.imap;
+		lockmode = cow_args.lockmode;
 		end_fsb = imap.br_startoff + imap.br_blockcount;
 		length = XFS_FSB_TO_B(mp, end_fsb) - offset;
 	} else {
@@ -1190,9 +1179,21 @@ xfs_direct_write_iomap_begin(
 		end_fsb = min(end_fsb, imap.br_startoff + imap.br_blockcount);
 	xfs_iunlock(ip, lockmode);
 
-	return xfs_iomap_write_direct(NULL, ip, offset_fsb,
-			end_fsb - offset_fsb, flags, &imap,
-			offset, length, iomap, iomap_flags);
+	{
+		struct xfs_direct_write_args args = {
+			.ip		= ip,
+			.offset_fsb	= offset_fsb,
+			.count_fsb	= end_fsb - offset_fsb,
+			.offset		= offset,
+			.length		= length,
+			.flags		= flags,
+			.iomap_flags	= iomap_flags,
+			.imap		= imap,
+			.iomap		= iomap,
+		};
+
+		return xfs_iomap_write_direct(&args);
+	}
 
 out_unlock:
 	if (lockmode)
diff --git a/fs/xfs/xfs_iomap.h b/fs/xfs/xfs_iomap.h
index c883a7175cd0..7dc0b5993bed 100644
--- a/fs/xfs/xfs_iomap.h
+++ b/fs/xfs/xfs_iomap.h
@@ -12,11 +12,26 @@ struct xfs_inode;
 struct xfs_bmbt_irec;
 struct xfs_zone_alloc_ctx;
 
-int xfs_iomap_write_direct(struct xfs_trans *tp, struct xfs_inode *ip,
-		xfs_fileoff_t offset_fsb, xfs_fileoff_t count_fsb,
-		unsigned int flags, struct xfs_bmbt_irec *imap,
-		loff_t offset, loff_t length, struct iomap *iomap,
-		u16 iomap_flags);
+struct xfs_direct_write_args {
+	struct xfs_trans	*tp;
+	struct xfs_inode	*ip;
+	xfs_fileoff_t		offset_fsb;	/* block range to allocate */
+	xfs_fileoff_t		count_fsb;
+	loff_t			offset;		/* IO byte range for tracing */
+	loff_t			length;
+	unsigned int		flags;
+	u16			iomap_flags;
+	unsigned int		lockmode;
+	struct xfs_bmbt_irec	imap;
+	struct xfs_bmbt_irec	cmap;
+	int			nimaps;
+	bool			shared;
+	bool			convert_now;
+	struct iomap		*iomap;
+	struct iomap		*srcmap;
+};
+
+int xfs_iomap_write_direct(struct xfs_direct_write_args *args);
 int xfs_iomap_write_unwritten(struct xfs_inode *, xfs_off_t, xfs_off_t, bool);
 xfs_fileoff_t xfs_iomap_eof_align_last_fsb(struct xfs_inode *ip,
 		xfs_fileoff_t end_fsb);
diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
index f9c0e748448e..495e081838d0 100644
--- a/fs/xfs/xfs_pnfs.c
+++ b/fs/xfs/xfs_pnfs.c
@@ -200,9 +200,19 @@ xfs_fs_map_blocks(
 					       imap.br_blockcount);
 		xfs_iunlock(ip, lock_flags);
 
-		error = xfs_iomap_write_direct(NULL, ip, offset_fsb,
-				end_fsb - offset_fsb, 0, &imap,
-				offset, length, iomap, 0);
+		{
+			struct xfs_direct_write_args args = {
+				.ip		= ip,
+				.offset_fsb	= offset_fsb,
+				.count_fsb	= end_fsb - offset_fsb,
+				.offset		= offset,
+				.length		= length,
+				.imap		= imap,
+				.iomap		= iomap,
+			};
+
+			error = xfs_iomap_write_direct(&args);
+		}
 		if (error)
 			goto out_unlock;
 
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 597300f39611..f0dcc66c91ec 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -430,13 +430,9 @@ xfs_reflink_convert_unwritten(
 
 static int
 xfs_reflink_fill_cow_hole(
-	struct xfs_trans	**tpp,
-	struct xfs_inode	*ip,
-	struct xfs_bmbt_irec	*imap,
-	struct xfs_bmbt_irec	*cmap,
-	bool			*shared,
-	bool			convert_now)
+	struct xfs_direct_write_args *args)
 {
+	struct xfs_inode	*ip = args->ip;
 	struct xfs_mount	*mp = ip->i_mount;
 	xfs_filblks_t		resaligned;
 	unsigned int		dblocks = 0, rblocks = 0;
@@ -444,10 +440,11 @@ xfs_reflink_fill_cow_hole(
 	int			error;
 	bool			found;
 
-	ASSERT(*tpp);
+	ASSERT(args->tp);
 
-	error = xfs_find_trim_cow_extent(ip, imap, cmap, shared, &found);
-	if (error || !*shared)
+	error = xfs_find_trim_cow_extent(ip, &args->imap, &args->cmap,
+			&args->shared, &found);
+	if (error || !args->shared)
 		return error;
 
 	if (found)
@@ -460,10 +457,10 @@ xfs_reflink_fill_cow_hole(
 	 * state was known under the ILOCK. The transaction has not been
 	 * dirtied yet, so on ENOSPC it can safely be cancelled by the caller.
 	 */
-	ASSERT(!((*tpp)->t_flags & XFS_TRANS_DIRTY));
+	ASSERT(!(args->tp->t_flags & XFS_TRANS_DIRTY));
 
-	resaligned = xfs_aligned_fsb_count(imap->br_startoff,
-			imap->br_blockcount, xfs_get_cowextsz_hint(ip));
+	resaligned = xfs_aligned_fsb_count(args->imap.br_startoff,
+			args->imap.br_blockcount, xfs_get_cowextsz_hint(ip));
 	if (XFS_IS_REALTIME_INODE(ip)) {
 		dblocks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
 		rblocks = resaligned;
@@ -471,36 +468,33 @@ xfs_reflink_fill_cow_hole(
 		dblocks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
 	}
 
-	error = xfs_trans_reserve_more_inode(*tpp, ip, dblocks, rblocks,
+	error = xfs_trans_reserve_more_inode(args->tp, ip, dblocks, rblocks,
 			false);
 	if (error)
 		return error;
 
 	/* Allocate the entire reservation as unwritten blocks. */
 	nimaps = 1;
-	error = xfs_bmapi_write(*tpp, ip, imap->br_startoff,
-			imap->br_blockcount,
-			XFS_BMAPI_COWFORK | XFS_BMAPI_PREALLOC, 0, cmap,
-			&nimaps);
+	error = xfs_bmapi_write(args->tp, ip, args->imap.br_startoff,
+			args->imap.br_blockcount,
+			XFS_BMAPI_COWFORK | XFS_BMAPI_PREALLOC, 0,
+			&args->cmap, &nimaps);
 	if (error)
 		return error;
 
 	xfs_inode_set_cowblocks_tag(ip);
 
 convert:
-	return xfs_reflink_convert_unwritten(ip, imap, cmap, convert_now);
+	return xfs_reflink_convert_unwritten(ip, &args->imap, &args->cmap,
+			args->convert_now);
 }
 
 static int
 xfs_reflink_fill_delalloc(
-	struct xfs_trans	**tpp,
-	struct xfs_inode	*ip,
-	struct xfs_bmbt_irec	*imap,
-	struct xfs_bmbt_irec	*cmap,
-	bool			*shared,
-	bool			convert_now)
+	struct xfs_direct_write_args *args)
 {
-	struct xfs_trans	*tp = *tpp;
+	struct xfs_inode	*ip = args->ip;
+	struct xfs_trans	*tp = args->tp;
 	int			nimaps;
 	int			error;
 	bool			found;
@@ -508,25 +502,25 @@ xfs_reflink_fill_delalloc(
 	ASSERT(tp);
 
 	do {
-		error = xfs_find_trim_cow_extent(ip, imap, cmap, shared,
-				&found);
-		if (error || !*shared)
+		error = xfs_find_trim_cow_extent(ip, &args->imap, &args->cmap,
+				&args->shared, &found);
+		if (error || !args->shared)
 			goto out_error;
 
 		if (found)
 			break;
 
-		ASSERT(isnullstartblock(cmap->br_startblock) ||
-		       cmap->br_startblock == DELAYSTARTBLOCK);
+		ASSERT(isnullstartblock(args->cmap.br_startblock) ||
+		       args->cmap.br_startblock == DELAYSTARTBLOCK);
 
 		/*
 		 * Replace delalloc reservation with an unwritten extent.
 		 */
 		nimaps = 1;
-		error = xfs_bmapi_write(tp, ip, cmap->br_startoff,
-				cmap->br_blockcount,
+		error = xfs_bmapi_write(tp, ip, args->cmap.br_startoff,
+				args->cmap.br_blockcount,
 				XFS_BMAPI_COWFORK | XFS_BMAPI_PREALLOC, 0,
-				cmap, &nimaps);
+				&args->cmap, &nimaps);
 		if (error)
 			goto out_error;
 
@@ -535,25 +529,22 @@ xfs_reflink_fill_delalloc(
 		error = xfs_defer_finish(&tp);
 		if (error)
 			goto out_error;
-	} while (cmap->br_startoff + cmap->br_blockcount <= imap->br_startoff);
+	} while (args->cmap.br_startoff + args->cmap.br_blockcount <=
+			args->imap.br_startoff);
 
-	error = xfs_reflink_convert_unwritten(ip, imap, cmap, convert_now);
+	error = xfs_reflink_convert_unwritten(ip, &args->imap, &args->cmap,
+			args->convert_now);
 out_error:
-	*tpp = tp;
+	args->tp = tp;
 	return error;
 }
 
 /* Allocate all CoW reservations covering a range of blocks in a file. */
 int
 xfs_reflink_allocate_cow(
-	struct xfs_trans	**tpp,
-	struct xfs_inode	*ip,
-	struct xfs_bmbt_irec	*imap,
-	struct xfs_bmbt_irec	*cmap,
-	bool			*shared,
-	uint			*lockmode,
-	bool			convert_now)
+	struct xfs_direct_write_args *args)
 {
+	struct xfs_inode	*ip = args->ip;
 	int			error;
 	bool			found;
 
@@ -563,34 +554,33 @@ xfs_reflink_allocate_cow(
 		xfs_ifork_init_cow(ip);
 	}
 
-	error = xfs_find_trim_cow_extent(ip, imap, cmap, shared, &found);
-	if (error || !*shared)
+	error = xfs_find_trim_cow_extent(ip, &args->imap, &args->cmap,
+			&args->shared, &found);
+	if (error || !args->shared)
 		return error;
 
 	/* CoW fork has a real extent */
 	if (found)
-		return xfs_reflink_convert_unwritten(ip, imap, cmap,
-				convert_now);
+		return xfs_reflink_convert_unwritten(ip, &args->imap,
+				&args->cmap, args->convert_now);
 
 	/*
 	 * Allocation is now required, so we need a transaction context from
 	 * the caller if it hasn't already supplied one.
 	 */
-	if (!*tpp)
+	if (!args->tp)
 		return -EAGAIN;
 
-	if (cmap->br_startoff > imap->br_startoff)
-		return xfs_reflink_fill_cow_hole(tpp, ip, imap, cmap, shared,
-				convert_now);
+	if (args->cmap.br_startoff > args->imap.br_startoff)
+		return xfs_reflink_fill_cow_hole(args);
 
 	/*
 	 * CoW fork has a delalloc reservation. Replace it with a real extent.
 	 * There may or may not be a data fork mapping.
 	 */
-	if (isnullstartblock(cmap->br_startblock) ||
-	    cmap->br_startblock == DELAYSTARTBLOCK)
-		return xfs_reflink_fill_delalloc(tpp, ip, imap, cmap, shared,
-				convert_now);
+	if (isnullstartblock(args->cmap.br_startblock) ||
+	    args->cmap.br_startblock == DELAYSTARTBLOCK)
+		return xfs_reflink_fill_delalloc(args);
 
 	/* Shouldn't get here. */
 	ASSERT(0);
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index 3da8374829c3..57c8a9a018bd 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -6,6 +6,8 @@
 #ifndef __XFS_REFLINK_H
 #define __XFS_REFLINK_H 1
 
+struct xfs_direct_write_args;
+
 /*
  * 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
@@ -30,9 +32,7 @@ int xfs_reflink_trim_around_shared(struct xfs_inode *ip,
 int xfs_bmap_trim_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap,
 		bool *shared);
 
-int xfs_reflink_allocate_cow(struct xfs_trans **tpp, struct xfs_inode *ip,
-		struct xfs_bmbt_irec *imap, struct xfs_bmbt_irec *cmap,
-		bool *shared, uint *lockmode, bool convert_now);
+int xfs_reflink_allocate_cow(struct xfs_direct_write_args *args);
 extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset,
 		xfs_off_t count);
 int xfs_reflink_convert_cow_locked(struct xfs_inode *ip,
-- 
2.55.0

[PATCH 29/33] xfs: convert xfs_direct_write_iomap_begin to use dwa struct throughout

[Dave Chinner]

Convert xfs_direct_write_iomap_begin() to use a single
xfs_direct_write_args struct (dwa) allocated on the stack instead
of individual local variables for the fields that are passed down
the call chain.

This replaces the temporary args struct construction at each call
site with a single struct initialised at function entry. The
imap, nimaps, lockmode, iomap_flags, and other fields that were
previously local variables are now accessed through dwa.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_iomap.c | 109 +++++++++++++++++++--------------------------
 1 file changed, 47 insertions(+), 62 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index ed80b23115bf..b388ded20ffc 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1043,14 +1043,21 @@ xfs_direct_write_iomap_begin(
 {
 	struct xfs_inode	*ip = XFS_I(inode);
 	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_bmbt_irec	imap;
-	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
+	struct xfs_direct_write_args dwa = {
+		.ip		= ip,
+		.offset_fsb	= XFS_B_TO_FSBT(mp, offset),
+		.offset		= offset,
+		.length		= length,
+		.flags		= flags,
+		.nimaps		= 1,
+		.convert_now	= (flags & IOMAP_DIRECT) || IS_DAX(inode),
+		.iomap		= iomap,
+		.srcmap		= srcmap,
+	};
 	xfs_fileoff_t		end_fsb = xfs_iomap_end_fsb(mp, offset, length);
 	xfs_fileoff_t		orig_end_fsb = end_fsb;
-	int			nimaps = 1, error = 0;
-	u16			iomap_flags = 0;
 	bool			needs_alloc;
-	unsigned int		lockmode;
+	int			error = 0;
 	u64			seq;
 
 	ASSERT(flags & (IOMAP_WRITE | IOMAP_ZERO));
@@ -1064,49 +1071,36 @@ xfs_direct_write_iomap_begin(
 	 * there is no other metadata changes pending or have been made here.
 	 */
 	if (offset + length > i_size_read(inode))
-		iomap_flags |= IOMAP_F_DIRTY;
+		dwa.iomap_flags |= IOMAP_F_DIRTY;
 
 	/* HW-offload atomics are always used in this path */
 	if (flags & IOMAP_ATOMIC)
-		iomap_flags |= IOMAP_F_ATOMIC_BIO;
+		dwa.iomap_flags |= IOMAP_F_ATOMIC_BIO;
 
 	if (xfs_is_cow_inode(ip)) {
-		struct xfs_direct_write_args cow_args = {
-			.ip		= ip,
-			.offset		= offset,
-			.length		= length,
-			.flags		= flags,
-			.iomap_flags	= iomap_flags,
-			.convert_now	= (flags & IOMAP_DIRECT) ||
-					  IS_DAX(inode),
-			.iomap		= iomap,
-			.srcmap		= srcmap,
-		};
-
-		error = xfs_direct_write_cow_iomap_begin(&cow_args, true);
+		error = xfs_direct_write_cow_iomap_begin(&dwa, true);
 		if (error)
 			return error;
-		if (!cow_args.nimaps)
+		if (!dwa.nimaps)
 			return 0;
 
-		imap = cow_args.imap;
-		lockmode = cow_args.lockmode;
-		end_fsb = imap.br_startoff + imap.br_blockcount;
-		length = XFS_FSB_TO_B(mp, end_fsb) - offset;
+		end_fsb = dwa.imap.br_startoff + dwa.imap.br_blockcount;
+		dwa.length = XFS_FSB_TO_B(mp, end_fsb) - offset;
 	} else {
-		lockmode = XFS_ILOCK_SHARED;
+		dwa.lockmode = XFS_ILOCK_SHARED;
 
-		error = xfs_ilock_for_iomap(ip, flags, &lockmode);
+		error = xfs_ilock_for_iomap(ip, flags, &dwa.lockmode);
 		if (error)
 			return error;
 
-		error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb,
-				&imap, &nimaps, 0);
+		error = xfs_bmapi_read(ip, dwa.offset_fsb,
+				end_fsb - dwa.offset_fsb,
+				&dwa.imap, &dwa.nimaps, 0);
 		if (error)
 			goto out_unlock;
 	}
 
-	needs_alloc = imap_needs_alloc(inode, flags, &imap, nimaps);
+	needs_alloc = imap_needs_alloc(inode, flags, &dwa.imap, dwa.nimaps);
 
 	if (flags & IOMAP_ATOMIC) {
 		error = -ENOPROTOOPT;
@@ -1115,11 +1109,11 @@ xfs_direct_write_iomap_begin(
 		 * then we may end up with multiple extents, which means that
 		 * REQ_ATOMIC-based cannot be used, so avoid this possibility.
 		 */
-		if (needs_alloc && orig_end_fsb - offset_fsb > 1)
+		if (needs_alloc && orig_end_fsb - dwa.offset_fsb > 1)
 			goto out_unlock;
 
-		if (!xfs_bmap_hw_atomic_write_possible(ip, &imap, offset_fsb,
-				orig_end_fsb))
+		if (!xfs_bmap_hw_atomic_write_possible(ip, &dwa.imap,
+				dwa.offset_fsb, orig_end_fsb))
 			goto out_unlock;
 	}
 
@@ -1134,7 +1128,7 @@ xfs_direct_write_iomap_begin(
 	 */
 	if (flags & (IOMAP_NOWAIT | IOMAP_OVERWRITE_ONLY)) {
 		error = -EAGAIN;
-		if (!imap_spans_range(&imap, offset_fsb, end_fsb))
+		if (!imap_spans_range(&dwa.imap, dwa.offset_fsb, end_fsb))
 			goto out_unlock;
 	}
 
@@ -1146,15 +1140,17 @@ xfs_direct_write_iomap_begin(
 	 */
 	if (flags & IOMAP_OVERWRITE_ONLY) {
 		error = -EAGAIN;
-		if (imap.br_state != XFS_EXT_NORM &&
-	            ((offset | length) & mp->m_blockmask))
+		if (dwa.imap.br_state != XFS_EXT_NORM &&
+		    ((offset | dwa.length) & mp->m_blockmask))
 			goto out_unlock;
 	}
 
-	seq = xfs_iomap_inode_sequence(ip, iomap_flags);
-	xfs_iunlock(ip, lockmode);
-	trace_xfs_iomap_found(ip, offset, length, XFS_DATA_FORK, &imap);
-	return xfs_bmbt_to_iomap(ip, iomap, &imap, flags, iomap_flags, seq);
+	seq = xfs_iomap_inode_sequence(ip, dwa.iomap_flags);
+	xfs_iunlock(ip, dwa.lockmode);
+	trace_xfs_iomap_found(ip, offset, dwa.length, XFS_DATA_FORK,
+			&dwa.imap);
+	return xfs_bmbt_to_iomap(ip, iomap, &dwa.imap, flags,
+			dwa.iomap_flags, seq);
 
 allocate_blocks:
 	error = -EAGAIN;
@@ -1170,34 +1166,23 @@ xfs_direct_write_iomap_begin(
 	 * Note that the values needs to be less than 32-bits wide until the
 	 * lower level functions are updated.
 	 */
-	length = min_t(loff_t, length, 1024 * PAGE_SIZE);
-	end_fsb = xfs_iomap_end_fsb(mp, offset, length);
+	dwa.length = min_t(loff_t, dwa.length, 1024 * PAGE_SIZE);
+	end_fsb = xfs_iomap_end_fsb(mp, offset, dwa.length);
 
-	if (offset + length > XFS_ISIZE(ip))
+	if (offset + dwa.length > XFS_ISIZE(ip))
 		end_fsb = xfs_iomap_eof_align_last_fsb(ip, end_fsb);
-	else if (nimaps && imap.br_startblock == HOLESTARTBLOCK)
-		end_fsb = min(end_fsb, imap.br_startoff + imap.br_blockcount);
-	xfs_iunlock(ip, lockmode);
+	else if (dwa.nimaps && dwa.imap.br_startblock == HOLESTARTBLOCK)
+		end_fsb = min(end_fsb, dwa.imap.br_startoff +
+				dwa.imap.br_blockcount);
 
-	{
-		struct xfs_direct_write_args args = {
-			.ip		= ip,
-			.offset_fsb	= offset_fsb,
-			.count_fsb	= end_fsb - offset_fsb,
-			.offset		= offset,
-			.length		= length,
-			.flags		= flags,
-			.iomap_flags	= iomap_flags,
-			.imap		= imap,
-			.iomap		= iomap,
-		};
+	dwa.count_fsb = end_fsb - dwa.offset_fsb;
+	xfs_iunlock(ip, dwa.lockmode);
 
-		return xfs_iomap_write_direct(&args);
-	}
+	return xfs_iomap_write_direct(&dwa);
 
 out_unlock:
-	if (lockmode)
-		xfs_iunlock(ip, lockmode);
+	if (dwa.lockmode)
+		xfs_iunlock(ip, dwa.lockmode);
 	return error;
 }
 
-- 
2.55.0

[PATCH 30/33] xfs: restructure xfs_direct_write_iomap_begin with unified retry loop

[Dave Chinner]

Replace the split COW/non-COW structure in xfs_direct_write_iomap_begin()
with a unified do {} while (dwa.tp) loop that handles both COW extent
allocation and data fork allocation atomically under a single ILOCK
hold.

The loop structure:
  - xfs_ilock_for_iomap() is called once before the loop
  - xfs_bmapi_read() at the top of each iteration reads fresh state
  - COW check calls xfs_direct_write_cow_iomap_begin() with
    needs_tp=false, which returns -EAGAIN if a transaction is needed
  - If allocation is needed (needs_alloc) but no transaction, falls
    through to alloc_trans
  - If a transaction is available, calls xfs_iomap_write_direct()
    with the transaction, commits, and returns
  - Overwrite path cancels unused tp and returns the mapping
  - alloc_trans at the tail: ASSERT(!tp), checks NOWAIT/OVERWRITE,
    drops the ILOCK, allocates a zero-block transaction via
    xfs_trans_alloc_inode() (which reacquires ILOCK_EXCL), and
    loops back to re-read the extent tree

This makes the extent lookup and allocation atomic with respect to
the ILOCK for both COW and non-COW paths.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_iomap.c | 218 ++++++++++++++++++++++++++++-----------------
 1 file changed, 135 insertions(+), 83 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index b388ded20ffc..d58ecc05a4d1 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1077,110 +1077,162 @@ xfs_direct_write_iomap_begin(
 	if (flags & IOMAP_ATOMIC)
 		dwa.iomap_flags |= IOMAP_F_ATOMIC_BIO;
 
-	if (xfs_is_cow_inode(ip)) {
-		error = xfs_direct_write_cow_iomap_begin(&dwa, true);
-		if (error)
-			return error;
-		if (!dwa.nimaps)
-			return 0;
-
-		end_fsb = dwa.imap.br_startoff + dwa.imap.br_blockcount;
-		dwa.length = XFS_FSB_TO_B(mp, end_fsb) - offset;
-	} else {
+	if (xfs_is_cow_inode(ip))
+		dwa.lockmode = XFS_ILOCK_EXCL;
+	else
 		dwa.lockmode = XFS_ILOCK_SHARED;
 
-		error = xfs_ilock_for_iomap(ip, flags, &dwa.lockmode);
-		if (error)
-			return error;
+	error = xfs_ilock_for_iomap(ip, flags, &dwa.lockmode);
+	if (error)
+		return error;
 
+	do {
+		dwa.nimaps = 1;
 		error = xfs_bmapi_read(ip, dwa.offset_fsb,
 				end_fsb - dwa.offset_fsb,
 				&dwa.imap, &dwa.nimaps, 0);
 		if (error)
 			goto out_unlock;
-	}
 
-	needs_alloc = imap_needs_alloc(inode, flags, &dwa.imap, dwa.nimaps);
-
-	if (flags & IOMAP_ATOMIC) {
-		error = -ENOPROTOOPT;
-		/*
-		 * If we allocate less than what is required for the write
-		 * then we may end up with multiple extents, which means that
-		 * REQ_ATOMIC-based cannot be used, so avoid this possibility.
-		 */
-		if (needs_alloc && orig_end_fsb - dwa.offset_fsb > 1)
-			goto out_unlock;
+		if (xfs_is_cow_inode(ip)) {
+			error = xfs_direct_write_cow_iomap_begin(&dwa, false);
+			if (error == -EAGAIN)
+				goto alloc_trans;
+			if (error)
+				goto out_unlock;
+			if (!dwa.nimaps) {
+				/*
+				 * COW iomaps filled in. Commit the
+				 * transaction if one was needed and
+				 * return.
+				 */
+				if (dwa.tp)
+					error = xfs_trans_commit(dwa.tp);
+				xfs_iunlock(ip, dwa.lockmode);
+				return error;
+			}
 
-		if (!xfs_bmap_hw_atomic_write_possible(ip, &dwa.imap,
-				dwa.offset_fsb, orig_end_fsb))
-			goto out_unlock;
-	}
+			end_fsb = dwa.imap.br_startoff +
+					dwa.imap.br_blockcount;
+			dwa.length = XFS_FSB_TO_B(mp, end_fsb) - offset;
+		}
 
-	if (needs_alloc)
-		goto allocate_blocks;
+		needs_alloc = imap_needs_alloc(inode, flags, &dwa.imap,
+				dwa.nimaps);
+
+		if (flags & IOMAP_ATOMIC) {
+			error = -ENOPROTOOPT;
+			/*
+			 * If we allocate less than what is required for the
+			 * write then we may end up with multiple extents,
+			 * which means that REQ_ATOMIC-based cannot be used,
+			 * so avoid this possibility.
+			 */
+			if (needs_alloc &&
+			    orig_end_fsb - dwa.offset_fsb > 1)
+				goto out_unlock;
+			if (!xfs_bmap_hw_atomic_write_possible(ip, &dwa.imap,
+					dwa.offset_fsb, orig_end_fsb))
+				goto out_unlock;
+		}
 
-	/*
-	 * NOWAIT and OVERWRITE I/O needs to span the entire requested I/O with
-	 * a single map so that we avoid partial IO failures due to the rest of
-	 * the I/O range not covered by this map triggering an EAGAIN condition
-	 * when it is subsequently mapped and aborting the I/O.
-	 */
-	if (flags & (IOMAP_NOWAIT | IOMAP_OVERWRITE_ONLY)) {
-		error = -EAGAIN;
-		if (!imap_spans_range(&dwa.imap, dwa.offset_fsb, end_fsb))
-			goto out_unlock;
-	}
+		if (needs_alloc) {
+			if (!dwa.tp)
+				goto alloc_trans;
+
+			/*
+			 * Cap the maximum length we map to a sane size to
+			 * keep the chunks of work done where somewhat
+			 * symmetric with the work writeback does.
+			 *
+			 * Note that the values needs to be less than
+			 * 32-bits wide until the lower level functions
+			 * are updated.
+			 */
+			dwa.length = min_t(loff_t, dwa.length,
+					1024 * PAGE_SIZE);
+			end_fsb = xfs_iomap_end_fsb(mp, offset, dwa.length);
+
+			if (offset + dwa.length > XFS_ISIZE(ip))
+				end_fsb = xfs_iomap_eof_align_last_fsb(ip,
+						end_fsb);
+			else if (dwa.nimaps &&
+				 dwa.imap.br_startblock == HOLESTARTBLOCK)
+				end_fsb = min(end_fsb, dwa.imap.br_startoff +
+						dwa.imap.br_blockcount);
+
+			dwa.count_fsb = end_fsb - dwa.offset_fsb;
+
+			error = xfs_iomap_write_direct(&dwa);
+			if (error)
+				goto out_unlock;
 
-	/*
-	 * For overwrite only I/O, we cannot convert unwritten extents without
-	 * requiring sub-block zeroing.  This can only be done under an
-	 * exclusive IOLOCK, hence return -EAGAIN if this is not a written
-	 * extent to tell the caller to try again.
-	 */
-	if (flags & IOMAP_OVERWRITE_ONLY) {
-		error = -EAGAIN;
-		if (dwa.imap.br_state != XFS_EXT_NORM &&
-		    ((offset | dwa.length) & mp->m_blockmask))
-			goto out_unlock;
-	}
+			error = xfs_trans_commit(dwa.tp);
+			xfs_iunlock(ip, dwa.lockmode);
+			return error;
+		}
 
-	seq = xfs_iomap_inode_sequence(ip, dwa.iomap_flags);
-	xfs_iunlock(ip, dwa.lockmode);
-	trace_xfs_iomap_found(ip, offset, dwa.length, XFS_DATA_FORK,
-			&dwa.imap);
-	return xfs_bmbt_to_iomap(ip, iomap, &dwa.imap, flags,
-			dwa.iomap_flags, seq);
+		/*
+		 * Overwrite path - no allocation needed. Cancel unused
+		 * transaction if allocated and return the mapping.
+		 */
+		if (dwa.tp) {
+			xfs_trans_cancel(dwa.tp);
+			dwa.tp = NULL;
+		}
 
-allocate_blocks:
-	error = -EAGAIN;
-	if (flags & (IOMAP_NOWAIT | IOMAP_OVERWRITE_ONLY))
-		goto out_unlock;
+		/*
+		 * NOWAIT and OVERWRITE I/O needs to span the entire
+		 * requested I/O with a single map so that we avoid
+		 * partial IO failures due to the rest of the I/O range
+		 * not covered by this map triggering an EAGAIN condition
+		 * when it is subsequently mapped and aborting the I/O.
+		 */
+		if (flags & (IOMAP_NOWAIT | IOMAP_OVERWRITE_ONLY)) {
+			error = -EAGAIN;
+			if (!imap_spans_range(&dwa.imap, dwa.offset_fsb,
+					end_fsb))
+				goto out_unlock;
+		}
+		/*
+		 * For overwrite only I/O, we cannot convert unwritten
+		 * extents without requiring sub-block zeroing. This
+		 * can only be done under an exclusive IOLOCK, hence
+		 * return -EAGAIN if this is not a written extent to
+		 * tell the caller to try again.
+		 */
+		if (flags & IOMAP_OVERWRITE_ONLY) {
+			error = -EAGAIN;
+			if (dwa.imap.br_state != XFS_EXT_NORM &&
+			    ((offset | dwa.length) & mp->m_blockmask))
+				goto out_unlock;
+		}
 
-	/*
-	 * We cap the maximum length we map to a sane size  to keep the chunks
-	 * of work done where somewhat symmetric with the work writeback does.
-	 * This is a completely arbitrary number pulled out of thin air as a
-	 * best guess for initial testing.
-	 *
-	 * Note that the values needs to be less than 32-bits wide until the
-	 * lower level functions are updated.
-	 */
-	dwa.length = min_t(loff_t, dwa.length, 1024 * PAGE_SIZE);
-	end_fsb = xfs_iomap_end_fsb(mp, offset, dwa.length);
+		seq = xfs_iomap_inode_sequence(ip, dwa.iomap_flags);
+		xfs_iunlock(ip, dwa.lockmode);
+		trace_xfs_iomap_found(ip, offset, dwa.length, XFS_DATA_FORK,
+				&dwa.imap);
+		return xfs_bmbt_to_iomap(ip, iomap, &dwa.imap, flags,
+				dwa.iomap_flags, seq);
 
-	if (offset + dwa.length > XFS_ISIZE(ip))
-		end_fsb = xfs_iomap_eof_align_last_fsb(ip, end_fsb);
-	else if (dwa.nimaps && dwa.imap.br_startblock == HOLESTARTBLOCK)
-		end_fsb = min(end_fsb, dwa.imap.br_startoff +
-				dwa.imap.br_blockcount);
+alloc_trans:
+		ASSERT(!dwa.tp);
 
-	dwa.count_fsb = end_fsb - dwa.offset_fsb;
-	xfs_iunlock(ip, dwa.lockmode);
+		error = -EAGAIN;
+		if (flags & (IOMAP_NOWAIT | IOMAP_OVERWRITE_ONLY))
+			goto out_unlock;
 
-	return xfs_iomap_write_direct(&dwa);
+		xfs_iunlock(ip, dwa.lockmode);
+		error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write,
+				0, 0, false, &dwa.tp);
+		if (error)
+			return error;
+		dwa.lockmode = XFS_ILOCK_EXCL;
+	} while (dwa.tp);
 
 out_unlock:
+	if (dwa.tp)
+		xfs_trans_cancel(dwa.tp);
 	if (dwa.lockmode)
 		xfs_iunlock(ip, dwa.lockmode);
 	return error;
-- 
2.55.0

[PATCH 31/33] xfs: clean up xfs_direct_write_cow_iomap_begin after restructure

[Dave Chinner]

Now that xfs_direct_write_iomap_begin() handles all locking,
transaction allocation, extent reading, and retry logic, clean up
xfs_direct_write_cow_iomap_begin() to remove the dead transitional
code.

Remove the needs_tp parameter and all the internal lock management,
transaction allocation, retry loop, and local_tp handling. The
function now assumes the caller holds the ILOCK and has already
determined that COW is needed via imap_needs_cow().

Lift the imap_needs_cow() check from cow_iomap_begin up to
xfs_direct_write_iomap_begin(), replacing the xfs_is_cow_inode()
check after the xfs_bmapi_read(). This is safe because
imap_needs_cow() checks xfs_is_cow_inode() internally as its first
test.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_iomap.c | 115 +++++++--------------------------------------
 1 file changed, 16 insertions(+), 99 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index d58ecc05a4d1..4b2860e80d9f 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -892,116 +892,44 @@ xfs_bmap_hw_atomic_write_possible(
 
 /*
  * Handle COW extent allocation and iomap setup for direct writes to reflinked
- * files.
+ * files. The caller holds the ILOCK and has already determined that COW is
+ * needed via imap_needs_cow().
  *
- * Transitional tpp handling:
- *	!tpp = do everything internally using local_tp
- *	tpp & !*tpp = caller did locking, wants -EAGAIN if transaction required
- *	tpp && *tpp = caller did locking and transaction allocation
+ * On return, args->nimaps indicates what the caller should do next:
+ *   nimaps == 0: COW was handled, iomap/srcmap are filled in.
+ *                Caller should commit args->tp, unlock, and return.
+ *   nimaps > 0:  Extent is not shared. Caller should continue with the
+ *                normal IO path using the imap.
  *
- * The caller passes in an imap and nimaps that the COW allocation will fill
- * with the data fork extent mapping. On return, *nimaps indicates whether the
- * caller needs to continue with the normal IO path:
- *
- *   *nimaps == 0: COW was handled, iomap/srcmap are filled in, ILOCK released.
- *                 Caller should return 0 immediately.
- *   *nimaps > 0:  Extent is not shared, imap is valid, ILOCK is still held.
- *                 Caller should continue with the normal IO path.
+ * Returns -EAGAIN if a transaction is needed but args->tp is NULL.
  */
 static int
 xfs_direct_write_cow_iomap_begin(
-	struct xfs_direct_write_args *args,
-	bool			needs_tp)
+	struct xfs_direct_write_args *args)
 {
 	struct xfs_inode	*ip = args->ip;
 	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_trans	*local_tp = NULL;
 	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, args->offset);
 	xfs_fileoff_t		end_fsb = xfs_iomap_end_fsb(mp, args->offset,
 					args->length);
 	int			error;
 	u64			seq;
 
-	if (needs_tp) {
-		args->lockmode = XFS_ILOCK_EXCL;
-
-		error = xfs_ilock_for_iomap(ip, args->flags, &args->lockmode);
-		if (error)
-			return error;
-
-retry:
-		args->nimaps = 1;
-		error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb,
-				&args->imap, &args->nimaps, 0);
-		if (error)
-			goto out_unlock;
-	}
-
-	if (!imap_needs_cow(ip, args->flags, &args->imap, args->nimaps)) {
-		if (local_tp) {
-			xfs_trans_cancel(local_tp);
-			args->tp = NULL;
-		}
-		return 0;
-	}
-
-	error = -EAGAIN;
-	if (args->flags & IOMAP_NOWAIT)
-		goto out_unlock;
-
 	error = xfs_reflink_allocate_cow(args);
-	if (error == -EAGAIN) {
-		ASSERT(!args->tp);
-
-		if (!needs_tp)
-			goto out_unlock;
-
-		/*
-		 * Handle the retry internally. Drop the ILOCK and allocate a
-		 * zero-block reservation transaction, which will re-acquire
-		 * the ILOCK. We cannot determine what extent type will be
-		 * found once we've regained the ILOCK, so the callees will
-		 * use xfs_trans_reserve_more_inode() directly to reserve any
-		 * blocks they require before they start modifications. This
-		 * allows ENOSPC to be returned and the transaction cancelled
-		 * safely if the block reservation cannot be made.
-		 *
-		 * Retry the imap lookup since the extent tree may have changed
-		 * while the ILOCK was not held.
-		 */
-		xfs_iunlock(ip, args->lockmode);
-
-		error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write,
-				0, 0, false, &args->tp);
-		if (error)
-			return error;
-		local_tp = args->tp;
-
-		goto retry;
-	}
 	if (error)
-		goto out_unlock;
-
-	if (local_tp) {
-		error = xfs_trans_commit(local_tp);
-		args->tp = NULL;
-		if (error)
-			goto out_unlock;
-	}
+		return error;
 
 	if (!args->shared)
 		return 0;
 
 	if ((args->flags & IOMAP_ATOMIC) &&
 	    !xfs_bmap_hw_atomic_write_possible(ip, &args->cmap,
-			offset_fsb, end_fsb)) {
-		error = -ENOPROTOOPT;
-		goto out_unlock;
-	}
+			offset_fsb, end_fsb))
+		return -ENOPROTOOPT;
 
 	/*
 	 * COW extent found and allocated. Set up iomap/srcmap and return
-	 * with *nimaps = 0 to tell the caller the COW path is complete.
+	 * with nimaps = 0 to tell the caller the COW path is complete.
 	 */
 	args->nimaps = 0;
 	args->length = XFS_FSB_TO_B(mp,
@@ -1014,22 +942,11 @@ xfs_direct_write_cow_iomap_begin(
 		error = xfs_bmbt_to_iomap(ip, args->srcmap, &args->imap,
 				args->flags, 0, seq);
 		if (error)
-			goto out_unlock;
+			return error;
 	}
 	seq = xfs_iomap_inode_sequence(ip, IOMAP_F_SHARED);
-	if (needs_tp)
-		xfs_iunlock(ip, args->lockmode);
 	return xfs_bmbt_to_iomap(ip, args->iomap, &args->cmap, args->flags,
 			IOMAP_F_SHARED, seq);
-
-out_unlock:
-	if (local_tp) {
-		if (args->tp)
-			xfs_trans_cancel(local_tp);
-		args->tp = NULL;
-		xfs_iunlock(ip, args->lockmode);
-	}
-	return error;
 }
 
 static int
@@ -1094,8 +1011,8 @@ xfs_direct_write_iomap_begin(
 		if (error)
 			goto out_unlock;
 
-		if (xfs_is_cow_inode(ip)) {
-			error = xfs_direct_write_cow_iomap_begin(&dwa, false);
+		if (imap_needs_cow(ip, flags, &dwa.imap, dwa.nimaps)) {
+			error = xfs_direct_write_cow_iomap_begin(&dwa);
 			if (error == -EAGAIN)
 				goto alloc_trans;
 			if (error)
-- 
2.55.0

[PATCH 32/33] xfs: make pNFS block allocation atomic with inode update

[Dave Chinner]

Restructure xfs_fs_map_blocks() to perform the extent allocation
and inode update in a single synchronous transaction, making the
operation atomic with respect to the ILOCK.

Previously the function used two separate transactions: one for
the extent allocation (via xfs_iomap_write_direct) and one for the
inode update (xfs_fs_map_update_inode), followed by an explicit
log force to ensure persistence.

Now the function uses a do {} while (!dwa.tp) loop to allocate a
transaction and re-read the mapping atomically. The inode update
(SUID/SGID strip, timestamps, PREALLOC flag) is performed in the
same transaction as the allocation, and the transaction is committed
synchronously via xfs_trans_set_sync().

xfs_fs_map_update_inode() is converted to take a transaction
parameter and no longer allocates or commits its own transaction.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_pnfs.c | 124 +++++++++++++++++++++++-----------------------
 1 file changed, 61 insertions(+), 63 deletions(-)

diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
index 495e081838d0..37194e62d3cb 100644
--- a/fs/xfs/xfs_pnfs.c
+++ b/fs/xfs/xfs_pnfs.c
@@ -88,29 +88,17 @@ xfs_fs_get_uuid(
  * is from the client to indicate that data has been written and the file size
  * can be extended.
  */
-static int
+static void
 xfs_fs_map_update_inode(
+	struct xfs_trans	*tp,
 	struct xfs_inode	*ip)
 {
-	struct xfs_trans	*tp;
-	int			error;
-
-	error = xfs_trans_alloc(ip->i_mount, &M_RES(ip->i_mount)->tr_writeid,
-			0, 0, 0, &tp);
-	if (error)
-		return error;
-
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
-
 	VFS_I(ip)->i_mode &= ~S_ISUID;
 	if (VFS_I(ip)->i_mode & S_IXGRP)
 		VFS_I(ip)->i_mode &= ~S_ISGID;
 	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
 	ip->i_diflags |= XFS_DIFLAG_PREALLOC;
-
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
-	return xfs_trans_commit(tp);
 }
 
 /*
@@ -127,6 +115,10 @@ xfs_fs_map_blocks(
 {
 	struct xfs_inode	*ip = XFS_I(inode);
 	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_direct_write_args dwa = {
+		.ip		= ip,
+		.iomap		= iomap,
+	};
 	struct xfs_bmbt_irec	imap;
 	xfs_fileoff_t		offset_fsb, end_fsb;
 	loff_t			limit;
@@ -182,61 +174,67 @@ xfs_fs_map_blocks(
 	end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)offset + length);
 	offset_fsb = XFS_B_TO_FSBT(mp, offset);
 
+	/*
+	 * Extent allocation requires a transaction. If we find a hole, drop
+	 * the ILOCK, allocate a transaction and re-read the mapping so we
+	 * don't use a stale imap for determining the allocation.
+	 */
 	lock_flags = xfs_ilock_data_map_shared(ip);
-	/* request mappings for the specified range only */
-	error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb,
-				&imap, &nimaps, 0);
-	if (error) {
-		xfs_iunlock(ip, lock_flags);
-		goto out_unlock;
-	}
-	ASSERT(!nimaps || imap.br_startblock != DELAYSTARTBLOCK);
-
-	if (write && (!nimaps || imap.br_startblock == HOLESTARTBLOCK)) {
-		if (offset + length > XFS_ISIZE(ip))
-			end_fsb = xfs_iomap_eof_align_last_fsb(ip, end_fsb);
-		else if (nimaps && imap.br_startblock == HOLESTARTBLOCK)
-			end_fsb = min(end_fsb, imap.br_startoff +
-					       imap.br_blockcount);
-		xfs_iunlock(ip, lock_flags);
-
-		{
-			struct xfs_direct_write_args args = {
-				.ip		= ip,
-				.offset_fsb	= offset_fsb,
-				.count_fsb	= end_fsb - offset_fsb,
-				.offset		= offset,
-				.length		= length,
-				.imap		= imap,
-				.iomap		= iomap,
-			};
-
-			error = xfs_iomap_write_direct(&args);
-		}
+	do {
+		nimaps = 1;
+		error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb,
+					&imap, &nimaps, 0);
 		if (error)
-			goto out_unlock;
+			goto out_cancel;
+		ASSERT(!nimaps || imap.br_startblock != DELAYSTARTBLOCK);
+
+		if (!write || (nimaps &&
+			       imap.br_startblock != HOLESTARTBLOCK)) {
+			seq = xfs_iomap_inode_sequence(ip, 0);
+			error = xfs_bmbt_to_iomap(ip, iomap, &imap, 0, 0, seq);
+			goto out_cancel;
+		}
 
-		/*
-		 * Ensure the next transaction is committed synchronously so
-		 * that the blocks allocated and handed out to the client are
-		 * guaranteed to be present even after a server crash.
-		 */
-		error = xfs_fs_map_update_inode(ip);
-		if (!error)
-			error = xfs_log_force_inode(ip);
-		if (error)
-			goto out_unlock;
+		if (!dwa.tp) {
+			xfs_iunlock(ip, lock_flags);
+			error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write,
+					0, 0, false, &dwa.tp);
+			if (error)
+				goto out_unlock;
+			lock_flags = XFS_ILOCK_EXCL;
+		}
+	} while (!dwa.tp);
+
+	if (offset + length > XFS_ISIZE(ip))
+		end_fsb = xfs_iomap_eof_align_last_fsb(ip, end_fsb);
+	else if (nimaps && imap.br_startblock == HOLESTARTBLOCK)
+		end_fsb = min(end_fsb, imap.br_startoff + imap.br_blockcount);
+
+	dwa.offset_fsb = offset_fsb;
+	dwa.count_fsb = end_fsb - offset_fsb;
+	dwa.offset = offset;
+	dwa.length = length;
+	dwa.imap = imap;
+	error = xfs_iomap_write_direct(&dwa);
+	if (error)
+		goto out_cancel;
 
-	} else {
-		seq = xfs_iomap_inode_sequence(ip, 0);
-		xfs_iunlock(ip, lock_flags);
-		error = xfs_bmbt_to_iomap(ip, iomap, &imap, 0, 0, seq);
-	}
-	xfs_iunlock(ip, XFS_IOLOCK_EXCL);
-	*device_generation = mp->m_generation;
-	return error;
+	/*
+	 * Update the inode and commit synchronously so that the blocks
+	 * are guaranteed persistent before being handed to the client.
+	 */
+	xfs_fs_map_update_inode(dwa.tp, ip);
+	xfs_trans_set_sync(dwa.tp);
+	error = xfs_trans_commit(dwa.tp);
+	dwa.tp = NULL;
+
+out_cancel:
+	if (dwa.tp)
+		xfs_trans_cancel(dwa.tp);
+	xfs_iunlock(ip, lock_flags);
 out_unlock:
 	xfs_iunlock(ip, XFS_IOLOCK_EXCL);
+	*device_generation = mp->m_generation;
 	return error;
 }
 
-- 
2.55.0

[PATCH 33/33] xfs: remove dead internal transaction path from xfs_iomap_write_direct

[Dave Chinner]

Now that all callers supply a valid transaction via args->tp,
remove the dead internal transaction allocation path from
xfs_iomap_write_direct().

The function is now a pure allocation helper: it reserves blocks
via xfs_trans_reserve_more_inode(), extends the inode extent count,
calls xfs_bmapi_write(), and fills in the iomap. All error paths
return directly since the caller manages the transaction and lock
lifecycle.

The xfs_qm_dqattach() call is replaced with xfs_qm_dqattach_locked()
since the ILOCK is always held on entry.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/xfs_iomap.c | 48 ++++++++--------------------------------------
 1 file changed, 8 insertions(+), 40 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 4b2860e80d9f..4c7b11e4a6a1 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -271,7 +271,6 @@ xfs_iomap_write_direct(
 {
 	struct xfs_inode	*ip = args->ip;
 	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_trans	*local_tp = NULL;
 	xfs_filblks_t		resaligned;
 	int			nimaps;
 	unsigned int		dblocks, rblocks;
@@ -281,6 +280,7 @@ xfs_iomap_write_direct(
 	int			nr_exts = XFS_IEXT_ADD_NOSPLIT_CNT;
 	u64			seq;
 
+	ASSERT(args->tp);
 	ASSERT(args->count_fsb > 0);
 
 	resaligned = xfs_aligned_fsb_count(args->offset_fsb, args->count_fsb,
@@ -293,10 +293,7 @@ xfs_iomap_write_direct(
 		rblocks = 0;
 	}
 
-	if (args->tp)
-		error = xfs_qm_dqattach_locked(ip, false);
-	else
-		error = xfs_qm_dqattach(ip);
+	error = xfs_qm_dqattach_locked(ip, false);
 	if (error)
 		return error;
 
@@ -322,55 +319,26 @@ xfs_iomap_write_direct(
 		}
 	}
 
-	if (args->tp) {
-		error = xfs_trans_reserve_more_inode(args->tp, ip, dblocks,
-				rblocks, force);
-		if (error)
-			return error;
-	} else {
-		error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, dblocks,
-				rblocks, force, &args->tp);
-		if (error)
-			return error;
-		local_tp = args->tp;
-	}
+	error = xfs_trans_reserve_more_inode(args->tp, ip, dblocks, rblocks,
+			force);
+	if (error)
+		return error;
 
 	error = xfs_iext_count_extend(args->tp, ip, XFS_DATA_FORK, nr_exts);
 	if (error)
-		goto out_trans_cancel;
+		return error;
 
 	nimaps = 1;
 	error = xfs_bmapi_write(args->tp, ip, args->offset_fsb, args->count_fsb,
 				bmapi_flags, 0, &args->imap, &nimaps);
 	if (error)
-		goto out_trans_cancel;
+		return error;
 
 	seq = xfs_iomap_inode_sequence(ip, args->iomap_flags);
-	if (local_tp) {
-		error = xfs_trans_commit(args->tp);
-		args->tp = NULL;
-		xfs_iunlock(ip, XFS_ILOCK_EXCL);
-		if (error)
-			return error;
-		if (unlikely(!xfs_valid_startblock(ip,
-				args->imap.br_startblock))) {
-			xfs_bmap_mark_sick(ip, XFS_DATA_FORK);
-			return xfs_alert_fsblock_zero(ip, &args->imap);
-		}
-	}
-
 	trace_xfs_iomap_alloc(ip, args->offset, args->length, XFS_DATA_FORK,
 			&args->imap);
 	return xfs_bmbt_to_iomap(ip, args->iomap, &args->imap, args->flags,
 			args->iomap_flags | IOMAP_F_NEW, seq);
-
-out_trans_cancel:
-	if (local_tp) {
-		xfs_trans_cancel(args->tp);
-		args->tp = NULL;
-		xfs_iunlock(ip, XFS_ILOCK_EXCL);
-	}
-	return error;
 }
 
 STATIC bool
-- 
2.55.0