[PATCH 1/3] xfs: EFI recovery needs it's own transaction reservation

From: Dave Chinner <david@fromorbit.com>
To: linux-xfs@vger.kernel.org
Subject: [PATCH 1/3] xfs: EFI recovery needs it's own transaction reservation
Date: Wed,  9 Sep 2020 18:19:10 +1000	[thread overview]
Message-ID: <20200909081912.1185392-2-david@fromorbit.com> (raw)
In-Reply-To: <20200909081912.1185392-1-david@fromorbit.com>

From: Dave Chinner <dchinner@redhat.com>

Recovering an EFI currently uses a itruncate reservation, which is
designed for a rolling transaction that modifies the BMBT and
logs the EFI in one commit, then frees the space and logs the EFD in
the second commit.

Recovering the EFI only requires the second transaction in this
pair, and hence has a smaller log space requirement than a truncate
operation. Hence when the extent free is being processed at runtime,
the log reservation that is held by the filesystem is only enough to
complete the extent free, not the entire truncate operation.

Hence if the EFI pins the tail of the log and the log fills up while
the extent is being freed, the amount of reserved free space in the
log is not enough to start another entire truncate operation. Hence
if we crash at this point, log recovery will deadlock with the EFI
pinning the tail of the log and the log not having enough free space
to reserve an itruncate transaction.

As such, EFI recovery needs it's own log space reservation separate
to the itruncate reservation. We only need what is required free the
extent, and this matches the space we have reserved at runtime for
this operation and hence should prevent the recovery deadlock from
occurring.

This patch adds the new reservation in a way that minimises the
change such that it should be back-portable to older kernels easily.
Follow up patches will factor and rework the reservations to be more
correct and more tightly defined.

Note: this would appear to be a generic problem with intent
recovery; we use the entire operation reservation for recovery,
not the reservation that was held at runtime after the intent was
logged. I suspect all intents are going to require their own
reservation as a result.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_trans_resv.c | 10 ++++++++++
 fs/xfs/libxfs/xfs_trans_resv.h |  2 ++
 fs/xfs/xfs_extfree_item.c      |  2 +-
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index d1a0848cb52e..da2ec052ac0a 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -916,6 +916,16 @@ xfs_trans_resv_calc(
 		resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT;
 	resp->tr_qm_dqalloc.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
 
+	/*
+	 * Log recovery reservations for intent replay
+	 *
+	 * EFI recovery is itruncate minus the initial transaction that logs
+	 * logs the EFI.
+	 */
+	resp->tr_efi.tr_logres = resp->tr_itruncate.tr_logres;
+	resp->tr_efi.tr_logcount = resp->tr_itruncate.tr_logcount - 1;
+	resp->tr_efi.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
+
 	/*
 	 * The following transactions are logged in logical format with
 	 * a default log count.
diff --git a/fs/xfs/libxfs/xfs_trans_resv.h b/fs/xfs/libxfs/xfs_trans_resv.h
index 7241ab28cf84..13173b3eaac9 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.h
+++ b/fs/xfs/libxfs/xfs_trans_resv.h
@@ -50,6 +50,8 @@ struct xfs_trans_resv {
 	struct xfs_trans_res	tr_qm_equotaoff;/* end of turn quota off */
 	struct xfs_trans_res	tr_sb;		/* modify superblock */
 	struct xfs_trans_res	tr_fsyncts;	/* update timestamps on fsync */
+	struct xfs_trans_res	tr_efi;		/* EFI log item recovery */
+
 };
 
 /* shorthand way of accessing reservation structure */
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 6cb8cd11072a..1ea9ab4cd44e 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -618,7 +618,7 @@ xfs_efi_item_recover(
 		}
 	}
 
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_efi, 0, 0, 0, &tp);
 	if (error)
 		return error;
 	efdp = xfs_trans_get_efd(tp, efip, efip->efi_format.efi_nextents);
-- 
2.28.0

