[PATCHSET RFC achender 0/2] xfs: refactor log recovery resource capture

[PATCHSET RFC achender 0/2] xfs: refactor log recovery resource capture

[Darrick J. Wong]

Hi all,

During review of Allison's logged xattrs patchset last cycle, I noticed
that there was an opportunity to clean up some code structure
differences between how regular runtime deferred attributes hold on to
resources across a transaction roll, and how it's done during log
recovery.  This series, in cleaning that up, should shorten her
patchset and simplify it a bit.

During regular operation, transactions are allowed to hold up to two
inodes and two buffers across a transaction roll to finish deferred log
items.  This implies that log recovery of a log intent item ought to be
able to do the same.  However, current log recovery code open-codes
saving only a single inode, because that was all that was required.

With atomic extent swapping and logged extended attributes upon us, it
has become evident that we need to use the same runtime mechanisms
during recovery.  Refactor the deferred ops code to use the same
resource capture mechanisms for both.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=log-recovery-defer-capture-5.16
---
 fs/xfs/libxfs/xfs_defer.c  |  171 +++++++++++++++++++++++++++++++-------------
 fs/xfs/libxfs/xfs_defer.h  |   38 ++++++++--
 fs/xfs/xfs_bmap_item.c     |    2 -
 fs/xfs/xfs_extfree_item.c  |    2 -
 fs/xfs/xfs_log_recover.c   |   12 +--
 fs/xfs/xfs_refcount_item.c |    2 -
 fs/xfs/xfs_rmap_item.c     |    2 -
 fs/xfs/xfs_trans.h         |    6 --
 8 files changed, 157 insertions(+), 78 deletions(-)

[PATCH 1/2] xfs: formalize the process of holding onto resources across a defer roll

[Darrick J. Wong]

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

Transaction users are allowed to flag up to two buffers and two inodes
for ownership preservation across a deferred transaction roll.  Hoist
the variables and code responsible for this out of xfs_defer_trans_roll
so that we can use it for the defer capture mechanism.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_defer.c |   85 +++++++++++++++++++++++++++++----------------
 fs/xfs/libxfs/xfs_defer.h |   24 +++++++++++++
 fs/xfs/xfs_trans.h        |    6 ---
 3 files changed, 78 insertions(+), 37 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index eff4a127188e..7c6490f3e537 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -232,23 +232,20 @@ xfs_defer_trans_abort(
 	}
 }
 
-/* Roll a transaction so we can do some deferred op processing. */
-STATIC int
-xfs_defer_trans_roll(
-	struct xfs_trans		**tpp)
+/*
+ * Capture resources that the caller said not to release ("held") when the
+ * transaction commits.  Caller is responsible for zero-initializing @dres.
+ */
+static int
+xfs_defer_save_resources(
+	struct xfs_defer_resources	*dres,
+	struct xfs_trans		*tp)
 {
-	struct xfs_trans		*tp = *tpp;
 	struct xfs_buf_log_item		*bli;
 	struct xfs_inode_log_item	*ili;
 	struct xfs_log_item		*lip;
-	struct xfs_buf			*bplist[XFS_DEFER_OPS_NR_BUFS];
-	struct xfs_inode		*iplist[XFS_DEFER_OPS_NR_INODES];
-	unsigned int			ordered = 0; /* bitmap */
-	int				bpcount = 0, ipcount = 0;
-	int				i;
-	int				error;
 
-	BUILD_BUG_ON(NBBY * sizeof(ordered) < XFS_DEFER_OPS_NR_BUFS);
+	BUILD_BUG_ON(NBBY * sizeof(dres->dr_ordered) < XFS_DEFER_OPS_NR_BUFS);
 
 	list_for_each_entry(lip, &tp->t_items, li_trans) {
 		switch (lip->li_type) {
@@ -256,28 +253,29 @@ xfs_defer_trans_roll(
 			bli = container_of(lip, struct xfs_buf_log_item,
 					   bli_item);
 			if (bli->bli_flags & XFS_BLI_HOLD) {
-				if (bpcount >= XFS_DEFER_OPS_NR_BUFS) {
+				if (dres->dr_bufs >= XFS_DEFER_OPS_NR_BUFS) {
 					ASSERT(0);
 					return -EFSCORRUPTED;
 				}
 				if (bli->bli_flags & XFS_BLI_ORDERED)
-					ordered |= (1U << bpcount);
+					dres->dr_ordered |=
+							(1U << dres->dr_bufs);
 				else
 					xfs_trans_dirty_buf(tp, bli->bli_buf);
-				bplist[bpcount++] = bli->bli_buf;
+				dres->dr_bp[dres->dr_bufs++] = bli->bli_buf;
 			}
 			break;
 		case XFS_LI_INODE:
 			ili = container_of(lip, struct xfs_inode_log_item,
 					   ili_item);
 			if (ili->ili_lock_flags == 0) {
-				if (ipcount >= XFS_DEFER_OPS_NR_INODES) {
+				if (dres->dr_inos >= XFS_DEFER_OPS_NR_INODES) {
 					ASSERT(0);
 					return -EFSCORRUPTED;
 				}
 				xfs_trans_log_inode(tp, ili->ili_inode,
 						    XFS_ILOG_CORE);
-				iplist[ipcount++] = ili->ili_inode;
+				dres->dr_ip[dres->dr_inos++] = ili->ili_inode;
 			}
 			break;
 		default:
@@ -285,7 +283,43 @@ xfs_defer_trans_roll(
 		}
 	}
 
-	trace_xfs_defer_trans_roll(tp, _RET_IP_);
+	return 0;
+}
+
+/* Attach the held resources to the transaction. */
+static void
+xfs_defer_restore_resources(
+	struct xfs_trans		*tp,
+	struct xfs_defer_resources	*dres)
+{
+	unsigned short			i;
+
+	/* Rejoin the joined inodes. */
+	for (i = 0; i < dres->dr_inos; i++)
+		xfs_trans_ijoin(tp, dres->dr_ip[i], 0);
+
+	/* Rejoin the buffers and dirty them so the log moves forward. */
+	for (i = 0; i < dres->dr_bufs; i++) {
+		xfs_trans_bjoin(tp, dres->dr_bp[i]);
+		if (dres->dr_ordered & (1U << i))
+			xfs_trans_ordered_buf(tp, dres->dr_bp[i]);
+		xfs_trans_bhold(tp, dres->dr_bp[i]);
+	}
+}
+
+/* Roll a transaction so we can do some deferred op processing. */
+STATIC int
+xfs_defer_trans_roll(
+	struct xfs_trans		**tpp)
+{
+	struct xfs_defer_resources	dres = { };
+	int				error;
+
+	error = xfs_defer_save_resources(&dres, *tpp);
+	if (error)
+		return error;
+
+	trace_xfs_defer_trans_roll(*tpp, _RET_IP_);
 
 	/*
 	 * Roll the transaction.  Rolling always given a new transaction (even
@@ -295,22 +329,11 @@ xfs_defer_trans_roll(
 	 * happened.
 	 */
 	error = xfs_trans_roll(tpp);
-	tp = *tpp;
 
-	/* Rejoin the joined inodes. */
-	for (i = 0; i < ipcount; i++)
-		xfs_trans_ijoin(tp, iplist[i], 0);
-
-	/* Rejoin the buffers and dirty them so the log moves forward. */
-	for (i = 0; i < bpcount; i++) {
-		xfs_trans_bjoin(tp, bplist[i]);
-		if (ordered & (1U << i))
-			xfs_trans_ordered_buf(tp, bplist[i]);
-		xfs_trans_bhold(tp, bplist[i]);
-	}
+	xfs_defer_restore_resources(*tpp, &dres);
 
 	if (error)
-		trace_xfs_defer_trans_roll_error(tp, error);
+		trace_xfs_defer_trans_roll_error(*tpp, error);
 	return error;
 }
 
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index 05472f71fffe..e095abb96f1a 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -64,6 +64,30 @@ extern const struct xfs_defer_op_type xfs_rmap_update_defer_type;
 extern const struct xfs_defer_op_type xfs_extent_free_defer_type;
 extern const struct xfs_defer_op_type xfs_agfl_free_defer_type;
 
+/*
+ * Deferred operation item relogging limits.
+ */
+#define XFS_DEFER_OPS_NR_INODES	2	/* join up to two inodes */
+#define XFS_DEFER_OPS_NR_BUFS	2	/* join up to two buffers */
+
+/* Resources that must be held across a transaction roll. */
+struct xfs_defer_resources {
+	/* held buffers */
+	struct xfs_buf		*dr_bp[XFS_DEFER_OPS_NR_BUFS];
+
+	/* inodes with no unlock flags */
+	struct xfs_inode	*dr_ip[XFS_DEFER_OPS_NR_INODES];
+
+	/* number of held buffers */
+	unsigned short		dr_bufs;
+
+	/* bitmap of ordered buffers */
+	unsigned short		dr_ordered;
+
+	/* number of held inodes */
+	unsigned short		dr_inos;
+};
+
 /*
  * This structure enables a dfops user to detach the chain of deferred
  * operations from a transaction so that they can be continued later.
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 50da47f23a07..3d2e89c4d446 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -112,12 +112,6 @@ void	xfs_log_item_init(struct xfs_mount *mp, struct xfs_log_item *item,
 #define XFS_ITEM_LOCKED		2
 #define XFS_ITEM_FLUSHING	3
 
-/*
- * Deferred operation item relogging limits.
- */
-#define XFS_DEFER_OPS_NR_INODES	2	/* join up to two inodes */
-#define XFS_DEFER_OPS_NR_BUFS	2	/* join up to two buffers */
-
 /*
  * This is the structure maintained for every active transaction.
  */

Re: [PATCH 1/2] xfs: formalize the process of holding onto resources across a defer roll

[Allison Henderson]

On 9/17/21 6:30 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Transaction users are allowed to flag up to two buffers and two inodes
> for ownership preservation across a deferred transaction roll.  Hoist
> the variables and code responsible for this out of xfs_defer_trans_roll
> so that we can use it for the defer capture mechanism.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Ok, makes sense
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>   fs/xfs/libxfs/xfs_defer.c |   85 +++++++++++++++++++++++++++++----------------
>   fs/xfs/libxfs/xfs_defer.h |   24 +++++++++++++
>   fs/xfs/xfs_trans.h        |    6 ---
>   3 files changed, 78 insertions(+), 37 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index eff4a127188e..7c6490f3e537 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -232,23 +232,20 @@ xfs_defer_trans_abort(
>   	}
>   }
>   
> -/* Roll a transaction so we can do some deferred op processing. */
> -STATIC int
> -xfs_defer_trans_roll(
> -	struct xfs_trans		**tpp)
> +/*
> + * Capture resources that the caller said not to release ("held") when the
> + * transaction commits.  Caller is responsible for zero-initializing @dres.
> + */
> +static int
> +xfs_defer_save_resources(
> +	struct xfs_defer_resources	*dres,
> +	struct xfs_trans		*tp)
>   {
> -	struct xfs_trans		*tp = *tpp;
>   	struct xfs_buf_log_item		*bli;
>   	struct xfs_inode_log_item	*ili;
>   	struct xfs_log_item		*lip;
> -	struct xfs_buf			*bplist[XFS_DEFER_OPS_NR_BUFS];
> -	struct xfs_inode		*iplist[XFS_DEFER_OPS_NR_INODES];
> -	unsigned int			ordered = 0; /* bitmap */
> -	int				bpcount = 0, ipcount = 0;
> -	int				i;
> -	int				error;
>   
> -	BUILD_BUG_ON(NBBY * sizeof(ordered) < XFS_DEFER_OPS_NR_BUFS);
> +	BUILD_BUG_ON(NBBY * sizeof(dres->dr_ordered) < XFS_DEFER_OPS_NR_BUFS);
>   
>   	list_for_each_entry(lip, &tp->t_items, li_trans) {
>   		switch (lip->li_type) {
> @@ -256,28 +253,29 @@ xfs_defer_trans_roll(
>   			bli = container_of(lip, struct xfs_buf_log_item,
>   					   bli_item);
>   			if (bli->bli_flags & XFS_BLI_HOLD) {
> -				if (bpcount >= XFS_DEFER_OPS_NR_BUFS) {
> +				if (dres->dr_bufs >= XFS_DEFER_OPS_NR_BUFS) {
>   					ASSERT(0);
>   					return -EFSCORRUPTED;
>   				}
>   				if (bli->bli_flags & XFS_BLI_ORDERED)
> -					ordered |= (1U << bpcount);
> +					dres->dr_ordered |=
> +							(1U << dres->dr_bufs);
>   				else
>   					xfs_trans_dirty_buf(tp, bli->bli_buf);
> -				bplist[bpcount++] = bli->bli_buf;
> +				dres->dr_bp[dres->dr_bufs++] = bli->bli_buf;
>   			}
>   			break;
>   		case XFS_LI_INODE:
>   			ili = container_of(lip, struct xfs_inode_log_item,
>   					   ili_item);
>   			if (ili->ili_lock_flags == 0) {
> -				if (ipcount >= XFS_DEFER_OPS_NR_INODES) {
> +				if (dres->dr_inos >= XFS_DEFER_OPS_NR_INODES) {
>   					ASSERT(0);
>   					return -EFSCORRUPTED;
>   				}
>   				xfs_trans_log_inode(tp, ili->ili_inode,
>   						    XFS_ILOG_CORE);
> -				iplist[ipcount++] = ili->ili_inode;
> +				dres->dr_ip[dres->dr_inos++] = ili->ili_inode;
>   			}
>   			break;
>   		default:
> @@ -285,7 +283,43 @@ xfs_defer_trans_roll(
>   		}
>   	}
>   
> -	trace_xfs_defer_trans_roll(tp, _RET_IP_);
> +	return 0;
> +}
> +
> +/* Attach the held resources to the transaction. */
> +static void
> +xfs_defer_restore_resources(
> +	struct xfs_trans		*tp,
> +	struct xfs_defer_resources	*dres)
> +{
> +	unsigned short			i;
> +
> +	/* Rejoin the joined inodes. */
> +	for (i = 0; i < dres->dr_inos; i++)
> +		xfs_trans_ijoin(tp, dres->dr_ip[i], 0);
> +
> +	/* Rejoin the buffers and dirty them so the log moves forward. */
> +	for (i = 0; i < dres->dr_bufs; i++) {
> +		xfs_trans_bjoin(tp, dres->dr_bp[i]);
> +		if (dres->dr_ordered & (1U << i))
> +			xfs_trans_ordered_buf(tp, dres->dr_bp[i]);
> +		xfs_trans_bhold(tp, dres->dr_bp[i]);
> +	}
> +}
> +
> +/* Roll a transaction so we can do some deferred op processing. */
> +STATIC int
> +xfs_defer_trans_roll(
> +	struct xfs_trans		**tpp)
> +{
> +	struct xfs_defer_resources	dres = { };
> +	int				error;
> +
> +	error = xfs_defer_save_resources(&dres, *tpp);
> +	if (error)
> +		return error;
> +
> +	trace_xfs_defer_trans_roll(*tpp, _RET_IP_);
>   
>   	/*
>   	 * Roll the transaction.  Rolling always given a new transaction (even
> @@ -295,22 +329,11 @@ xfs_defer_trans_roll(
>   	 * happened.
>   	 */
>   	error = xfs_trans_roll(tpp);
> -	tp = *tpp;
>   
> -	/* Rejoin the joined inodes. */
> -	for (i = 0; i < ipcount; i++)
> -		xfs_trans_ijoin(tp, iplist[i], 0);
> -
> -	/* Rejoin the buffers and dirty them so the log moves forward. */
> -	for (i = 0; i < bpcount; i++) {
> -		xfs_trans_bjoin(tp, bplist[i]);
> -		if (ordered & (1U << i))
> -			xfs_trans_ordered_buf(tp, bplist[i]);
> -		xfs_trans_bhold(tp, bplist[i]);
> -	}
> +	xfs_defer_restore_resources(*tpp, &dres);
>   
>   	if (error)
> -		trace_xfs_defer_trans_roll_error(tp, error);
> +		trace_xfs_defer_trans_roll_error(*tpp, error);
>   	return error;
>   }
>   
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index 05472f71fffe..e095abb96f1a 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -64,6 +64,30 @@ extern const struct xfs_defer_op_type xfs_rmap_update_defer_type;
>   extern const struct xfs_defer_op_type xfs_extent_free_defer_type;
>   extern const struct xfs_defer_op_type xfs_agfl_free_defer_type;
>   
> +/*
> + * Deferred operation item relogging limits.
> + */
> +#define XFS_DEFER_OPS_NR_INODES	2	/* join up to two inodes */
> +#define XFS_DEFER_OPS_NR_BUFS	2	/* join up to two buffers */
> +
> +/* Resources that must be held across a transaction roll. */
> +struct xfs_defer_resources {
> +	/* held buffers */
> +	struct xfs_buf		*dr_bp[XFS_DEFER_OPS_NR_BUFS];
> +
> +	/* inodes with no unlock flags */
> +	struct xfs_inode	*dr_ip[XFS_DEFER_OPS_NR_INODES];
> +
> +	/* number of held buffers */
> +	unsigned short		dr_bufs;
> +
> +	/* bitmap of ordered buffers */
> +	unsigned short		dr_ordered;
> +
> +	/* number of held inodes */
> +	unsigned short		dr_inos;
> +};
> +
>   /*
>    * This structure enables a dfops user to detach the chain of deferred
>    * operations from a transaction so that they can be continued later.
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 50da47f23a07..3d2e89c4d446 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -112,12 +112,6 @@ void	xfs_log_item_init(struct xfs_mount *mp, struct xfs_log_item *item,
>   #define XFS_ITEM_LOCKED		2
>   #define XFS_ITEM_FLUSHING	3
>   
> -/*
> - * Deferred operation item relogging limits.
> - */
> -#define XFS_DEFER_OPS_NR_INODES	2	/* join up to two inodes */
> -#define XFS_DEFER_OPS_NR_BUFS	2	/* join up to two buffers */
> -
>   /*
>    * This is the structure maintained for every active transaction.
>    */
> 

[PATCH 2/2] xfs: port the defer ops capture and continue to resource capture

[Darrick J. Wong]

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

When log recovery tries to recover a transaction that had log intent
items attached to it, it has to save certain parts of the transaction
state (reservation, dfops chain, inodes with no automatic unlock) so
that it can finish single-stepping the recovered transactions before
finishing the chains.

This is done with the xfs_defer_ops_capture and xfs_defer_ops_continue
functions.  Right now they open-code this functionality, so let's port
this to the formalized resource capture structure that we introduced in
the previous patch.  This enables us to hold up to two inodes and two
buffers during log recovery, the same way we do for regular runtime.

With this patch applied, we'll be ready to support atomic extent swap
which holds two inodes; and logged xattrs which holds one inode and one
xattr leaf buffer.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_defer.c  |   86 +++++++++++++++++++++++++++++++++-----------
 fs/xfs/libxfs/xfs_defer.h  |   14 +++----
 fs/xfs/xfs_bmap_item.c     |    2 +
 fs/xfs/xfs_extfree_item.c  |    2 +
 fs/xfs/xfs_log_recover.c   |   12 ++----
 fs/xfs/xfs_refcount_item.c |    2 +
 fs/xfs/xfs_rmap_item.c     |    2 +
 7 files changed, 79 insertions(+), 41 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 7c6490f3e537..136a367d7b16 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -650,10 +650,11 @@ xfs_defer_move(
  */
 static struct xfs_defer_capture *
 xfs_defer_ops_capture(
-	struct xfs_trans		*tp,
-	struct xfs_inode		*capture_ip)
+	struct xfs_trans		*tp)
 {
 	struct xfs_defer_capture	*dfc;
+	unsigned short			i;
+	int				error;
 
 	if (list_empty(&tp->t_dfops))
 		return NULL;
@@ -677,27 +678,48 @@ xfs_defer_ops_capture(
 	/* Preserve the log reservation size. */
 	dfc->dfc_logres = tp->t_log_res;
 
+	error = xfs_defer_save_resources(&dfc->dfc_held, tp);
+	if (error) {
+		/*
+		 * Resource capture should never fail, but if it does, we
+		 * still have to shut down the log and release things
+		 * properly.
+		 */
+		xfs_force_shutdown(tp->t_mountp, SHUTDOWN_CORRUPT_INCORE);
+	}
+
 	/*
-	 * Grab an extra reference to this inode and attach it to the capture
-	 * structure.
+	 * Grab extra references to the inodes and buffers because callers are
+	 * expected to release their held references after we commit the
+	 * transaction.
 	 */
-	if (capture_ip) {
-		ihold(VFS_I(capture_ip));
-		dfc->dfc_capture_ip = capture_ip;
+	for (i = 0; i < dfc->dfc_held.dr_inos; i++) {
+		ASSERT(xfs_isilocked(dfc->dfc_held.dr_ip[i], XFS_ILOCK_EXCL));
+		ihold(VFS_I(dfc->dfc_held.dr_ip[i]));
 	}
 
+	for (i = 0; i < dfc->dfc_held.dr_bufs; i++)
+		xfs_buf_hold(dfc->dfc_held.dr_bp[i]);
+
 	return dfc;
 }
 
 /* Release all resources that we used to capture deferred ops. */
 void
-xfs_defer_ops_release(
+xfs_defer_ops_capture_free(
 	struct xfs_mount		*mp,
 	struct xfs_defer_capture	*dfc)
 {
+	unsigned short			i;
+
 	xfs_defer_cancel_list(mp, &dfc->dfc_dfops);
-	if (dfc->dfc_capture_ip)
-		xfs_irele(dfc->dfc_capture_ip);
+
+	for (i = 0; i < dfc->dfc_held.dr_bufs; i++)
+		xfs_buf_relse(dfc->dfc_held.dr_bp[i]);
+
+	for (i = 0; i < dfc->dfc_held.dr_inos; i++)
+		xfs_irele(dfc->dfc_held.dr_ip[i]);
+
 	kmem_free(dfc);
 }
 
@@ -712,24 +734,21 @@ xfs_defer_ops_release(
 int
 xfs_defer_ops_capture_and_commit(
 	struct xfs_trans		*tp,
-	struct xfs_inode		*capture_ip,
 	struct list_head		*capture_list)
 {
 	struct xfs_mount		*mp = tp->t_mountp;
 	struct xfs_defer_capture	*dfc;
 	int				error;
 
-	ASSERT(!capture_ip || xfs_isilocked(capture_ip, XFS_ILOCK_EXCL));
-
 	/* If we don't capture anything, commit transaction and exit. */
-	dfc = xfs_defer_ops_capture(tp, capture_ip);
+	dfc = xfs_defer_ops_capture(tp);
 	if (!dfc)
 		return xfs_trans_commit(tp);
 
 	/* Commit the transaction and add the capture structure to the list. */
 	error = xfs_trans_commit(tp);
 	if (error) {
-		xfs_defer_ops_release(mp, dfc);
+		xfs_defer_ops_capture_free(mp, dfc);
 		return error;
 	}
 
@@ -747,17 +766,19 @@ void
 xfs_defer_ops_continue(
 	struct xfs_defer_capture	*dfc,
 	struct xfs_trans		*tp,
-	struct xfs_inode		**captured_ipp)
+	struct xfs_defer_resources	*dres)
 {
 	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
 	ASSERT(!(tp->t_flags & XFS_TRANS_DIRTY));
 
 	/* Lock and join the captured inode to the new transaction. */
-	if (dfc->dfc_capture_ip) {
-		xfs_ilock(dfc->dfc_capture_ip, XFS_ILOCK_EXCL);
-		xfs_trans_ijoin(tp, dfc->dfc_capture_ip, 0);
-	}
-	*captured_ipp = dfc->dfc_capture_ip;
+	if (dfc->dfc_held.dr_inos == 2)
+		xfs_lock_two_inodes(dfc->dfc_held.dr_ip[0], XFS_ILOCK_EXCL,
+				    dfc->dfc_held.dr_ip[1], XFS_ILOCK_EXCL);
+	else if (dfc->dfc_held.dr_inos == 1)
+		xfs_ilock(dfc->dfc_held.dr_ip[0], XFS_ILOCK_EXCL);
+	xfs_defer_restore_resources(tp, &dfc->dfc_held);
+	memcpy(dres, &dfc->dfc_held, sizeof(struct xfs_defer_resources));
 
 	/* Move captured dfops chain and state to the transaction. */
 	list_splice_init(&dfc->dfc_dfops, &tp->t_dfops);
@@ -765,3 +786,26 @@ xfs_defer_ops_continue(
 
 	kmem_free(dfc);
 }
+
+/* Release the resources captured and continued during recovery. */
+void
+xfs_defer_resources_rele(
+	struct xfs_defer_resources	*dres)
+{
+	unsigned short			i;
+
+	for (i = 0; i < dres->dr_inos; i++) {
+		xfs_iunlock(dres->dr_ip[i], XFS_ILOCK_EXCL);
+		xfs_irele(dres->dr_ip[i]);
+		dres->dr_ip[i] = NULL;
+	}
+
+	for (i = 0; i < dres->dr_bufs; i++) {
+		xfs_buf_relse(dres->dr_bp[i]);
+		dres->dr_bp[i] = NULL;
+	}
+
+	dres->dr_inos = 0;
+	dres->dr_bufs = 0;
+	dres->dr_ordered = 0;
+}
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index e095abb96f1a..7952695c7c41 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -107,11 +107,7 @@ struct xfs_defer_capture {
 	/* Log reservation saved from the transaction. */
 	unsigned int		dfc_logres;
 
-	/*
-	 * An inode reference that must be maintained to complete the deferred
-	 * work.
-	 */
-	struct xfs_inode	*dfc_capture_ip;
+	struct xfs_defer_resources dfc_held;
 };
 
 /*
@@ -119,9 +115,11 @@ struct xfs_defer_capture {
  * This doesn't normally happen except log recovery.
  */
 int xfs_defer_ops_capture_and_commit(struct xfs_trans *tp,
-		struct xfs_inode *capture_ip, struct list_head *capture_list);
+		struct list_head *capture_list);
 void xfs_defer_ops_continue(struct xfs_defer_capture *d, struct xfs_trans *tp,
-		struct xfs_inode **captured_ipp);
-void xfs_defer_ops_release(struct xfs_mount *mp, struct xfs_defer_capture *d);
+		struct xfs_defer_resources *dres);
+void xfs_defer_ops_capture_free(struct xfs_mount *mp,
+		struct xfs_defer_capture *d);
+void xfs_defer_resources_rele(struct xfs_defer_resources *dres);
 
 #endif /* __XFS_DEFER_H__ */
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 03159970133f..e66c85a75104 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -532,7 +532,7 @@ xfs_bui_item_recover(
 	 * Commit transaction, which frees the transaction and saves the inode
 	 * for later replay activities.
 	 */
-	error = xfs_defer_ops_capture_and_commit(tp, ip, capture_list);
+	error = xfs_defer_ops_capture_and_commit(tp, capture_list);
 	if (error)
 		goto err_unlock;
 
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 3f8a0713573a..8f12931b0cbb 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -637,7 +637,7 @@ xfs_efi_item_recover(
 
 	}
 
-	return xfs_defer_ops_capture_and_commit(tp, NULL, capture_list);
+	return xfs_defer_ops_capture_and_commit(tp, capture_list);
 
 abort_error:
 	xfs_trans_cancel(tp);
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 10562ecbd9ea..53366cc0bc9e 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2466,11 +2466,11 @@ xlog_finish_defer_ops(
 {
 	struct xfs_defer_capture *dfc, *next;
 	struct xfs_trans	*tp;
-	struct xfs_inode	*ip;
 	int			error = 0;
 
 	list_for_each_entry_safe(dfc, next, capture_list, dfc_list) {
 		struct xfs_trans_res	resv;
+		struct xfs_defer_resources dres;
 
 		/*
 		 * Create a new transaction reservation from the captured
@@ -2494,13 +2494,9 @@ xlog_finish_defer_ops(
 		 * from recovering a single intent item.
 		 */
 		list_del_init(&dfc->dfc_list);
-		xfs_defer_ops_continue(dfc, tp, &ip);
-
+		xfs_defer_ops_continue(dfc, tp, &dres);
 		error = xfs_trans_commit(tp);
-		if (ip) {
-			xfs_iunlock(ip, XFS_ILOCK_EXCL);
-			xfs_irele(ip);
-		}
+		xfs_defer_resources_rele(&dres);
 		if (error)
 			return error;
 	}
@@ -2520,7 +2516,7 @@ xlog_abort_defer_ops(
 
 	list_for_each_entry_safe(dfc, next, capture_list, dfc_list) {
 		list_del_init(&dfc->dfc_list);
-		xfs_defer_ops_release(mp, dfc);
+		xfs_defer_ops_capture_free(mp, dfc);
 	}
 }
 /*
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 46904b793bd4..61bbbe816b5e 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -557,7 +557,7 @@ xfs_cui_item_recover(
 	}
 
 	xfs_refcount_finish_one_cleanup(tp, rcur, error);
-	return xfs_defer_ops_capture_and_commit(tp, NULL, capture_list);
+	return xfs_defer_ops_capture_and_commit(tp, capture_list);
 
 abort_error:
 	xfs_refcount_finish_one_cleanup(tp, rcur, error);
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 5f0695980467..181cd24d2ba9 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -587,7 +587,7 @@ xfs_rui_item_recover(
 	}
 
 	xfs_rmap_finish_one_cleanup(tp, rcur, error);
-	return xfs_defer_ops_capture_and_commit(tp, NULL, capture_list);
+	return xfs_defer_ops_capture_and_commit(tp, capture_list);
 
 abort_error:
 	xfs_rmap_finish_one_cleanup(tp, rcur, error);

Re: [PATCH 2/2] xfs: port the defer ops capture and continue to resource capture

[Allison Henderson]

On 9/17/21 6:30 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> When log recovery tries to recover a transaction that had log intent
> items attached to it, it has to save certain parts of the transaction
> state (reservation, dfops chain, inodes with no automatic unlock) so
> that it can finish single-stepping the recovered transactions before
> finishing the chains.
> 
> This is done with the xfs_defer_ops_capture and xfs_defer_ops_continue
> functions.  Right now they open-code this functionality, so let's port
> this to the formalized resource capture structure that we introduced in
> the previous patch.  This enables us to hold up to two inodes and two
> buffers during log recovery, the same way we do for regular runtime.
> 
> With this patch applied, we'll be ready to support atomic extent swap
> which holds two inodes; and logged xattrs which holds one inode and one
> xattr leaf buffer.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Ok, makes sense
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>   fs/xfs/libxfs/xfs_defer.c  |   86 +++++++++++++++++++++++++++++++++-----------
>   fs/xfs/libxfs/xfs_defer.h  |   14 +++----
>   fs/xfs/xfs_bmap_item.c     |    2 +
>   fs/xfs/xfs_extfree_item.c  |    2 +
>   fs/xfs/xfs_log_recover.c   |   12 ++----
>   fs/xfs/xfs_refcount_item.c |    2 +
>   fs/xfs/xfs_rmap_item.c     |    2 +
>   7 files changed, 79 insertions(+), 41 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index 7c6490f3e537..136a367d7b16 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -650,10 +650,11 @@ xfs_defer_move(
>    */
>   static struct xfs_defer_capture *
>   xfs_defer_ops_capture(
> -	struct xfs_trans		*tp,
> -	struct xfs_inode		*capture_ip)
> +	struct xfs_trans		*tp)
>   {
>   	struct xfs_defer_capture	*dfc;
> +	unsigned short			i;
> +	int				error;
>   
>   	if (list_empty(&tp->t_dfops))
>   		return NULL;
> @@ -677,27 +678,48 @@ xfs_defer_ops_capture(
>   	/* Preserve the log reservation size. */
>   	dfc->dfc_logres = tp->t_log_res;
>   
> +	error = xfs_defer_save_resources(&dfc->dfc_held, tp);
> +	if (error) {
> +		/*
> +		 * Resource capture should never fail, but if it does, we
> +		 * still have to shut down the log and release things
> +		 * properly.
> +		 */
> +		xfs_force_shutdown(tp->t_mountp, SHUTDOWN_CORRUPT_INCORE);
> +	}
> +
>   	/*
> -	 * Grab an extra reference to this inode and attach it to the capture
> -	 * structure.
> +	 * Grab extra references to the inodes and buffers because callers are
> +	 * expected to release their held references after we commit the
> +	 * transaction.
>   	 */
> -	if (capture_ip) {
> -		ihold(VFS_I(capture_ip));
> -		dfc->dfc_capture_ip = capture_ip;
> +	for (i = 0; i < dfc->dfc_held.dr_inos; i++) {
> +		ASSERT(xfs_isilocked(dfc->dfc_held.dr_ip[i], XFS_ILOCK_EXCL));
> +		ihold(VFS_I(dfc->dfc_held.dr_ip[i]));
>   	}
>   
> +	for (i = 0; i < dfc->dfc_held.dr_bufs; i++)
> +		xfs_buf_hold(dfc->dfc_held.dr_bp[i]);
> +
>   	return dfc;
>   }
>   
>   /* Release all resources that we used to capture deferred ops. */
>   void
> -xfs_defer_ops_release(
> +xfs_defer_ops_capture_free(
>   	struct xfs_mount		*mp,
>   	struct xfs_defer_capture	*dfc)
>   {
> +	unsigned short			i;
> +
>   	xfs_defer_cancel_list(mp, &dfc->dfc_dfops);
> -	if (dfc->dfc_capture_ip)
> -		xfs_irele(dfc->dfc_capture_ip);
> +
> +	for (i = 0; i < dfc->dfc_held.dr_bufs; i++)
> +		xfs_buf_relse(dfc->dfc_held.dr_bp[i]);
> +
> +	for (i = 0; i < dfc->dfc_held.dr_inos; i++)
> +		xfs_irele(dfc->dfc_held.dr_ip[i]);
> +
>   	kmem_free(dfc);
>   }
>   
> @@ -712,24 +734,21 @@ xfs_defer_ops_release(
>   int
>   xfs_defer_ops_capture_and_commit(
>   	struct xfs_trans		*tp,
> -	struct xfs_inode		*capture_ip,
>   	struct list_head		*capture_list)
>   {
>   	struct xfs_mount		*mp = tp->t_mountp;
>   	struct xfs_defer_capture	*dfc;
>   	int				error;
>   
> -	ASSERT(!capture_ip || xfs_isilocked(capture_ip, XFS_ILOCK_EXCL));
> -
>   	/* If we don't capture anything, commit transaction and exit. */
> -	dfc = xfs_defer_ops_capture(tp, capture_ip);
> +	dfc = xfs_defer_ops_capture(tp);
>   	if (!dfc)
>   		return xfs_trans_commit(tp);
>   
>   	/* Commit the transaction and add the capture structure to the list. */
>   	error = xfs_trans_commit(tp);
>   	if (error) {
> -		xfs_defer_ops_release(mp, dfc);
> +		xfs_defer_ops_capture_free(mp, dfc);
>   		return error;
>   	}
>   
> @@ -747,17 +766,19 @@ void
>   xfs_defer_ops_continue(
>   	struct xfs_defer_capture	*dfc,
>   	struct xfs_trans		*tp,
> -	struct xfs_inode		**captured_ipp)
> +	struct xfs_defer_resources	*dres)
>   {
>   	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
>   	ASSERT(!(tp->t_flags & XFS_TRANS_DIRTY));
>   
>   	/* Lock and join the captured inode to the new transaction. */
> -	if (dfc->dfc_capture_ip) {
> -		xfs_ilock(dfc->dfc_capture_ip, XFS_ILOCK_EXCL);
> -		xfs_trans_ijoin(tp, dfc->dfc_capture_ip, 0);
> -	}
> -	*captured_ipp = dfc->dfc_capture_ip;
> +	if (dfc->dfc_held.dr_inos == 2)
> +		xfs_lock_two_inodes(dfc->dfc_held.dr_ip[0], XFS_ILOCK_EXCL,
> +				    dfc->dfc_held.dr_ip[1], XFS_ILOCK_EXCL);
> +	else if (dfc->dfc_held.dr_inos == 1)
> +		xfs_ilock(dfc->dfc_held.dr_ip[0], XFS_ILOCK_EXCL);
> +	xfs_defer_restore_resources(tp, &dfc->dfc_held);
> +	memcpy(dres, &dfc->dfc_held, sizeof(struct xfs_defer_resources));
>   
>   	/* Move captured dfops chain and state to the transaction. */
>   	list_splice_init(&dfc->dfc_dfops, &tp->t_dfops);
> @@ -765,3 +786,26 @@ xfs_defer_ops_continue(
>   
>   	kmem_free(dfc);
>   }
> +
> +/* Release the resources captured and continued during recovery. */
> +void
> +xfs_defer_resources_rele(
> +	struct xfs_defer_resources	*dres)
> +{
> +	unsigned short			i;
> +
> +	for (i = 0; i < dres->dr_inos; i++) {
> +		xfs_iunlock(dres->dr_ip[i], XFS_ILOCK_EXCL);
> +		xfs_irele(dres->dr_ip[i]);
> +		dres->dr_ip[i] = NULL;
> +	}
> +
> +	for (i = 0; i < dres->dr_bufs; i++) {
> +		xfs_buf_relse(dres->dr_bp[i]);
> +		dres->dr_bp[i] = NULL;
> +	}
> +
> +	dres->dr_inos = 0;
> +	dres->dr_bufs = 0;
> +	dres->dr_ordered = 0;
> +}
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index e095abb96f1a..7952695c7c41 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -107,11 +107,7 @@ struct xfs_defer_capture {
>   	/* Log reservation saved from the transaction. */
>   	unsigned int		dfc_logres;
>   
> -	/*
> -	 * An inode reference that must be maintained to complete the deferred
> -	 * work.
> -	 */
> -	struct xfs_inode	*dfc_capture_ip;
> +	struct xfs_defer_resources dfc_held;
>   };
>   
>   /*
> @@ -119,9 +115,11 @@ struct xfs_defer_capture {
>    * This doesn't normally happen except log recovery.
>    */
>   int xfs_defer_ops_capture_and_commit(struct xfs_trans *tp,
> -		struct xfs_inode *capture_ip, struct list_head *capture_list);
> +		struct list_head *capture_list);
>   void xfs_defer_ops_continue(struct xfs_defer_capture *d, struct xfs_trans *tp,
> -		struct xfs_inode **captured_ipp);
> -void xfs_defer_ops_release(struct xfs_mount *mp, struct xfs_defer_capture *d);
> +		struct xfs_defer_resources *dres);
> +void xfs_defer_ops_capture_free(struct xfs_mount *mp,
> +		struct xfs_defer_capture *d);
> +void xfs_defer_resources_rele(struct xfs_defer_resources *dres);
>   
>   #endif /* __XFS_DEFER_H__ */
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index 03159970133f..e66c85a75104 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -532,7 +532,7 @@ xfs_bui_item_recover(
>   	 * Commit transaction, which frees the transaction and saves the inode
>   	 * for later replay activities.
>   	 */
> -	error = xfs_defer_ops_capture_and_commit(tp, ip, capture_list);
> +	error = xfs_defer_ops_capture_and_commit(tp, capture_list);
>   	if (error)
>   		goto err_unlock;
>   
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 3f8a0713573a..8f12931b0cbb 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -637,7 +637,7 @@ xfs_efi_item_recover(
>   
>   	}
>   
> -	return xfs_defer_ops_capture_and_commit(tp, NULL, capture_list);
> +	return xfs_defer_ops_capture_and_commit(tp, capture_list);
>   
>   abort_error:
>   	xfs_trans_cancel(tp);
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 10562ecbd9ea..53366cc0bc9e 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2466,11 +2466,11 @@ xlog_finish_defer_ops(
>   {
>   	struct xfs_defer_capture *dfc, *next;
>   	struct xfs_trans	*tp;
> -	struct xfs_inode	*ip;
>   	int			error = 0;
>   
>   	list_for_each_entry_safe(dfc, next, capture_list, dfc_list) {
>   		struct xfs_trans_res	resv;
> +		struct xfs_defer_resources dres;
>   
>   		/*
>   		 * Create a new transaction reservation from the captured
> @@ -2494,13 +2494,9 @@ xlog_finish_defer_ops(
>   		 * from recovering a single intent item.
>   		 */
>   		list_del_init(&dfc->dfc_list);
> -		xfs_defer_ops_continue(dfc, tp, &ip);
> -
> +		xfs_defer_ops_continue(dfc, tp, &dres);
>   		error = xfs_trans_commit(tp);
> -		if (ip) {
> -			xfs_iunlock(ip, XFS_ILOCK_EXCL);
> -			xfs_irele(ip);
> -		}
> +		xfs_defer_resources_rele(&dres);
>   		if (error)
>   			return error;
>   	}
> @@ -2520,7 +2516,7 @@ xlog_abort_defer_ops(
>   
>   	list_for_each_entry_safe(dfc, next, capture_list, dfc_list) {
>   		list_del_init(&dfc->dfc_list);
> -		xfs_defer_ops_release(mp, dfc);
> +		xfs_defer_ops_capture_free(mp, dfc);
>   	}
>   }
>   /*
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index 46904b793bd4..61bbbe816b5e 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -557,7 +557,7 @@ xfs_cui_item_recover(
>   	}
>   
>   	xfs_refcount_finish_one_cleanup(tp, rcur, error);
> -	return xfs_defer_ops_capture_and_commit(tp, NULL, capture_list);
> +	return xfs_defer_ops_capture_and_commit(tp, capture_list);
>   
>   abort_error:
>   	xfs_refcount_finish_one_cleanup(tp, rcur, error);
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index 5f0695980467..181cd24d2ba9 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -587,7 +587,7 @@ xfs_rui_item_recover(
>   	}
>   
>   	xfs_rmap_finish_one_cleanup(tp, rcur, error);
> -	return xfs_defer_ops_capture_and_commit(tp, NULL, capture_list);
> +	return xfs_defer_ops_capture_and_commit(tp, capture_list);
>   
>   abort_error:
>   	xfs_rmap_finish_one_cleanup(tp, rcur, error);
> 

Re: [PATCH 2/2] xfs: port the defer ops capture and continue to resource capture

[Allison Henderson]

On 9/17/21 6:30 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> When log recovery tries to recover a transaction that had log intent
> items attached to it, it has to save certain parts of the transaction
> state (reservation, dfops chain, inodes with no automatic unlock) so
> that it can finish single-stepping the recovered transactions before
> finishing the chains.
> 
> This is done with the xfs_defer_ops_capture and xfs_defer_ops_continue
> functions.  Right now they open-code this functionality, so let's port
> this to the formalized resource capture structure that we introduced in
> the previous patch.  This enables us to hold up to two inodes and two
> buffers during log recovery, the same way we do for regular runtime.
> 
> With this patch applied, we'll be ready to support atomic extent swap
> which holds two inodes; and logged xattrs which holds one inode and one
> xattr leaf buffer.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Ok, I think this is a cleaner way to address to issues for both features
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>   fs/xfs/libxfs/xfs_defer.c  |   86 +++++++++++++++++++++++++++++++++-----------
>   fs/xfs/libxfs/xfs_defer.h  |   14 +++----
>   fs/xfs/xfs_bmap_item.c     |    2 +
>   fs/xfs/xfs_extfree_item.c  |    2 +
>   fs/xfs/xfs_log_recover.c   |   12 ++----
>   fs/xfs/xfs_refcount_item.c |    2 +
>   fs/xfs/xfs_rmap_item.c     |    2 +
>   7 files changed, 79 insertions(+), 41 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index 7c6490f3e537..136a367d7b16 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -650,10 +650,11 @@ xfs_defer_move(
>    */
>   static struct xfs_defer_capture *
>   xfs_defer_ops_capture(
> -	struct xfs_trans		*tp,
> -	struct xfs_inode		*capture_ip)
> +	struct xfs_trans		*tp)
>   {
>   	struct xfs_defer_capture	*dfc;
> +	unsigned short			i;
> +	int				error;
>   
>   	if (list_empty(&tp->t_dfops))
>   		return NULL;
> @@ -677,27 +678,48 @@ xfs_defer_ops_capture(
>   	/* Preserve the log reservation size. */
>   	dfc->dfc_logres = tp->t_log_res;
>   
> +	error = xfs_defer_save_resources(&dfc->dfc_held, tp);
> +	if (error) {
> +		/*
> +		 * Resource capture should never fail, but if it does, we
> +		 * still have to shut down the log and release things
> +		 * properly.
> +		 */
> +		xfs_force_shutdown(tp->t_mountp, SHUTDOWN_CORRUPT_INCORE);
> +	}
> +
>   	/*
> -	 * Grab an extra reference to this inode and attach it to the capture
> -	 * structure.
> +	 * Grab extra references to the inodes and buffers because callers are
> +	 * expected to release their held references after we commit the
> +	 * transaction.
>   	 */
> -	if (capture_ip) {
> -		ihold(VFS_I(capture_ip));
> -		dfc->dfc_capture_ip = capture_ip;
> +	for (i = 0; i < dfc->dfc_held.dr_inos; i++) {
> +		ASSERT(xfs_isilocked(dfc->dfc_held.dr_ip[i], XFS_ILOCK_EXCL));
> +		ihold(VFS_I(dfc->dfc_held.dr_ip[i]));
>   	}
>   
> +	for (i = 0; i < dfc->dfc_held.dr_bufs; i++)
> +		xfs_buf_hold(dfc->dfc_held.dr_bp[i]);
> +
>   	return dfc;
>   }
>   
>   /* Release all resources that we used to capture deferred ops. */
>   void
> -xfs_defer_ops_release(
> +xfs_defer_ops_capture_free(
>   	struct xfs_mount		*mp,
>   	struct xfs_defer_capture	*dfc)
>   {
> +	unsigned short			i;
> +
>   	xfs_defer_cancel_list(mp, &dfc->dfc_dfops);
> -	if (dfc->dfc_capture_ip)
> -		xfs_irele(dfc->dfc_capture_ip);
> +
> +	for (i = 0; i < dfc->dfc_held.dr_bufs; i++)
> +		xfs_buf_relse(dfc->dfc_held.dr_bp[i]);
> +
> +	for (i = 0; i < dfc->dfc_held.dr_inos; i++)
> +		xfs_irele(dfc->dfc_held.dr_ip[i]);
> +
>   	kmem_free(dfc);
>   }
>   
> @@ -712,24 +734,21 @@ xfs_defer_ops_release(
>   int
>   xfs_defer_ops_capture_and_commit(
>   	struct xfs_trans		*tp,
> -	struct xfs_inode		*capture_ip,
>   	struct list_head		*capture_list)
>   {
>   	struct xfs_mount		*mp = tp->t_mountp;
>   	struct xfs_defer_capture	*dfc;
>   	int				error;
>   
> -	ASSERT(!capture_ip || xfs_isilocked(capture_ip, XFS_ILOCK_EXCL));
> -
>   	/* If we don't capture anything, commit transaction and exit. */
> -	dfc = xfs_defer_ops_capture(tp, capture_ip);
> +	dfc = xfs_defer_ops_capture(tp);
>   	if (!dfc)
>   		return xfs_trans_commit(tp);
>   
>   	/* Commit the transaction and add the capture structure to the list. */
>   	error = xfs_trans_commit(tp);
>   	if (error) {
> -		xfs_defer_ops_release(mp, dfc);
> +		xfs_defer_ops_capture_free(mp, dfc);
>   		return error;
>   	}
>   
> @@ -747,17 +766,19 @@ void
>   xfs_defer_ops_continue(
>   	struct xfs_defer_capture	*dfc,
>   	struct xfs_trans		*tp,
> -	struct xfs_inode		**captured_ipp)
> +	struct xfs_defer_resources	*dres)
>   {
>   	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
>   	ASSERT(!(tp->t_flags & XFS_TRANS_DIRTY));
>   
>   	/* Lock and join the captured inode to the new transaction. */
> -	if (dfc->dfc_capture_ip) {
> -		xfs_ilock(dfc->dfc_capture_ip, XFS_ILOCK_EXCL);
> -		xfs_trans_ijoin(tp, dfc->dfc_capture_ip, 0);
> -	}
> -	*captured_ipp = dfc->dfc_capture_ip;
> +	if (dfc->dfc_held.dr_inos == 2)
> +		xfs_lock_two_inodes(dfc->dfc_held.dr_ip[0], XFS_ILOCK_EXCL,
> +				    dfc->dfc_held.dr_ip[1], XFS_ILOCK_EXCL);
> +	else if (dfc->dfc_held.dr_inos == 1)
> +		xfs_ilock(dfc->dfc_held.dr_ip[0], XFS_ILOCK_EXCL);
> +	xfs_defer_restore_resources(tp, &dfc->dfc_held);
> +	memcpy(dres, &dfc->dfc_held, sizeof(struct xfs_defer_resources));
>   
>   	/* Move captured dfops chain and state to the transaction. */
>   	list_splice_init(&dfc->dfc_dfops, &tp->t_dfops);
> @@ -765,3 +786,26 @@ xfs_defer_ops_continue(
>   
>   	kmem_free(dfc);
>   }
> +
> +/* Release the resources captured and continued during recovery. */
> +void
> +xfs_defer_resources_rele(
> +	struct xfs_defer_resources	*dres)
> +{
> +	unsigned short			i;
> +
> +	for (i = 0; i < dres->dr_inos; i++) {
> +		xfs_iunlock(dres->dr_ip[i], XFS_ILOCK_EXCL);
> +		xfs_irele(dres->dr_ip[i]);
> +		dres->dr_ip[i] = NULL;
> +	}
> +
> +	for (i = 0; i < dres->dr_bufs; i++) {
> +		xfs_buf_relse(dres->dr_bp[i]);
> +		dres->dr_bp[i] = NULL;
> +	}
> +
> +	dres->dr_inos = 0;
> +	dres->dr_bufs = 0;
> +	dres->dr_ordered = 0;
> +}
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index e095abb96f1a..7952695c7c41 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -107,11 +107,7 @@ struct xfs_defer_capture {
>   	/* Log reservation saved from the transaction. */
>   	unsigned int		dfc_logres;
>   
> -	/*
> -	 * An inode reference that must be maintained to complete the deferred
> -	 * work.
> -	 */
> -	struct xfs_inode	*dfc_capture_ip;
> +	struct xfs_defer_resources dfc_held;
>   };
>   
>   /*
> @@ -119,9 +115,11 @@ struct xfs_defer_capture {
>    * This doesn't normally happen except log recovery.
>    */
>   int xfs_defer_ops_capture_and_commit(struct xfs_trans *tp,
> -		struct xfs_inode *capture_ip, struct list_head *capture_list);
> +		struct list_head *capture_list);
>   void xfs_defer_ops_continue(struct xfs_defer_capture *d, struct xfs_trans *tp,
> -		struct xfs_inode **captured_ipp);
> -void xfs_defer_ops_release(struct xfs_mount *mp, struct xfs_defer_capture *d);
> +		struct xfs_defer_resources *dres);
> +void xfs_defer_ops_capture_free(struct xfs_mount *mp,
> +		struct xfs_defer_capture *d);
> +void xfs_defer_resources_rele(struct xfs_defer_resources *dres);
>   
>   #endif /* __XFS_DEFER_H__ */
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index 03159970133f..e66c85a75104 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -532,7 +532,7 @@ xfs_bui_item_recover(
>   	 * Commit transaction, which frees the transaction and saves the inode
>   	 * for later replay activities.
>   	 */
> -	error = xfs_defer_ops_capture_and_commit(tp, ip, capture_list);
> +	error = xfs_defer_ops_capture_and_commit(tp, capture_list);
>   	if (error)
>   		goto err_unlock;
>   
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 3f8a0713573a..8f12931b0cbb 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -637,7 +637,7 @@ xfs_efi_item_recover(
>   
>   	}
>   
> -	return xfs_defer_ops_capture_and_commit(tp, NULL, capture_list);
> +	return xfs_defer_ops_capture_and_commit(tp, capture_list);
>   
>   abort_error:
>   	xfs_trans_cancel(tp);
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 10562ecbd9ea..53366cc0bc9e 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2466,11 +2466,11 @@ xlog_finish_defer_ops(
>   {
>   	struct xfs_defer_capture *dfc, *next;
>   	struct xfs_trans	*tp;
> -	struct xfs_inode	*ip;
>   	int			error = 0;
>   
>   	list_for_each_entry_safe(dfc, next, capture_list, dfc_list) {
>   		struct xfs_trans_res	resv;
> +		struct xfs_defer_resources dres;
>   
>   		/*
>   		 * Create a new transaction reservation from the captured
> @@ -2494,13 +2494,9 @@ xlog_finish_defer_ops(
>   		 * from recovering a single intent item.
>   		 */
>   		list_del_init(&dfc->dfc_list);
> -		xfs_defer_ops_continue(dfc, tp, &ip);
> -
> +		xfs_defer_ops_continue(dfc, tp, &dres);
>   		error = xfs_trans_commit(tp);
> -		if (ip) {
> -			xfs_iunlock(ip, XFS_ILOCK_EXCL);
> -			xfs_irele(ip);
> -		}
> +		xfs_defer_resources_rele(&dres);
>   		if (error)
>   			return error;
>   	}
> @@ -2520,7 +2516,7 @@ xlog_abort_defer_ops(
>   
>   	list_for_each_entry_safe(dfc, next, capture_list, dfc_list) {
>   		list_del_init(&dfc->dfc_list);
> -		xfs_defer_ops_release(mp, dfc);
> +		xfs_defer_ops_capture_free(mp, dfc);
>   	}
>   }
>   /*
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index 46904b793bd4..61bbbe816b5e 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -557,7 +557,7 @@ xfs_cui_item_recover(
>   	}
>   
>   	xfs_refcount_finish_one_cleanup(tp, rcur, error);
> -	return xfs_defer_ops_capture_and_commit(tp, NULL, capture_list);
> +	return xfs_defer_ops_capture_and_commit(tp, capture_list);
>   
>   abort_error:
>   	xfs_refcount_finish_one_cleanup(tp, rcur, error);
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index 5f0695980467..181cd24d2ba9 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -587,7 +587,7 @@ xfs_rui_item_recover(
>   	}
>   
>   	xfs_rmap_finish_one_cleanup(tp, rcur, error);
> -	return xfs_defer_ops_capture_and_commit(tp, NULL, capture_list);
> +	return xfs_defer_ops_capture_and_commit(tp, capture_list);
>   
>   abort_error:
>   	xfs_rmap_finish_one_cleanup(tp, rcur, error);
> 

Re: [PATCHSET RFC achender 0/2] xfs: refactor log recovery resource capture

[Allison Henderson]

On 9/17/21 6:30 PM, Darrick J. Wong wrote:
> Hi all,
> 
> During review of Allison's logged xattrs patchset last cycle, I noticed
> that there was an opportunity to clean up some code structure
> differences between how regular runtime deferred attributes hold on to
> resources across a transaction roll, and how it's done during log
> recovery.  This series, in cleaning that up, should shorten her
> patchset and simplify it a bit.
> 
> During regular operation, transactions are allowed to hold up to two
> inodes and two buffers across a transaction roll to finish deferred log
> items.  This implies that log recovery of a log intent item ought to be
> able to do the same.  However, current log recovery code open-codes
> saving only a single inode, because that was all that was required.
> 
> With atomic extent swapping and logged extended attributes upon us, it
> has become evident that we need to use the same runtime mechanisms
> during recovery.  Refactor the deferred ops code to use the same
> resource capture mechanisms for both.
> 
> If you're going to start using this mess, you probably ought to just
> pull from my git trees, which are linked below.
> 
> This is an extraordinary way to destroy everything.  Enjoy!
> Comments and questions are, as always, welcome.
> 
> --D
> 
Hi Darrick,

Sorry to for the delay, thanks for putting this together.  These 
improvements look good to me, I will see if I can get this worked in 
underneath delayed attrs and give it a test run.

Allison

> kernel git tree:
> https://urldefense.com/v3/__https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=log-recovery-defer-capture-5.16__;!!ACWV5N9M2RV99hQ!ZyRkandC0OnbO6-EW6AVFwC62a54u8Ki6doYqvl2vJDfgBmkzB4GFZe5s25s8_0XfN1E$
> ---
>   fs/xfs/libxfs/xfs_defer.c  |  171 +++++++++++++++++++++++++++++++-------------
>   fs/xfs/libxfs/xfs_defer.h  |   38 ++++++++--
>   fs/xfs/xfs_bmap_item.c     |    2 -
>   fs/xfs/xfs_extfree_item.c  |    2 -
>   fs/xfs/xfs_log_recover.c   |   12 +--
>   fs/xfs/xfs_refcount_item.c |    2 -
>   fs/xfs/xfs_rmap_item.c     |    2 -
>   fs/xfs/xfs_trans.h         |    6 --
>   8 files changed, 157 insertions(+), 78 deletions(-)
>