Re: [PATCH 3/5] xfs: restrict when we try to align cow fork delalloc to cowextsz hints

[Christoph Hellwig <hch@lst.de>]

On Thu, Jun 13, 2024 at 09:13:10PM -0700, Darrick J. Wong wrote:
> > Looking at the caller below I don't think we need the use_cowextszhint
> > flag here, we can just locally check for prealloc beeing non-0 in
> > the branch below:
> 
> That won't work, because xfs_buffered_write_iomap_begin only sets
> @prealloc to nonzero if it thinks is an extending write.  For the cow
> fork, we create delalloc reservations that are aligned to the cowextsize
> value for overwrites below eof.

Yeah.  For that we'd need to move the retry loop into
xfs_bmapi_reserve_delalloc - which honestly feels like the more logical
place for it anyway.  As in the untested version below, also note
my XXX comment about a comment being added:

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index c101cf266bc4db..58ff21cb84e0f5 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4059,19 +4059,33 @@ xfs_bmapi_reserve_delalloc(
 	uint64_t		fdblocks;
 	int			error;
 	xfs_fileoff_t		aoff = off;
+	bool			use_cowextszhint =
+		whichfork == XFS_COW_FORK && !prealloc;
 
 	/*
 	 * Cap the alloc length. Keep track of prealloc so we know whether to
 	 * tag the inode before we return.
 	 */
+retry:
 	alen = XFS_FILBLKS_MIN(len + prealloc, XFS_MAX_BMBT_EXTLEN);
 	if (!eof)
 		alen = XFS_FILBLKS_MIN(alen, got->br_startoff - aoff);
 	if (prealloc && alen >= len)
 		prealloc = alen - len;
 
-	/* Figure out the extent size, adjust alen */
-	if (whichfork == XFS_COW_FORK) {
+	/*
+	 * If we're targeting the COW fork but aren't creating a speculative
+	 * posteof preallocation, try to expand the reservation to align with
+	 * the cow extent size hint if there's sufficient free space.
+	 *
+	 * Unlike the data fork, the CoW cancellation functions will free all
+	 * the reservations at inactivation, so we don't require that every
+	 * delalloc reservation have a dirty pagecache.
+	 *
+	 * XXX(hch): I can't see where we actually require dirty pagecache
+	 * for speculative data fork preallocations.  What am I missing?
+	 */
+	if (use_cowextszhint) {
 		struct xfs_bmbt_irec	prev;
 		xfs_extlen_t		extsz = xfs_get_cowextsz_hint(ip);
 
@@ -4090,7 +4104,7 @@ xfs_bmapi_reserve_delalloc(
 	 */
 	error = xfs_quota_reserve_blkres(ip, alen);
 	if (error)
-		return error;
+		goto out;
 
 	/*
 	 * Split changing sb for alen and indlen since they could be coming
@@ -4140,6 +4154,16 @@ xfs_bmapi_reserve_delalloc(
 out_unreserve_quota:
 	if (XFS_IS_QUOTA_ON(mp))
 		xfs_quota_unreserve_blkres(ip, alen);
+out:
+	if (error == -ENOSPC || error == -EDQUOT) {
+		trace_xfs_delalloc_enospc(ip, off, len);
+		if (prealloc || use_cowextszhint) {
+			/* retry without any preallocation */
+			prealloc = 0;
+			use_cowextszhint = false;
+			goto retry;
+		}
+	}
 	return error;
 }
 
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 3783426739258c..34cce017fe7ce1 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1148,27 +1148,13 @@ xfs_buffered_write_iomap_begin(
 		}
 	}
 
-retry:
 	error = xfs_bmapi_reserve_delalloc(ip, allocfork, offset_fsb,
 			end_fsb - offset_fsb, prealloc_blocks,
 			allocfork == XFS_DATA_FORK ? &imap : &cmap,
 			allocfork == XFS_DATA_FORK ? &icur : &ccur,
 			allocfork == XFS_DATA_FORK ? eof : cow_eof);
-	switch (error) {
-	case 0:
-		break;
-	case -ENOSPC:
-	case -EDQUOT:
-		/* retry without any preallocation */
-		trace_xfs_delalloc_enospc(ip, offset, count);
-		if (prealloc_blocks) {
-			prealloc_blocks = 0;
-			goto retry;
-		}
-		fallthrough;
-	default:
+	if (error)
 		goto out_unlock;
-	}
 
 	if (allocfork == XFS_COW_FORK) {
 		trace_xfs_iomap_alloc(ip, offset, count, allocfork, &cmap);