All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Darrick J. Wong" <djwong@kernel.org>
To: djwong@kernel.org, david@fromorbit.com
Cc: Dave Chinner <dchinner@redhat.com>, linux-xfs@vger.kernel.org
Subject: [PATCH 2/9] xfs: stop artificially limiting the length of bunmap calls
Date: Tue, 26 Apr 2022 17:52:03 -0700	[thread overview]
Message-ID: <165102072359.3922658.10110553526152188988.stgit@magnolia> (raw)
In-Reply-To: <165102071223.3922658.5241787533081256670.stgit@magnolia>

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

In commit e1a4e37cc7b6, we clamped the length of bunmapi calls on the
data forks of shared files to avoid two failure scenarios: one where the
extent being unmapped is so sparsely shared that we exceed the
transaction reservation with the sheer number of refcount btree updates
and EFI intent items; and the other where we attach so many deferred
updates to the transaction that we pin the log tail and later the log
head meets the tail, causing the log to livelock.

We avoid triggering the first problem by tracking the number of ops in
the refcount btree cursor and forcing a requeue of the refcount intent
item any time we think that we might be close to overflowing.  This has
been baked into XFS since before the original e1a4 patch.

A recent patchset fixed the second problem by changing the deferred ops
code to finish all the work items created by each round of trying to
complete a refcount intent item, which eliminates the long chains of
deferred items (27dad); and causing long-running transactions to relog
their intent log items when space in the log gets low (74f4d).

Because this clamp affects /any/ unmapping request regardless of the
sharing factors of the component blocks, it degrades the performance of
all large unmapping requests -- whereas with an unshared file we can
unmap millions of blocks in one go, shared files are limited to
unmapping a few thousand blocks at a time, which causes the upper level
code to spin in a bunmapi loop even if it wasn't needed.

This also eliminates one more place where log recovery behavior can
differ from online behavior, because bunmapi operations no longer need
to requeue.  The fstest generic/447 was created to test the old fix, and
it still passes with this applied.

Partial-revert-of: e1a4e37cc7b6 ("xfs: try to avoid blowing out the transaction reservation when bunmaping a shared extent")
Depends: 27dada070d59 ("xfs: change the order in which child and parent defer ops ar finished")
Depends: 74f4d6a1e065 ("xfs: only relog deferred intent items if free space in the log gets low")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.c |   22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 24462bdfd8e7..6833110d1bd4 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5280,7 +5280,6 @@ __xfs_bunmapi(
 	int			whichfork;	/* data or attribute fork */
 	xfs_fsblock_t		sum;
 	xfs_filblks_t		len = *rlen;	/* length to unmap in file */
-	xfs_fileoff_t		max_len;
 	xfs_fileoff_t		end;
 	struct xfs_iext_cursor	icur;
 	bool			done = false;
@@ -5299,16 +5298,6 @@ __xfs_bunmapi(
 	ASSERT(len > 0);
 	ASSERT(nexts >= 0);
 
-	/*
-	 * Guesstimate how many blocks we can unmap without running the risk of
-	 * blowing out the transaction with a mix of EFIs and reflink
-	 * adjustments.
-	 */
-	if (tp && xfs_is_reflink_inode(ip) && whichfork == XFS_DATA_FORK)
-		max_len = min(len, xfs_refcount_max_unmap(tp->t_log_res));
-	else
-		max_len = len;
-
 	error = xfs_iread_extents(tp, ip, whichfork);
 	if (error)
 		return error;
@@ -5347,7 +5336,7 @@ __xfs_bunmapi(
 
 	extno = 0;
 	while (end != (xfs_fileoff_t)-1 && end >= start &&
-	       (nexts == 0 || extno < nexts) && max_len > 0) {
+	       (nexts == 0 || extno < nexts)) {
 		/*
 		 * Is the found extent after a hole in which end lives?
 		 * Just back up to the previous extent, if so.
@@ -5381,14 +5370,6 @@ __xfs_bunmapi(
 		if (del.br_startoff + del.br_blockcount > end + 1)
 			del.br_blockcount = end + 1 - del.br_startoff;
 
-		/* How much can we safely unmap? */
-		if (max_len < del.br_blockcount) {
-			del.br_startoff += del.br_blockcount - max_len;
-			if (!wasdel)
-				del.br_startblock += del.br_blockcount - max_len;
-			del.br_blockcount = max_len;
-		}
-
 		if (!isrt)
 			goto delete;
 
@@ -5524,7 +5505,6 @@ __xfs_bunmapi(
 		if (error)
 			goto error0;
 
-		max_len -= del.br_blockcount;
 		end = del.br_startoff - 1;
 nodelete:
 		/*


  parent reply	other threads:[~2022-04-27  0:52 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-27  0:51 [PATCHSET v2 0/9] xfs: fix reflink inefficiencies Darrick J. Wong
2022-04-27  0:51 ` [PATCH 1/9] xfs: count EFIs when deciding to ask for a continuation of a refcount update Darrick J. Wong
2022-04-27 22:59   ` Darrick J. Wong
2022-04-28 12:46   ` Christoph Hellwig
2022-04-27  0:52 ` Darrick J. Wong [this message]
2022-04-28 12:46   ` [PATCH 2/9] xfs: stop artificially limiting the length of bunmap calls Christoph Hellwig
2022-04-27  0:52 ` [PATCH 3/9] xfs: remove a __xfs_bunmapi call from reflink Darrick J. Wong
2022-04-27  0:52 ` [PATCH 4/9] xfs: create shadow transaction reservations for computing minimum log size Darrick J. Wong
2022-04-27  4:28   ` Dave Chinner
2022-04-28 12:50   ` Christoph Hellwig
2022-04-27  0:52 ` [PATCH 5/9] xfs: report "max_resp" used for min log size computation Darrick J. Wong
2022-04-27  4:28   ` Dave Chinner
2022-04-28 12:51   ` Christoph Hellwig
2022-04-27  0:52 ` [PATCH 6/9] xfs: reduce the absurdly large log operation count Darrick J. Wong
2022-04-27  4:29   ` Dave Chinner
2022-04-28 12:54   ` Christoph Hellwig
2022-04-27  0:52 ` [PATCH 7/9] xfs: reduce transaction reservations with reflink Darrick J. Wong
2022-04-27  4:30   ` Dave Chinner
2022-04-28 12:55   ` Christoph Hellwig
2022-04-27  0:52 ` [PATCH 8/9] xfs: rewrite xfs_reflink_end_cow to use intents Darrick J. Wong
2022-04-28 12:56   ` Christoph Hellwig
2022-04-27  0:52 ` [PATCH 9/9] xfs: rename xfs_*alloc*_log_count to _block_count Darrick J. Wong
2022-04-27  4:32   ` Dave Chinner
2022-04-28 12:56   ` Christoph Hellwig

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=165102072359.3922658.10110553526152188988.stgit@magnolia \
    --to=djwong@kernel.org \
    --cc=david@fromorbit.com \
    --cc=dchinner@redhat.com \
    --cc=linux-xfs@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.