public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: stable@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	patches@lists.linux.dev, linux-xfs@vger.kernel.org,
	"Darrick J. Wong" <djwong@kernel.org>,
	Christoph Hellwig <hch@lst.de>,
	Chandan Babu R <chandanbabu@kernel.org>,
	Catherine Hoang <catherine.hoang@oracle.com>
Subject: [PATCH 6.6 044/124] xfs: restrict when we try to align cow fork delalloc to cowextsz hints
Date: Mon, 21 Oct 2024 12:24:08 +0200	[thread overview]
Message-ID: <20241021102258.434209222@linuxfoundation.org> (raw)
In-Reply-To: <20241021102256.706334758@linuxfoundation.org>

6.6-stable review patch.  If anyone has any objections, please let me know.

------------------

From: "Darrick J. Wong" <djwong@kernel.org>

commit 288e1f693f04e66be99f27e7cbe4a45936a66745 upstream.

xfs/205 produces the following failure when always_cow is enabled:

#  --- a/tests/xfs/205.out	2024-02-28 16:20:24.437887970 -0800
#  +++ b/tests/xfs/205.out.bad	2024-06-03 21:13:40.584000000 -0700
#  @@ -1,4 +1,5 @@
#   QA output created by 205
#   *** one file
#  +   !!! disk full (expected)
#   *** one file, a few bytes at a time
#   *** done

This is the result of overly aggressive attempts to align cow fork
delalloc reservations to the CoW extent size hint.  Looking at the trace
data, we're trying to append a single fsblock to the "fred" file.
Trying to create a speculative post-eof reservation fails because
there's not enough space.

We then set @prealloc_blocks to zero and try again, but the cowextsz
alignment code triggers, which expands our request for a 1-fsblock
reservation into a 39-block reservation.  There's not enough space for
that, so the whole write fails with ENOSPC even though there's
sufficient space in the filesystem to allocate the single block that we
need to land the write.

There are two things wrong here -- first, we shouldn't be attempting
speculative preallocations beyond what was requested when we're low on
space.  Second, if we've already computed a posteof preallocation, we
shouldn't bother trying to align that to the cowextsize hint.

Fix both of these problems by adding a flag that only enables the
expansion of the delalloc reservation to the cowextsize if we're doing a
non-extending write, and only if we're not doing an ENOSPC retry.  This
requires us to move the ENOSPC retry logic to xfs_bmapi_reserve_delalloc.

I probably should have caught this six years ago when 6ca30729c206d was
being reviewed, but oh well.  Update the comments to reflect what the
code does now.

Fixes: 6ca30729c206d ("xfs: bmap code cleanup")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Chandan Babu R <chandanbabu@kernel.org>
Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
Acked-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 fs/xfs/libxfs/xfs_bmap.c |   31 +++++++++++++++++++++++++++----
 fs/xfs/xfs_iomap.c       |   34 ++++++++++++----------------------
 2 files changed, 39 insertions(+), 26 deletions(-)

--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3974,20 +3974,32 @@ xfs_bmapi_reserve_delalloc(
 	xfs_extlen_t		alen;
 	xfs_extlen_t		indlen;
 	int			error;
-	xfs_fileoff_t		aoff = off;
+	xfs_fileoff_t		aoff;
+	bool			use_cowextszhint =
+					whichfork == XFS_COW_FORK && !prealloc;
 
+retry:
 	/*
 	 * Cap the alloc length. Keep track of prealloc so we know whether to
 	 * tag the inode before we return.
 	 */
+	aoff = off;
 	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 targetting 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.
+	 */
+	if (use_cowextszhint) {
 		struct xfs_bmbt_irec	prev;
 		xfs_extlen_t		extsz = xfs_get_cowextsz_hint(ip);
 
@@ -4006,7 +4018,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
@@ -4051,6 +4063,17 @@ out_unreserve_blocks:
 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 */
+			use_cowextszhint = false;
+			prealloc = 0;
+			goto retry;
+		}
+	}
 	return error;
 }
 
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1127,33 +1127,23 @@ 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:
-		goto out_unlock;
-	}
-
 	if (allocfork == XFS_COW_FORK) {
+		error = xfs_bmapi_reserve_delalloc(ip, allocfork, offset_fsb,
+				end_fsb - offset_fsb, prealloc_blocks, &cmap,
+				&ccur, cow_eof);
+		if (error)
+			goto out_unlock;
+
 		trace_xfs_iomap_alloc(ip, offset, count, allocfork, &cmap);
 		goto found_cow;
 	}
 
+	error = xfs_bmapi_reserve_delalloc(ip, allocfork, offset_fsb,
+			end_fsb - offset_fsb, prealloc_blocks, &imap, &icur,
+			eof);
+	if (error)
+		goto out_unlock;
+
 	/*
 	 * Flag newly allocated delalloc blocks with IOMAP_F_NEW so we punch
 	 * them out if the write happens to fail.



      parent reply	other threads:[~2024-10-21 10:36 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20241021102256.706334758@linuxfoundation.org>
2024-10-21 10:23 ` [PATCH 6.6 024/124] xfs: fix error returns from xfs_bmapi_write Greg Kroah-Hartman
2024-10-21 10:23 ` [PATCH 6.6 025/124] xfs: fix xfs_bmap_add_extent_delay_real for partial conversions Greg Kroah-Hartman
2024-10-21 10:23 ` [PATCH 6.6 026/124] xfs: remove a racy if_bytes check in xfs_reflink_end_cow_extent Greg Kroah-Hartman
2024-10-21 10:23 ` [PATCH 6.6 027/124] xfs: require XFS_SB_FEAT_INCOMPAT_LOG_XATTRS for attr log intent item recovery Greg Kroah-Hartman
2024-10-21 10:23 ` [PATCH 6.6 028/124] xfs: check opcode and iovec count match in xlog_recover_attri_commit_pass2 Greg Kroah-Hartman
2024-10-21 10:23 ` [PATCH 6.6 029/124] xfs: fix missing check for invalid attr flags Greg Kroah-Hartman
2024-10-21 10:23 ` [PATCH 6.6 030/124] xfs: check shortform attr entry flags specifically Greg Kroah-Hartman
2024-10-21 10:23 ` [PATCH 6.6 031/124] xfs: validate recovered name buffers when recovering xattr items Greg Kroah-Hartman
2024-10-21 10:23 ` [PATCH 6.6 032/124] xfs: enforce one namespace per attribute Greg Kroah-Hartman
2024-10-21 10:23 ` [PATCH 6.6 033/124] xfs: revert commit 44af6c7e59b12 Greg Kroah-Hartman
2024-10-21 10:23 ` [PATCH 6.6 034/124] xfs: use dontcache for grabbing inodes during scrub Greg Kroah-Hartman
2024-10-21 10:23 ` [PATCH 6.6 035/124] xfs: match lock mode in xfs_buffered_write_iomap_begin() Greg Kroah-Hartman
2024-10-21 10:24 ` [PATCH 6.6 036/124] xfs: make the seq argument to xfs_bmapi_convert_delalloc() optional Greg Kroah-Hartman
2024-10-21 10:24 ` [PATCH 6.6 037/124] xfs: make xfs_bmapi_convert_delalloc() to allocate the target offset Greg Kroah-Hartman
2024-10-21 10:24 ` [PATCH 6.6 038/124] xfs: convert delayed extents to unwritten when zeroing post eof blocks Greg Kroah-Hartman
2024-10-21 10:24 ` [PATCH 6.6 039/124] xfs: allow symlinks with short remote targets Greg Kroah-Hartman
2024-10-21 10:24 ` [PATCH 6.6 040/124] xfs: make sure sb_fdblocks is non-negative Greg Kroah-Hartman
2024-10-21 10:24 ` [PATCH 6.6 041/124] xfs: fix unlink vs cluster buffer instantiation race Greg Kroah-Hartman
2024-10-21 10:24 ` [PATCH 6.6 042/124] xfs: fix freeing speculative preallocations for preallocated files Greg Kroah-Hartman
2024-10-21 10:24 ` [PATCH 6.6 043/124] xfs: allow unlinked symlinks and dirs with zero size Greg Kroah-Hartman
2024-10-21 10:24 ` Greg Kroah-Hartman [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20241021102258.434209222@linuxfoundation.org \
    --to=gregkh@linuxfoundation.org \
    --cc=catherine.hoang@oracle.com \
    --cc=chandanbabu@kernel.org \
    --cc=djwong@kernel.org \
    --cc=hch@lst.de \
    --cc=linux-xfs@vger.kernel.org \
    --cc=patches@lists.linux.dev \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox