xfs: condense dfops and automatic relogging

The current transaction allocation code conditionally initializes the ->t_dfops indirection pointer. Transaction commit/cancel check the validity of the pointer to determine whether to finish/cancel the internal dfops.

This disallows the ability to use the internal dfops list as a temporary container (via xfs_trans_alloc_empty()). Refactor transaction allocation to always initialize ->t_dfops and check permanent reservation state on transaction commit/cancel.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_trans.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

fs/xfs/xfs_trans.c +1 -7

index 7bf5c1202719..8d3b7f28b193 100644
@@ -281,13 +281,7 @@ xfs_trans_alloc(
 	INIT_LIST_HEAD(&tp->t_items);
 	INIT_LIST_HEAD(&tp->t_busy);
 	tp->t_firstblock = NULLFSBLOCK;
-	/*
-	 * We only roll transactions with permanent log reservation. Don't init
-	 * ->t_dfops to skip attempts to finish or cancel an empty dfops with a
-	 * non-permanent res.
-	 */
-	if (resp->tr_logflags & XFS_TRANS_PERM_LOG_RES)
-		xfs_defer_init(tp, &tp->t_dfops_internal);
+	xfs_defer_init(tp, &tp->t_dfops_internal);
 
 	error = xfs_trans_reserve(tp, resp, blocks, rtextents);
 	if (error) {

fs/xfs/xfs_trans.c +1 -1

 		error = xfs_defer_finish_noroll(&tp);
 		if (error) {
 			xfs_defer_cancel(tp);

@@ -1029,7 +1023,7 @@ xfs_trans_cancel(
 
 	trace_xfs_trans_cancel(tp, _RET_IP_);
 
-	if (tp->t_dfops)
+	if (tp->t_flags & XFS_TRANS_PERM_LOG_RES)
 		xfs_defer_cancel(tp);
 
 	/*

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

fs/xfs/xfs_trans.c __xfs_trans_commit( · line 929

Quoted 5 lines

fs/xfs/xfs_trans.c +1 -1

 	trace_xfs_trans_commit(tp, _RET_IP_);
 
 	/* finish deferred items on final commit */
-	if (!regrant && tp->t_dfops) {
+	if (!regrant && (tp->t_flags & XFS_TRANS_PERM_LOG_RES)) {

The usage model of deferred ops is that one has to create a transaction with a permanent reservation, and only then start attaching deferred ops to the dfops inside the transaction. It's a programming error if a caller tries to finish deferred ops using a non-permanent transaction, and prior to this patch t_dfops would be NULL and we'd blow up immediately in xfs_defer_add(..., tp->t_dfops, ...);

However, now that we initialize t_dfops unconditionally, won't this cause the above programming mistake to leak silently any incorrectly queued defer ops?

Quoted 35 lines · Brian Foster

The current transaction allocation code conditionally initializes the ->t_dfops indirection pointer. Transaction commit/cancel check the validity of the pointer to determine whether to finish/cancel the internal dfops.

This disallows the ability to use the internal dfops list as a temporary container (via xfs_trans_alloc_empty()). Refactor transaction allocation to always initialize ->t_dfops and check permanent reservation state on transaction commit/cancel.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_trans.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

fs/xfs/xfs_trans.c +1 -7

index 7bf5c1202719..8d3b7f28b193 100644
@@ -281,13 +281,7 @@ xfs_trans_alloc(
 	INIT_LIST_HEAD(&tp->t_items);
 	INIT_LIST_HEAD(&tp->t_busy);
 	tp->t_firstblock = NULLFSBLOCK;
-	/*
-	 * We only roll transactions with permanent log reservation. Don't init
-	 * ->t_dfops to skip attempts to finish or cancel an empty dfops with a
-	 * non-permanent res.
-	 */
-	if (resp->tr_logflags & XFS_TRANS_PERM_LOG_RES)
-		xfs_defer_init(tp, &tp->t_dfops_internal);
+	xfs_defer_init(tp, &tp->t_dfops_internal);
 
 	error = xfs_trans_reserve(tp, resp, blocks, rtextents);
 	if (error) {

fs/xfs/xfs_trans.c __xfs_trans_commit( · line 929

Quoted 5 lines

fs/xfs/xfs_trans.c +1 -1

@@ -932,7 +926,7 @@ __xfs_trans_commit(
 	trace_xfs_trans_commit(tp, _RET_IP_);
 
 	/* finish deferred items on final commit */
-	if (!regrant && tp->t_dfops) {
+	if (!regrant && (tp->t_flags & XFS_TRANS_PERM_LOG_RES)) {

The usage model of deferred ops is that one has to create a transaction with a permanent reservation, and only then start attaching deferred ops to the dfops inside the transaction. It's a programming error if a caller tries to finish deferred ops using a non-permanent transaction, and prior to this patch t_dfops would be NULL and we'd blow up immediately in xfs_defer_add(..., tp->t_dfops, ...);

However, now that we initialize t_dfops unconditionally, won't this cause the above programming mistake to leak silently any incorrectly queued defer ops?

--D

Quoted 19 lines

fs/xfs/xfs_trans.c +1 -1

 		error = xfs_defer_finish_noroll(&tp);
 		if (error) {
 			xfs_defer_cancel(tp);

@@ -1029,7 +1023,7 @@ xfs_trans_cancel(
 
 	trace_xfs_trans_cancel(tp, _RET_IP_);
 
-	if (tp->t_dfops)
+	if (tp->t_flags & XFS_TRANS_PERM_LOG_RES)
 		xfs_defer_cancel(tp);
 
 	/*

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 49 lines · Darrick J. Wong

Quoted 35 lines · Brian Foster

The current transaction allocation code conditionally initializes the ->t_dfops indirection pointer. Transaction commit/cancel check the validity of the pointer to determine whether to finish/cancel the internal dfops.

This disallows the ability to use the internal dfops list as a temporary container (via xfs_trans_alloc_empty()). Refactor transaction allocation to always initialize ->t_dfops and check permanent reservation state on transaction commit/cancel.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_trans.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

fs/xfs/xfs_trans.c +1 -7

index 7bf5c1202719..8d3b7f28b193 100644
@@ -281,13 +281,7 @@ xfs_trans_alloc(
 	INIT_LIST_HEAD(&tp->t_items);
 	INIT_LIST_HEAD(&tp->t_busy);
 	tp->t_firstblock = NULLFSBLOCK;
-	/*
-	 * We only roll transactions with permanent log reservation. Don't init
-	 * ->t_dfops to skip attempts to finish or cancel an empty dfops with a
-	 * non-permanent res.
-	 */
-	if (resp->tr_logflags & XFS_TRANS_PERM_LOG_RES)
-		xfs_defer_init(tp, &tp->t_dfops_internal);
+	xfs_defer_init(tp, &tp->t_dfops_internal);
 
 	error = xfs_trans_reserve(tp, resp, blocks, rtextents);
 	if (error) {

fs/xfs/xfs_trans.c __xfs_trans_commit( · line 929

Quoted 5 lines

fs/xfs/xfs_trans.c +1 -1

@@ -932,7 +926,7 @@ __xfs_trans_commit(
 	trace_xfs_trans_commit(tp, _RET_IP_);
 
 	/* finish deferred items on final commit */
-	if (!regrant && tp->t_dfops) {
+	if (!regrant && (tp->t_flags & XFS_TRANS_PERM_LOG_RES)) {

The usage model of deferred ops is that one has to create a transaction with a permanent reservation, and only then start attaching deferred ops to the dfops inside the transaction. It's a programming error if a caller tries to finish deferred ops using a non-permanent transaction, and prior to this patch t_dfops would be NULL and we'd blow up immediately in xfs_defer_add(..., tp->t_dfops, ...);

Quoted 4 lines

However, now that we initialize t_dfops unconditionally, won't this cause the above programming mistake to leak silently any incorrectly queued defer ops?

Yeah, I'm not sure the previous behavior was really appropriate in that regard either. I'll add the xfs_defer_add() assert suggested by Christoph. We could also add a WARN_ON_ONCE() or something in the commit path to make sure we never see a !perm transaction with dfops, if you think that's useful..?

Brian

Quoted 17 lines

--D

Quoted 15 lines

fs/xfs/xfs_trans.c +1 -1

 		error = xfs_defer_finish_noroll(&tp);
 		if (error) {
 			xfs_defer_cancel(tp);

@@ -1029,7 +1023,7 @@ xfs_trans_cancel(
 
 	trace_xfs_trans_cancel(tp, _RET_IP_);
 
-	if (tp->t_dfops)
+	if (tp->t_flags & XFS_TRANS_PERM_LOG_RES)
 		xfs_defer_cancel(tp);
 
 	/*

Signature · 1 line

2.17.1

Quoted 8 lines

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Log intent recovery is the last user of an external (on-stack) dfops. The pattern exists because the dfops is used to collect additional deferred operations queued during the whole recovery sequence. The dfops is finished with a new transaction after intent recovery completes.

We already have a mechanism to create an empty, container-like transaction to support the scrub infrastructure. We can reuse that mechanism here to drop the final user of external dfops. This facilitates folding dfops state (i.e., dop_low) into the transaction, the elimination of now unused external dfops support and also eliminates the only caller of __xfs_defer_cancel().

Replace the on-stack dfops with an empty transaction and pass it around to the various helpers that queue and finish deferred operations during intent recovery.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_bmap_item.c     | 12 +++++------
 fs/xfs/xfs_bmap_item.h     |  3 +--
 fs/xfs/xfs_log_recover.c   | 42 ++++++++++++++++++--------------------
 fs/xfs/xfs_refcount_item.c | 12 +++++------
 fs/xfs/xfs_refcount_item.h |  3 +--
 5 files changed, 34 insertions(+), 38 deletions(-)

fs/xfs/xfs_bmap_item.c +0 -0

index e1d6c127b07d..992235e7c761 100644

fs/xfs/xfs_bmap_item.c +5 -3

+	struct xfs_bui_log_item		*buip)
 {
 	int				error = 0;
 	unsigned int			bui_type;

@@ -393,6 +392,7 @@ xfs_bui_recover(
 	struct xfs_trans		*tp;
 	struct xfs_inode		*ip = NULL;
 	struct xfs_bmbt_irec		irec;
+	struct xfs_mount		*mp = itp->t_mountp;
 
 	ASSERT(!test_bit(XFS_BUI_RECOVERED, &buip->bui_flags));
 

@@ -446,7 +446,7 @@ xfs_bui_recover(
 	 * finishes them on completion. Transfer current dfops state to this
 	 * transaction and transfer the result back before we return.
 	 */
-	xfs_defer_move(tp->t_dfops, dfops);
+	xfs_defer_move(tp->t_dfops, itp->t_dfops);
 	budp = xfs_trans_get_bud(tp, buip);
 
 	/* Grab the inode. */

@@ -494,7 +494,7 @@ xfs_bui_recover(
 	}
 
 	set_bit(XFS_BUI_RECOVERED, &buip->bui_flags);
-	xfs_defer_move(dfops, tp->t_dfops);
+	xfs_defer_move(itp->t_dfops, tp->t_dfops);
 	error = xfs_trans_commit(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	xfs_irele(ip);

@@ -502,7 +502,7 @@ xfs_bui_recover(
 	return error;
 
 err_inode:
-	xfs_defer_move(dfops, tp->t_dfops);
+	xfs_defer_move(itp->t_dfops, tp->t_dfops);
 	xfs_trans_cancel(tp);
 	if (ip) {
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);

fs/xfs/xfs_bmap_item.h +1 -2

index fd1a1b13df51..ebf6cd6d33e2 100644
@@ -79,7 +79,6 @@ struct xfs_bud_log_item *xfs_bud_init(struct xfs_mount *,
 		struct xfs_bui_log_item *);
 void xfs_bui_item_free(struct xfs_bui_log_item *);
 void xfs_bui_release(struct xfs_bui_log_item *);
-int xfs_bui_recover(struct xfs_mount *mp, struct xfs_bui_log_item *buip,
-		struct xfs_defer_ops *dfops);
+int xfs_bui_recover(struct xfs_trans *itp, struct xfs_bui_log_item *buip);
 
 #endif	/* __XFS_BMAP_ITEM_H__ */

fs/xfs/xfs_log_recover.c +0 -0

index 7776fde9430c..6650b790e43b 100644

fs/xfs/xfs_log_recover.c +9 -11

 	struct xfs_ail			*ailp,
-	struct xfs_log_item		*lip,
-	struct xfs_defer_ops		*dfops)
+	struct xfs_log_item		*lip)
 {
 	struct xfs_cui_log_item		*cuip;
 	int				error;

@@ -4749,7 +4748,7 @@ xlog_recover_process_cui(
 		return 0;
 
 	spin_unlock(&ailp->ail_lock);
-	error = xfs_cui_recover(mp, cuip, dfops);
+	error = xfs_cui_recover(tp, cuip);
 	spin_lock(&ailp->ail_lock);
 
 	return error;

@@ -4774,10 +4773,9 @@ xlog_recover_cancel_cui(
 /* Recover the BUI if necessary. */
 STATIC int
 xlog_recover_process_bui(
-	struct xfs_mount		*mp,
+	struct xfs_trans		*tp,
 	struct xfs_ail			*ailp,
-	struct xfs_log_item		*lip,
-	struct xfs_defer_ops		*dfops)
+	struct xfs_log_item		*lip)
 {
 	struct xfs_bui_log_item		*buip;
 	int				error;

@@ -4790,7 +4788,7 @@ xlog_recover_process_bui(
 		return 0;
 
 	spin_unlock(&ailp->ail_lock);
-	error = xfs_bui_recover(mp, buip, dfops);
+	error = xfs_bui_recover(tp, buip);
 	spin_lock(&ailp->ail_lock);
 
 	return error;

@@ -4829,9 +4827,9 @@ static inline bool xlog_item_is_intent(struct xfs_log_item *lip)
 /* Take all the collected deferred ops and finish them in order. */
 static int
 xlog_finish_defer_ops(
-	struct xfs_mount	*mp,
-	struct xfs_defer_ops	*dfops)
+	struct xfs_trans	*itp)
 {
+	struct xfs_mount	*mp = itp->t_mountp;
 	struct xfs_trans	*tp;
 	int64_t			freeblks;
 	uint			resblks;

@@ -4855,7 +4853,7 @@ xlog_finish_defer_ops(
 	if (error)
 		return error;
 	/* transfer all collected dfops to this transaction */
-	xfs_defer_move(tp->t_dfops, dfops);
+	xfs_defer_move(tp->t_dfops, itp->t_dfops);
 
 	return xfs_trans_commit(tp);
 }

@@ -4880,22 +4878,25 @@ STATIC int
 xlog_recover_process_intents(
 	struct xlog		*log)
 {
-	struct xfs_defer_ops	dfops;
+	struct xfs_trans	*tp;

fs/xfs/xfs_log_recover.c +8 -9

+	if (error)
+		return error;
+
 	ailp = log->l_ailp;
 	spin_lock(&ailp->ail_lock);
 	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
 #if defined(DEBUG) || defined(XFS_WARN)
 	last_lsn = xlog_assign_lsn(log->l_curr_cycle, log->l_curr_block);
 #endif
-	xfs_defer_init(NULL, &dfops);
 	while (lip != NULL) {
 		/*
 		 * We're done when we see something other than an intent.

@@ -4930,12 +4931,10 @@ xlog_recover_process_intents(
 			error = xlog_recover_process_rui(log->l_mp, ailp, lip);
 			break;
 		case XFS_LI_CUI:
-			error = xlog_recover_process_cui(log->l_mp, ailp, lip,
-					&dfops);
+			error = xlog_recover_process_cui(tp, ailp, lip);
 			break;
 		case XFS_LI_BUI:
-			error = xlog_recover_process_bui(log->l_mp, ailp, lip,
-					&dfops);
+			error = xlog_recover_process_bui(tp, ailp, lip);
 			break;
 		}
 		if (error)

@@ -4945,10 +4944,9 @@ xlog_recover_process_intents(
 out:
 	xfs_trans_ail_cursor_done(&cur);
 	spin_unlock(&ailp->ail_lock);
-	if (error)
-		__xfs_defer_cancel(&dfops);
-	else
-		error = xlog_finish_defer_ops(log->l_mp, &dfops);
+	if (!error)
+		error = xlog_finish_defer_ops(tp);
+	xfs_trans_cancel(tp);
 
 	return error;
 }

fs/xfs/xfs_refcount_item.c +6 -6

index d3582a06626f..d663ea1f4076 100644
@@ -380,9 +380,8 @@ xfs_cud_init(
  */
 int
 xfs_cui_recover(
-	struct xfs_mount		*mp,
-	struct xfs_cui_log_item		*cuip,
-	struct xfs_defer_ops		*dfops)
+	struct xfs_trans		*itp,
+	struct xfs_cui_log_item		*cuip)
 {
 	int				i;
 	int				error = 0;

@@ -398,6 +397,7 @@ xfs_cui_recover(
 	xfs_extlen_t			new_len;
 	struct xfs_bmbt_irec		irec;
 	bool				requeue_only = false;
+	struct xfs_mount		*mp = itp->t_mountp;
 
 	ASSERT(!test_bit(XFS_CUI_RECOVERED, &cuip->cui_flags));
 

@@ -457,7 +457,7 @@ xfs_cui_recover(
 	 * finishes them on completion. Transfer current dfops state to this
 	 * transaction and transfer the result back before we return.
 	 */
-	xfs_defer_move(tp->t_dfops, dfops);
+	xfs_defer_move(tp->t_dfops, itp->t_dfops);
 	cudp = xfs_trans_get_cud(tp, cuip);
 
 	for (i = 0; i < cuip->cui_format.cui_nextents; i++) {

@@ -522,13 +522,13 @@ xfs_cui_recover(
 
 	xfs_refcount_finish_one_cleanup(tp, rcur, error);
 	set_bit(XFS_CUI_RECOVERED, &cuip->cui_flags);
-	xfs_defer_move(dfops, tp->t_dfops);
+	xfs_defer_move(itp->t_dfops, tp->t_dfops);
 	error = xfs_trans_commit(tp);
 	return error;
 
 abort_error:
 	xfs_refcount_finish_one_cleanup(tp, rcur, error);
-	xfs_defer_move(dfops, tp->t_dfops);
+	xfs_defer_move(itp->t_dfops, tp->t_dfops);
 	xfs_trans_cancel(tp);
 	return error;
 }

fs/xfs/xfs_refcount_item.h +1 -2

index dd830b69cd1e..98895da52c1f 100644
@@ -82,7 +82,6 @@ struct xfs_cud_log_item *xfs_cud_init(struct xfs_mount *,
 		struct xfs_cui_log_item *);
 void xfs_cui_item_free(struct xfs_cui_log_item *);
 void xfs_cui_release(struct xfs_cui_log_item *);
-int xfs_cui_recover(struct xfs_mount *mp, struct xfs_cui_log_item *cuip,
-		struct xfs_defer_ops *dfops);
+int xfs_cui_recover(struct xfs_trans *itp, struct xfs_cui_log_item *cuip);
 
 #endif	/* __XFS_REFCOUNT_ITEM_H__ */

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

fs/xfs/xfs_bmap_item.c xfs_bud_init( · line 378

Quoted 4 lines

fs/xfs/xfs_bmap_item.c +1 -3

-	struct xfs_mount		*mp,
-	struct xfs_bui_log_item		*buip,
-	struct xfs_defer_ops		*dfops)
+	struct xfs_trans		*itp,

"itp"?

Oh, the intent-holder transaction...

Quoted 31 lines · Brian Foster

Log intent recovery is the last user of an external (on-stack) dfops. The pattern exists because the dfops is used to collect additional deferred operations queued during the whole recovery sequence. The dfops is finished with a new transaction after intent recovery completes.

We already have a mechanism to create an empty, container-like transaction to support the scrub infrastructure. We can reuse that mechanism here to drop the final user of external dfops. This facilitates folding dfops state (i.e., dop_low) into the transaction, the elimination of now unused external dfops support and also eliminates the only caller of __xfs_defer_cancel().

Replace the on-stack dfops with an empty transaction and pass it around to the various helpers that queue and finish deferred operations during intent recovery.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_bmap_item.c     | 12 +++++------
 fs/xfs/xfs_bmap_item.h     |  3 +--
 fs/xfs/xfs_log_recover.c   | 42 ++++++++++++++++++--------------------
 fs/xfs/xfs_refcount_item.c | 12 +++++------
 fs/xfs/xfs_refcount_item.h |  3 +--
 5 files changed, 34 insertions(+), 38 deletions(-)

fs/xfs/xfs_bmap_item.c +0 -0

index e1d6c127b07d..992235e7c761 100644

fs/xfs/xfs_bmap_item.c xfs_bud_init( · line 378

Quoted 7 lines

fs/xfs/xfs_bmap_item.c +1 -3

@@ -375,9 +375,8 @@ xfs_bud_init(
  */
 int
 xfs_bui_recover(
-	struct xfs_mount		*mp,
-	struct xfs_bui_log_item		*buip,
-	struct xfs_defer_ops		*dfops)
+	struct xfs_trans		*itp,

"itp"?

Oh, the intent-holder transaction...

Quoted 4 lines

fs/xfs/xfs_bmap_item.c +1 -0

+	struct xfs_bui_log_item		*buip)
 {
 	int				error = 0;
 	unsigned int			bui_type;

Quoted 4 lines

fs/xfs/xfs_log_recover.c +0 -0

index 7776fde9430c..6650b790e43b 100644

Quoted 15 lines

fs/xfs/xfs_log_recover.c +2 -2

@@ -4880,22 +4878,25 @@ STATIC int
 xlog_recover_process_intents(
 	struct xlog		*log)
 {
-	struct xfs_defer_ops	dfops;
+	struct xfs_trans	*tp;
 	struct xfs_ail_cursor	cur;
 	struct xfs_log_item	*lip;
 	struct xfs_ail		*ailp;
-	int			error = 0;
+	int			error;
 #if defined(DEBUG) || defined(XFS_WARN)
 	xfs_lsn_t		last_lsn;
 #endif
 

fs/xfs/xfs_log_recover.c STATIC int · line 4890

Quoted 1 line

fs/xfs/xfs_log_recover.c +1 -0

+	error = xfs_trans_alloc_empty(log->l_mp, &tp);

Here's where we allocate the intent-placeholder transaction, so why isn't this named "itp" as well?

I think I have a handle on what this patch does, though the variable naming itp/tp is a little confusing.

--D

Quoted 111 lines

fs/xfs/xfs_log_recover.c +8 -9

+	if (error)
+		return error;
+
 	ailp = log->l_ailp;
 	spin_lock(&ailp->ail_lock);
 	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
 #if defined(DEBUG) || defined(XFS_WARN)
 	last_lsn = xlog_assign_lsn(log->l_curr_cycle, log->l_curr_block);
 #endif
-	xfs_defer_init(NULL, &dfops);
 	while (lip != NULL) {
 		/*
 		 * We're done when we see something other than an intent.

@@ -4930,12 +4931,10 @@ xlog_recover_process_intents(
 			error = xlog_recover_process_rui(log->l_mp, ailp, lip);
 			break;
 		case XFS_LI_CUI:
-			error = xlog_recover_process_cui(log->l_mp, ailp, lip,
-					&dfops);
+			error = xlog_recover_process_cui(tp, ailp, lip);
 			break;
 		case XFS_LI_BUI:
-			error = xlog_recover_process_bui(log->l_mp, ailp, lip,
-					&dfops);
+			error = xlog_recover_process_bui(tp, ailp, lip);
 			break;
 		}
 		if (error)

@@ -4945,10 +4944,9 @@ xlog_recover_process_intents(
 out:
 	xfs_trans_ail_cursor_done(&cur);
 	spin_unlock(&ailp->ail_lock);
-	if (error)
-		__xfs_defer_cancel(&dfops);
-	else
-		error = xlog_finish_defer_ops(log->l_mp, &dfops);
+	if (!error)
+		error = xlog_finish_defer_ops(tp);
+	xfs_trans_cancel(tp);
 
 	return error;
 }

fs/xfs/xfs_refcount_item.c +6 -6

index d3582a06626f..d663ea1f4076 100644
@@ -380,9 +380,8 @@ xfs_cud_init(
  */
 int
 xfs_cui_recover(
-	struct xfs_mount		*mp,
-	struct xfs_cui_log_item		*cuip,
-	struct xfs_defer_ops		*dfops)
+	struct xfs_trans		*itp,
+	struct xfs_cui_log_item		*cuip)
 {
 	int				i;
 	int				error = 0;

@@ -398,6 +397,7 @@ xfs_cui_recover(
 	xfs_extlen_t			new_len;
 	struct xfs_bmbt_irec		irec;
 	bool				requeue_only = false;
+	struct xfs_mount		*mp = itp->t_mountp;
 
 	ASSERT(!test_bit(XFS_CUI_RECOVERED, &cuip->cui_flags));
 

@@ -457,7 +457,7 @@ xfs_cui_recover(
 	 * finishes them on completion. Transfer current dfops state to this
 	 * transaction and transfer the result back before we return.
 	 */
-	xfs_defer_move(tp->t_dfops, dfops);
+	xfs_defer_move(tp->t_dfops, itp->t_dfops);
 	cudp = xfs_trans_get_cud(tp, cuip);
 
 	for (i = 0; i < cuip->cui_format.cui_nextents; i++) {

@@ -522,13 +522,13 @@ xfs_cui_recover(
 
 	xfs_refcount_finish_one_cleanup(tp, rcur, error);
 	set_bit(XFS_CUI_RECOVERED, &cuip->cui_flags);
-	xfs_defer_move(dfops, tp->t_dfops);
+	xfs_defer_move(itp->t_dfops, tp->t_dfops);
 	error = xfs_trans_commit(tp);
 	return error;
 
 abort_error:
 	xfs_refcount_finish_one_cleanup(tp, rcur, error);
-	xfs_defer_move(dfops, tp->t_dfops);
+	xfs_defer_move(itp->t_dfops, tp->t_dfops);
 	xfs_trans_cancel(tp);
 	return error;
 }

fs/xfs/xfs_refcount_item.h +1 -2

index dd830b69cd1e..98895da52c1f 100644
@@ -82,7 +82,6 @@ struct xfs_cud_log_item *xfs_cud_init(struct xfs_mount *,
 		struct xfs_cui_log_item *);
 void xfs_cui_item_free(struct xfs_cui_log_item *);
 void xfs_cui_release(struct xfs_cui_log_item *);
-int xfs_cui_recover(struct xfs_mount *mp, struct xfs_cui_log_item *cuip,
-		struct xfs_defer_ops *dfops);
+int xfs_cui_recover(struct xfs_trans *itp, struct xfs_cui_log_item *cuip);
 
 #endif	/* __XFS_REFCOUNT_ITEM_H__ */

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 42 lines · Darrick J. Wong

Quoted 31 lines · Brian Foster

Log intent recovery is the last user of an external (on-stack) dfops. The pattern exists because the dfops is used to collect additional deferred operations queued during the whole recovery sequence. The dfops is finished with a new transaction after intent recovery completes.

We already have a mechanism to create an empty, container-like transaction to support the scrub infrastructure. We can reuse that mechanism here to drop the final user of external dfops. This facilitates folding dfops state (i.e., dop_low) into the transaction, the elimination of now unused external dfops support and also eliminates the only caller of __xfs_defer_cancel().

Replace the on-stack dfops with an empty transaction and pass it around to the various helpers that queue and finish deferred operations during intent recovery.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_bmap_item.c     | 12 +++++------
 fs/xfs/xfs_bmap_item.h     |  3 +--
 fs/xfs/xfs_log_recover.c   | 42 ++++++++++++++++++--------------------
 fs/xfs/xfs_refcount_item.c | 12 +++++------
 fs/xfs/xfs_refcount_item.h |  3 +--
 5 files changed, 34 insertions(+), 38 deletions(-)

fs/xfs/xfs_bmap_item.c +0 -0

index e1d6c127b07d..992235e7c761 100644

fs/xfs/xfs_bmap_item.c xfs_bud_init( · line 378

Quoted 7 lines

fs/xfs/xfs_bmap_item.c +1 -3

@@ -375,9 +375,8 @@ xfs_bud_init(
  */
 int
 xfs_bui_recover(
-	struct xfs_mount		*mp,
-	struct xfs_bui_log_item		*buip,
-	struct xfs_defer_ops		*dfops)
+	struct xfs_trans		*itp,

"itp"?

Quoted 2 lines

Oh, the intent-holder transaction...

Yeah..

Quoted 4 lines

Quoted 4 lines

fs/xfs/xfs_bmap_item.c +1 -0

+	struct xfs_bui_log_item		*buip)
 {
 	int				error = 0;
 	unsigned int			bui_type;

...

Quoted 4 lines

Quoted 4 lines

fs/xfs/xfs_log_recover.c +0 -0

index 7776fde9430c..6650b790e43b 100644

...

Quoted 15 lines

Quoted 15 lines

fs/xfs/xfs_log_recover.c +2 -2

@@ -4880,22 +4878,25 @@ STATIC int
 xlog_recover_process_intents(
 	struct xlog		*log)
 {
-	struct xfs_defer_ops	dfops;
+	struct xfs_trans	*tp;
 	struct xfs_ail_cursor	cur;
 	struct xfs_log_item	*lip;
 	struct xfs_ail		*ailp;
-	int			error = 0;
+	int			error;
 #if defined(DEBUG) || defined(XFS_WARN)
 	xfs_lsn_t		last_lsn;
 #endif
 

Quoted 5 lines

fs/xfs/xfs_log_recover.c STATIC int · line 4890

Quoted 1 line

fs/xfs/xfs_log_recover.c +1 -0

+	error = xfs_trans_alloc_empty(log->l_mp, &tp);

Here's where we allocate the intent-placeholder transaction, so why isn't this named "itp" as well?

I only used itp where tp conflicted with a local transaction (the contexts where we have to transfer state back and forth between two transactions).

Quoted 3 lines

I think I have a handle on what this patch does, though the variable naming itp/tp is a little confusing.

I have no real preference on the naming. I can call it parent_tp as Christoph suggests and use that up and down the chain rather than only in the contexts where both transactions exist if you think that is more clear..?

Brian

Quoted 109 lines

--D

Quoted 107 lines

fs/xfs/xfs_log_recover.c +8 -9

+	if (error)
+		return error;
+
 	ailp = log->l_ailp;
 	spin_lock(&ailp->ail_lock);
 	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
 #if defined(DEBUG) || defined(XFS_WARN)
 	last_lsn = xlog_assign_lsn(log->l_curr_cycle, log->l_curr_block);
 #endif
-	xfs_defer_init(NULL, &dfops);
 	while (lip != NULL) {
 		/*
 		 * We're done when we see something other than an intent.

@@ -4930,12 +4931,10 @@ xlog_recover_process_intents(
 			error = xlog_recover_process_rui(log->l_mp, ailp, lip);
 			break;
 		case XFS_LI_CUI:
-			error = xlog_recover_process_cui(log->l_mp, ailp, lip,
-					&dfops);
+			error = xlog_recover_process_cui(tp, ailp, lip);
 			break;
 		case XFS_LI_BUI:
-			error = xlog_recover_process_bui(log->l_mp, ailp, lip,
-					&dfops);
+			error = xlog_recover_process_bui(tp, ailp, lip);
 			break;
 		}
 		if (error)

@@ -4945,10 +4944,9 @@ xlog_recover_process_intents(
 out:
 	xfs_trans_ail_cursor_done(&cur);
 	spin_unlock(&ailp->ail_lock);
-	if (error)
-		__xfs_defer_cancel(&dfops);
-	else
-		error = xlog_finish_defer_ops(log->l_mp, &dfops);
+	if (!error)
+		error = xlog_finish_defer_ops(tp);
+	xfs_trans_cancel(tp);
 
 	return error;
 }

fs/xfs/xfs_refcount_item.c +6 -6

index d3582a06626f..d663ea1f4076 100644
@@ -380,9 +380,8 @@ xfs_cud_init(
  */
 int
 xfs_cui_recover(
-	struct xfs_mount		*mp,
-	struct xfs_cui_log_item		*cuip,
-	struct xfs_defer_ops		*dfops)
+	struct xfs_trans		*itp,
+	struct xfs_cui_log_item		*cuip)
 {
 	int				i;
 	int				error = 0;

@@ -398,6 +397,7 @@ xfs_cui_recover(
 	xfs_extlen_t			new_len;
 	struct xfs_bmbt_irec		irec;
 	bool				requeue_only = false;
+	struct xfs_mount		*mp = itp->t_mountp;
 
 	ASSERT(!test_bit(XFS_CUI_RECOVERED, &cuip->cui_flags));
 

@@ -457,7 +457,7 @@ xfs_cui_recover(
 	 * finishes them on completion. Transfer current dfops state to this
 	 * transaction and transfer the result back before we return.
 	 */
-	xfs_defer_move(tp->t_dfops, dfops);
+	xfs_defer_move(tp->t_dfops, itp->t_dfops);
 	cudp = xfs_trans_get_cud(tp, cuip);
 
 	for (i = 0; i < cuip->cui_format.cui_nextents; i++) {

@@ -522,13 +522,13 @@ xfs_cui_recover(
 
 	xfs_refcount_finish_one_cleanup(tp, rcur, error);
 	set_bit(XFS_CUI_RECOVERED, &cuip->cui_flags);
-	xfs_defer_move(dfops, tp->t_dfops);
+	xfs_defer_move(itp->t_dfops, tp->t_dfops);
 	error = xfs_trans_commit(tp);
 	return error;
 
 abort_error:
 	xfs_refcount_finish_one_cleanup(tp, rcur, error);
-	xfs_defer_move(dfops, tp->t_dfops);
+	xfs_defer_move(itp->t_dfops, tp->t_dfops);
 	xfs_trans_cancel(tp);
 	return error;
 }

fs/xfs/xfs_refcount_item.h +1 -2

index dd830b69cd1e..98895da52c1f 100644
@@ -82,7 +82,6 @@ struct xfs_cud_log_item *xfs_cud_init(struct xfs_mount *,
 		struct xfs_cui_log_item *);
 void xfs_cui_item_free(struct xfs_cui_log_item *);
 void xfs_cui_release(struct xfs_cui_log_item *);
-int xfs_cui_recover(struct xfs_mount *mp, struct xfs_cui_log_item *cuip,
-		struct xfs_defer_ops *dfops);
+int xfs_cui_recover(struct xfs_trans *itp, struct xfs_cui_log_item *cuip);
 
 #endif	/* __XFS_REFCOUNT_ITEM_H__ */

Signature · 1 line

2.17.1

Quoted 8 lines

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

With no more external dfops users, there is no need for an xfs_defer_ops cancel wrapper.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_defer.c | 5 +++--
 fs/xfs/libxfs/xfs_defer.h | 2 +-
 fs/xfs/xfs_trans.c        | 7 -------
 fs/xfs/xfs_trans.h        | 3 ---
 4 files changed, 4 insertions(+), 13 deletions(-)

fs/xfs/libxfs/xfs_defer.c +3 -2

index a5f7dc18a62f..ebead781613f 100644
@@ -465,9 +465,10 @@ xfs_defer_finish(
  * Free up any items left in the list.
  */
 void
-__xfs_defer_cancel(
-	struct xfs_defer_ops		*dop)
+xfs_defer_cancel(
+	struct xfs_trans		*tp)
 {
+	struct xfs_defer_ops		*dop = tp->t_dfops;
 	struct xfs_defer_pending	*dfp;
 	struct xfs_defer_pending	*pli;
 	struct list_head		*pwi;

fs/xfs/libxfs/xfs_defer.h +1 -1

index 85c41fe4dbae..da145fc04ae1 100644
@@ -50,7 +50,7 @@ void xfs_defer_add(struct xfs_defer_ops *dop, enum xfs_defer_ops_type type,
 		struct list_head *h);
 int xfs_defer_finish_noroll(struct xfs_trans **tp);
 int xfs_defer_finish(struct xfs_trans **tp);
-void __xfs_defer_cancel(struct xfs_defer_ops *dop);
+void xfs_defer_cancel(struct xfs_trans *);
 void xfs_defer_init(struct xfs_trans *tp, struct xfs_defer_ops *dop);
 bool xfs_defer_has_unfinished_work(struct xfs_defer_ops *dop);
 int xfs_defer_ijoin(struct xfs_defer_ops *dop, struct xfs_inode *ip);

fs/xfs/xfs_trans.c +0 -7

index 8d3b7f28b193..b0eeeaf6d24e 100644
@@ -1105,10 +1105,3 @@ xfs_trans_roll(
 	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
 	return xfs_trans_reserve(*tpp, &tres, 0, 0);
 }
-
-void
-xfs_defer_cancel(
-	struct xfs_trans	*tp)
-{
-	__xfs_defer_cancel(tp->t_dfops);
-}

fs/xfs/xfs_trans.h +0 -3

index 5170e89bec02..dc79e3c1d3e8 100644
@@ -214,9 +214,6 @@ xfs_trans_read_buf(
 				      flags, bpp, ops);
 }
 
-/* cancel dfops associated with a transaction */
-void xfs_defer_cancel(struct xfs_trans *);
-
 struct xfs_buf	*xfs_trans_getsb(xfs_trans_t *, struct xfs_mount *, int);
 
 void		xfs_trans_brelse(xfs_trans_t *, struct xfs_buf *);

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

With no more external dfops users, there is no need for an xfs_defer_ops cancel wrapper.

Signed-off-by Brian Foster <[email protected]>

All callers pass ->t_dfops of the associated transactions. Refactor the helpers to receive the transactions and facilitate further cleanups between xfs_defer_ops and xfs_trans.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_defer.c  | 15 ++++++++++-----
 fs/xfs/libxfs/xfs_defer.h  |  2 +-
 fs/xfs/xfs_bmap_item.c     |  6 +++---
 fs/xfs/xfs_log_recover.c   |  2 +-
 fs/xfs/xfs_refcount_item.c |  6 +++---
 fs/xfs/xfs_trans.c         |  2 +-
 6 files changed, 19 insertions(+), 14 deletions(-)

fs/xfs/libxfs/xfs_defer.c +10 -5

index ebead781613f..e3517a53c525 100644
@@ -324,9 +324,12 @@ xfs_defer_bjoin(
  */
 static void
 xfs_defer_reset(
-	struct xfs_defer_ops	*dop)
+	struct xfs_trans	*tp)
 {
+	struct xfs_defer_ops	*dop = tp->t_dfops;
+
 	ASSERT(!xfs_defer_has_unfinished_work(dop));
+
 	dop->dop_low = false;
 	memset(dop->dop_inodes, 0, sizeof(dop->dop_inodes));
 	memset(dop->dop_bufs, 0, sizeof(dop->dop_bufs));

@@ -457,7 +460,7 @@ xfs_defer_finish(
 		if (error)
 			return error;
 	}
-	xfs_defer_reset((*tp)->t_dfops);
+	xfs_defer_reset(*tp);
 	return 0;
 }
 

@@ -575,9 +578,11 @@ xfs_defer_init(
  */
 void
 xfs_defer_move(
-	struct xfs_defer_ops	*dst,
-	struct xfs_defer_ops	*src)
+	struct xfs_trans	*dtp,
+	struct xfs_trans	*stp)
 {
+	struct xfs_defer_ops	*dst = dtp->t_dfops;
+	struct xfs_defer_ops	*src = stp->t_dfops;
 	ASSERT(dst != src);
 
 	list_splice_init(&src->dop_intake, &dst->dop_intake);

@@ -587,5 +592,5 @@ xfs_defer_move(
 	memcpy(dst->dop_bufs, src->dop_bufs, sizeof(dst->dop_bufs));
 	dst->dop_low = src->dop_low;
 
-	xfs_defer_reset(src);
+	xfs_defer_reset(stp);
 }

fs/xfs/libxfs/xfs_defer.h +1 -1

index da145fc04ae1..d60c50498fdf 100644
@@ -55,7 +55,7 @@ void xfs_defer_init(struct xfs_trans *tp, struct xfs_defer_ops *dop);
 bool xfs_defer_has_unfinished_work(struct xfs_defer_ops *dop);
 int xfs_defer_ijoin(struct xfs_defer_ops *dop, struct xfs_inode *ip);
 int xfs_defer_bjoin(struct xfs_defer_ops *dop, struct xfs_buf *bp);
-void xfs_defer_move(struct xfs_defer_ops *dst, struct xfs_defer_ops *src);
+void xfs_defer_move(struct xfs_trans *dtp, struct xfs_trans *stp);
 
 /* Description of a deferred type. */
 struct xfs_defer_op_type {

fs/xfs/xfs_bmap_item.c +3 -3

index 992235e7c761..4b4318ea4b76 100644
@@ -446,7 +446,7 @@ xfs_bui_recover(
 	 * finishes them on completion. Transfer current dfops state to this
 	 * transaction and transfer the result back before we return.
 	 */
-	xfs_defer_move(tp->t_dfops, itp->t_dfops);
+	xfs_defer_move(tp, itp);
 	budp = xfs_trans_get_bud(tp, buip);
 
 	/* Grab the inode. */

@@ -494,7 +494,7 @@ xfs_bui_recover(
 	}
 
 	set_bit(XFS_BUI_RECOVERED, &buip->bui_flags);
-	xfs_defer_move(itp->t_dfops, tp->t_dfops);
+	xfs_defer_move(itp, tp);
 	error = xfs_trans_commit(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	xfs_irele(ip);

@@ -502,7 +502,7 @@ xfs_bui_recover(
 	return error;
 
 err_inode:
-	xfs_defer_move(itp->t_dfops, tp->t_dfops);
+	xfs_defer_move(itp, tp);
 	xfs_trans_cancel(tp);
 	if (ip) {
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);

fs/xfs/xfs_log_recover.c +1 -1

index 6650b790e43b..b4edd93a18e9 100644
@@ -4853,7 +4853,7 @@ xlog_finish_defer_ops(
 	if (error)
 		return error;
 	/* transfer all collected dfops to this transaction */
-	xfs_defer_move(tp->t_dfops, itp->t_dfops);
+	xfs_defer_move(tp, itp);
 
 	return xfs_trans_commit(tp);
 }

fs/xfs/xfs_refcount_item.c +3 -3

index d663ea1f4076..829b7a5bc707 100644
@@ -457,7 +457,7 @@ xfs_cui_recover(
 	 * finishes them on completion. Transfer current dfops state to this
 	 * transaction and transfer the result back before we return.
 	 */
-	xfs_defer_move(tp->t_dfops, itp->t_dfops);
+	xfs_defer_move(tp, itp);
 	cudp = xfs_trans_get_cud(tp, cuip);
 
 	for (i = 0; i < cuip->cui_format.cui_nextents; i++) {

@@ -522,13 +522,13 @@ xfs_cui_recover(
 
 	xfs_refcount_finish_one_cleanup(tp, rcur, error);
 	set_bit(XFS_CUI_RECOVERED, &cuip->cui_flags);
-	xfs_defer_move(itp->t_dfops, tp->t_dfops);
+	xfs_defer_move(itp, tp);
 	error = xfs_trans_commit(tp);
 	return error;
 
 abort_error:
 	xfs_refcount_finish_one_cleanup(tp, rcur, error);
-	xfs_defer_move(itp->t_dfops, tp->t_dfops);
+	xfs_defer_move(itp, tp);
 	xfs_trans_cancel(tp);
 	return error;
 }

fs/xfs/xfs_trans.c +1 -1

index b0eeeaf6d24e..8c088568b05d 100644
@@ -125,7 +125,7 @@ xfs_trans_dup(
 	if (tp->t_dfops != &tp->t_dfops_internal)
 		ntp->t_dfops = tp->t_dfops;
 	else
-		xfs_defer_move(ntp->t_dfops, tp->t_dfops);
+		xfs_defer_move(ntp, tp);
 
 	xfs_trans_dup_dqinfo(tp, ntp);
 

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

All callers pass ->t_dfops of the associated transactions. Refactor the helpers to receive the transactions and facilitate further cleanups between xfs_defer_ops and xfs_trans.

Signed-off-by Brian Foster <[email protected]>

The dop_low field enables the low free space allocation mode when a previous allocation has detected difficulty allocating blocks. It has historically been part of the xfs_defer_ops structure, which means if enabled, it remains enabled across a set of transactions until the deferred operations have completed and the dfops is reset.

Now that the dfops is embedded in the transaction, we can save a bit more space by using a transaction flag rather than a standalone boolean. Drop the ->dop_low field and replace it with a transaction flag that is set at the same points, carried across rolling transactions and cleared on completion of deferred operations. This essentially emulates the behavior of ->dop_low and so should not change behavior.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_bmap.c       |  8 ++++----
 fs/xfs/libxfs/xfs_bmap_btree.c |  4 ++--
 fs/xfs/libxfs/xfs_defer.c      | 16 ++++++++++++++--
 fs/xfs/libxfs/xfs_defer.h      | 11 -----------
 fs/xfs/libxfs/xfs_shared.h     | 12 ++++++++++++
 fs/xfs/xfs_filestream.c        |  3 ++-
 fs/xfs/xfs_trace.h             | 10 ++--------
 fs/xfs/xfs_trans.h             |  2 --
 8 files changed, 36 insertions(+), 30 deletions(-)

fs/xfs/libxfs/xfs_bmap.c +4 -4

index a85c0445b38f..8edf7522aaff 100644
@@ -700,7 +700,7 @@ xfs_bmap_extents_to_btree(
 	if (tp->t_firstblock == NULLFSBLOCK) {
 		args.type = XFS_ALLOCTYPE_START_BNO;
 		args.fsbno = XFS_INO_TO_FSB(mp, ip->i_ino);
-	} else if (tp->t_dfops->dop_low) {
+	} else if (tp->t_flags & XFS_TRANS_LOWMODE) {
 		args.type = XFS_ALLOCTYPE_START_BNO;
 		args.fsbno = tp->t_firstblock;
 	} else {

@@ -3449,7 +3449,7 @@ xfs_bmap_btalloc(
 			error = xfs_bmap_btalloc_nullfb(ap, &args, &blen);
 		if (error)
 			return error;
-	} else if (ap->tp->t_dfops->dop_low) {
+	} else if (ap->tp->t_flags & XFS_TRANS_LOWMODE) {
 		if (xfs_inode_is_filestream(ap->ip))
 			args.type = XFS_ALLOCTYPE_FIRST_AG;
 		else

@@ -3484,7 +3484,7 @@ xfs_bmap_btalloc(
 	 * is >= the stripe unit and the allocation offset is
 	 * at the end of file.
 	 */
-	if (!ap->tp->t_dfops->dop_low && ap->aeof) {
+	if (!(ap->tp->t_flags & XFS_TRANS_LOWMODE) && ap->aeof) {
 		if (!ap->offset) {
 			args.alignment = stripe_align;
 			atype = args.type;

@@ -3576,7 +3576,7 @@ xfs_bmap_btalloc(
 		args.total = ap->minlen;
 		if ((error = xfs_alloc_vextent(&args)))
 			return error;
-		ap->tp->t_dfops->dop_low = true;
+		ap->tp->t_flags |= XFS_TRANS_LOWMODE;
 	}
 	if (args.fsbno != NULLFSBLOCK) {
 		/*

fs/xfs/libxfs/xfs_bmap_btree.c +2 -2

index 01489714a253..955e29de8cae 100644
@@ -226,7 +226,7 @@ xfs_bmbt_alloc_block(
 		 * block allocation here and corrupt the filesystem.
 		 */
 		args.minleft = args.tp->t_blk_res;
-	} else if (cur->bc_tp->t_dfops->dop_low) {
+	} else if (cur->bc_tp->t_flags & XFS_TRANS_LOWMODE) {
 		args.type = XFS_ALLOCTYPE_START_BNO;
 	} else {
 		args.type = XFS_ALLOCTYPE_NEAR_BNO;

@@ -253,7 +253,7 @@ xfs_bmbt_alloc_block(
 		error = xfs_alloc_vextent(&args);
 		if (error)
 			goto error0;
-		cur->bc_tp->t_dfops->dop_low = true;
+		cur->bc_tp->t_flags |= XFS_TRANS_LOWMODE;
 	}
 	if (WARN_ON_ONCE(args.fsbno == NULLFSBLOCK)) {
 		*stat = 0;

fs/xfs/libxfs/xfs_defer.c +14 -2

index e3517a53c525..0306187b5f56 100644
@@ -330,9 +330,14 @@ xfs_defer_reset(
 
 	ASSERT(!xfs_defer_has_unfinished_work(dop));
 
-	dop->dop_low = false;
 	memset(dop->dop_inodes, 0, sizeof(dop->dop_inodes));
 	memset(dop->dop_bufs, 0, sizeof(dop->dop_bufs));
+
+	/*
+	 * Low mode state transfers across transaction rolls to mirror dfops
+	 * lifetime. Clear it now that dfops is reset.
+	 */
+	tp->t_flags &= ~XFS_TRANS_LOWMODE;
 }
 
 /*

@@ -590,7 +595,14 @@ xfs_defer_move(
 
 	memcpy(dst->dop_inodes, src->dop_inodes, sizeof(dst->dop_inodes));
 	memcpy(dst->dop_bufs, src->dop_bufs, sizeof(dst->dop_bufs));
-	dst->dop_low = src->dop_low;
+
+	/*
+	 * Low free space mode was historically controlled by a dfops field.
+	 * This meant that low mode state potentially carried across multiple
+	 * transaction rolls. Transfer low mode on a dfops move to preserve
+	 * that behavior.
+	 */
+	dtp->t_flags |= stp->t_flags & XFS_TRANS_LOWMODE;
 
 	xfs_defer_reset(stp);
 }

fs/xfs/libxfs/xfs_defer.h +0 -11

index d60c50498fdf..8908a2716774 100644
@@ -25,17 +25,6 @@ struct xfs_defer_pending {
 
 /*
  * Header for deferred operation list.
- *
- * dop_low is used by the allocator to activate the lowspace algorithm -
- * when free space is running low the extent allocator may choose to
- * allocate an extent from an AG without leaving sufficient space for
- * a btree split when inserting the new extent.  In this case the allocator
- * will enable the lowspace algorithm which is supposed to allow further
- * allocations (such as btree splits and newroots) to allocate from
- * sequential AGs.  In order to avoid locking AGs out of order the lowspace
- * algorithm will start searching for free space from AG 0.  If the correct
- * transaction reservations have been made then this algorithm will eventually
- * find all the space it needs.
  */
 enum xfs_defer_ops_type {
 	XFS_DEFER_OPS_TYPE_BMAP,

fs/xfs/libxfs/xfs_shared.h +12 -0

index 22089f1c880a..1c5debe748f0 100644
@@ -64,6 +64,18 @@ void	xfs_log_get_max_trans_res(struct xfs_mount *mp,
 #define XFS_TRANS_RESERVE	0x20    /* OK to use reserved data blocks */
 #define XFS_TRANS_NO_WRITECOUNT 0x40	/* do not elevate SB writecount */
 #define XFS_TRANS_NOFS		0x80	/* pass KM_NOFS to kmem_alloc */
+/*
+ * LOWMODE is used by the allocator to activate the lowspace algorithm - when
+ * free space is running low the extent allocator may choose to allocate an
+ * extent from an AG without leaving sufficient space for a btree split when
+ * inserting the new extent. In this case the allocator will enable the
+ * lowspace algorithm which is supposed to allow further allocations (such as
+ * btree splits and newroots) to allocate from sequential AGs. In order to
+ * avoid locking AGs out of order the lowspace algorithm will start searching
+ * for free space from AG 0. If the correct transaction reservations have been
+ * made then this algorithm will eventually find all the space it needs.
+ */
+#define XFS_TRANS_LOWMODE	0x100	/* allocate in low space mode */
 
 /*
  * Field values for xfs_trans_mod_sb.

fs/xfs/xfs_filestream.c +2 -1

index 212173c62588..182501373af2 100644
@@ -20,6 +20,7 @@
 #include "xfs_trace.h"
 #include "xfs_ag_resv.h"
 #include "xfs_trans.h"
+#include "xfs_shared.h"
 
 struct xfs_fstrm_item {
 	struct xfs_mru_cache_elem	mru;

@@ -378,7 +379,7 @@ xfs_filestream_new_ag(
 
 	if (xfs_alloc_is_userdata(ap->datatype))
 		flags |= XFS_PICK_USERDATA;
-	if (ap->tp->t_dfops->dop_low)
+	if (ap->tp->t_flags & XFS_TRANS_LOWMODE)
 		flags |= XFS_PICK_LOWSPACE;
 
 	err = xfs_filestream_pick_ag(pip, startag, agp, flags, minlen);

fs/xfs/xfs_trace.h +2 -8

index cc6995cfce66..8807f1bb814a 100644
@@ -2223,19 +2223,16 @@ DECLARE_EVENT_CLASS(xfs_defer_class,
 		__field(dev_t, dev)
 		__field(void *, dop)
 		__field(char, committed)
-		__field(char, low)
 		__field(unsigned long, caller_ip)
 	),
 	TP_fast_assign(
 		__entry->dev = mp ? mp->m_super->s_dev : 0;
 		__entry->dop = dop;
-		__entry->low = dop->dop_low;
 		__entry->caller_ip = caller_ip;
 	),
-	TP_printk("dev %d:%d ops %p low %d, caller %pS",
+	TP_printk("dev %d:%d ops %p caller %pS",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->dop,
-		  __entry->low,
 		  (char *)__entry->caller_ip)
 )
 #define DEFINE_DEFER_EVENT(name) \

@@ -2251,19 +2248,16 @@ DECLARE_EVENT_CLASS(xfs_defer_error_class,
 		__field(dev_t, dev)
 		__field(void *, dop)
 		__field(char, committed)
-		__field(char, low)
 		__field(int, error)
 	),
 	TP_fast_assign(
 		__entry->dev = mp ? mp->m_super->s_dev : 0;
 		__entry->dop = dop;
-		__entry->low = dop->dop_low;
 		__entry->error = error;
 	),
-	TP_printk("dev %d:%d ops %p low %d err %d",
+	TP_printk("dev %d:%d ops %p err %d",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->dop,
-		  __entry->low,
 		  __entry->error)
 )
 #define DEFINE_DEFER_ERROR_EVENT(name) \

fs/xfs/xfs_trans.h +0 -2

index dc79e3c1d3e8..7e493221160e 100644
@@ -101,8 +101,6 @@ struct xfs_defer_ops {
 	/* relog these with each roll */
 	struct xfs_inode	*dop_inodes[XFS_DEFER_OPS_NR_INODES];
 	struct xfs_buf		*dop_bufs[XFS_DEFER_OPS_NR_BUFS];
-
-	bool			dop_low;	/* alloc in low mode */
 };
 
 /*

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

fs/xfs/libxfs/xfs_defer.c xfs_defer_move( · line 605

Quoted 7 lines

fs/xfs/libxfs/xfs_defer.c +7 -0

+	/*
+	 * Low free space mode was historically controlled by a dfops field.
+	 * This meant that low mode state potentially carried across multiple
+	 * transaction rolls. Transfer low mode on a dfops move to preserve
+	 * that behavior.
+	 */
+	dtp->t_flags |= stp->t_flags & XFS_TRANS_LOWMODE;

Maybe add parentheses here for clarity.

Except for that:

Reviewed-by Christoph Hellwig <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Log items that require relogging during deferred operations processing are explicitly joined to the associated dfops via the xfs_defer_*join() helpers. These calls imply that the associated object is "held" by the transaction such that when rolled, the item can be immediately joined to a follow up transaction. For buffers, this means the buffer remains locked and held after each roll. For inodes, this means that the inode remains locked.

Failure to join a held item to the dfops structure means the associated object pins the tail of the log while dfops processing completes, because the item never relogs and is not unlocked or released until deferred processing completes.

Currently, all buffers that are held in transactions (XFS_BLI_HOLD) with deferred operations are explicitly joined to the dfops. This is not the case for inodes, however, as various contexts defer operations to transactions with held inodes without explicit joins to the associated dfops (and thus not relogging).

While this is not a catastrophic problem, it is not ideal. Given that we want to eventually relog such items automatically during dfops processing, start by explicitly adding these missing xfs_defer_ijoin() calls. A call is added everywhere an inode is joined to a transaction without transferring lock ownership and said transaction runs deferred operations.

All xfs_defer_ijoin() calls will eventually be replaced by automatic dfops inode relogging. This patch essentially implements the behavior change that would otherwise occur due to automatic inode dfops relogging.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_bmap.c | 1 +
 fs/xfs/xfs_bmap_util.c   | 1 +
 fs/xfs/xfs_inode.c       | 1 +
 fs/xfs/xfs_iomap.c       | 3 +++
 fs/xfs/xfs_reflink.c     | 1 +
 5 files changed, 7 insertions(+)

fs/xfs/libxfs/xfs_bmap.c +1 -0

index 8edf7522aaff..71687d805f79 100644
@@ -1119,6 +1119,7 @@ xfs_bmap_add_attrfork(
 			xfs_log_sb(tp);
 	}
 
+	xfs_defer_ijoin(tp->t_dfops, ip);
 	error = xfs_trans_commit(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	return error;

fs/xfs/xfs_bmap_util.c +1 -0

index 412dc58ae54d..0c58a66b39e5 100644
@@ -979,6 +979,7 @@ xfs_alloc_file_space(
 		/*
 		 * Complete the transaction
 		 */
+		xfs_defer_ijoin(tp->t_dfops, ip);
 		error = xfs_trans_commit(tp);
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
 		if (error)

fs/xfs/xfs_inode.c +1 -0

index 5fc1815c2b62..441c8593cfd7 100644
@@ -1810,6 +1810,7 @@ xfs_inactive_ifree(
 	 * Just ignore errors at this point.  There is nothing we can do except
 	 * to try to keep going. Make sure it's not a silent error.
 	 */
+	xfs_defer_ijoin(tp->t_dfops, ip);
 	error = xfs_trans_commit(tp);
 	if (error)
 		xfs_notice(mp, "%s: xfs_trans_commit returned error %d",

fs/xfs/xfs_iomap.c +3 -0

index 8e8ca9f03f0e..464b4b3225bb 100644
@@ -261,6 +261,7 @@ xfs_iomap_write_direct(
 	/*
 	 * Complete the transaction
 	 */
+	xfs_defer_ijoin(tp->t_dfops, ip);
 	error = xfs_trans_commit(tp);
 	if (error)
 		goto out_unlock;

@@ -762,6 +763,7 @@ xfs_iomap_write_allocate(
 			if (error)
 				goto trans_cancel;
 
+			xfs_defer_ijoin(tp->t_dfops, ip);
 			error = xfs_trans_commit(tp);
 			if (error)
 				goto error0;

@@ -879,6 +881,7 @@ xfs_iomap_write_unwritten(
 			xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 		}
 
+		xfs_defer_ijoin(tp->t_dfops, ip);
 		error = xfs_trans_commit(tp);
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
 		if (error)

fs/xfs/xfs_reflink.c +1 -0

index 9a0a56526266..e986fcf928e5 100644
@@ -435,6 +435,7 @@ xfs_reflink_allocate_cow(
 	xfs_inode_set_cowblocks_tag(ip);
 
 	/* Finish up. */
+	xfs_defer_ijoin(tp->t_dfops, ip);
 	error = xfs_trans_commit(tp);
 	if (error)
 		return error;

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Buffers that are held across deferred operations are explicitly joined to the dfops structure to ensure appropriate relogging. While buffers are currently joined explicitly, we can detect the conditions that require relogging at dfops finish time by inspecting the transaction item list for held buffers.

Replace the xfs_defer_bjoin() infrastructure with such detection and automatic relogging of held buffers. This eliminates the need for the per-dfops buffer list, replaced by an on-stack variant in xfs_defer_trans_roll().

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_attr.c  |  1 -
 fs/xfs/libxfs/xfs_defer.c | 55 ++++++++++++++++-----------------------
 fs/xfs/libxfs/xfs_defer.h |  1 -
 fs/xfs/xfs_dquot.c        |  1 -
 fs/xfs/xfs_trans.h        |  1 -
 5 files changed, 23 insertions(+), 36 deletions(-)

fs/xfs/libxfs/xfs_attr.c +0 -1

index 3deb5cdadf08..227887bee00d 100644
@@ -320,7 +320,6 @@ xfs_attr_set(
 		 * buffer and run into problems with the write verifier.
 		 */
 		xfs_trans_bhold(args.trans, leaf_bp);
-		xfs_defer_bjoin(args.trans->t_dfops, leaf_bp);
 		xfs_defer_ijoin(args.trans->t_dfops, dp);
 		error = xfs_defer_finish(&args.trans);
 		if (error)

fs/xfs/libxfs/xfs_defer.c +5 -0

index 0306187b5f56..d0ee55048a7a 100644
@@ -14,6 +14,7 @@
 #include "xfs_mount.h"
 #include "xfs_defer.h"
 #include "xfs_trans.h"
+#include "xfs_buf_item.h"
 #include "xfs_trace.h"
 
 /*

@@ -228,6 +229,10 @@ xfs_defer_trans_roll(
 	struct xfs_trans		**tp)
 {
 	struct xfs_defer_ops		*dop = (*tp)->t_dfops;
+	struct xfs_buf_log_item		*bli;
+	struct xfs_log_item		*lip;
+	struct xfs_buf			*bplist[XFS_DEFER_OPS_NR_BUFS];
+	int				bpcount = 0;
 	int				i;
 	int				error;
 

@@ -235,9 +240,21 @@ xfs_defer_trans_roll(
 	for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)
 		xfs_trans_log_inode(*tp, dop->dop_inodes[i], XFS_ILOG_CORE);
 

fs/xfs/libxfs/xfs_defer.c +11 -29

+				xfs_trans_dirty_buf(*tp, bli->bli_buf);
+				bplist[bpcount++] = bli->bli_buf;
+			}
+			break;
+		default:
+			break;
+		}
+	}
 
 	trace_xfs_defer_trans_roll((*tp)->t_mountp, dop, _RET_IP_);
 

@@ -255,9 +272,9 @@ xfs_defer_trans_roll(
 		xfs_trans_ijoin(*tp, dop->dop_inodes[i], 0);
 
 	/* Rejoin the buffers and dirty them so the log moves forward. */
-	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++) {
-		xfs_trans_bjoin(*tp, dop->dop_bufs[i]);
-		xfs_trans_bhold(*tp, dop->dop_bufs[i]);
+	for (i = 0; i < bpcount; i++) {
+		xfs_trans_bjoin(*tp, bplist[i]);
+		xfs_trans_bhold(*tp, bplist[i]);
 	}
 
 	return error;

@@ -295,30 +312,6 @@ xfs_defer_ijoin(
 	return -EFSCORRUPTED;
 }
 
-/*
- * Add this buffer to the deferred op.  Each joined buffer is relogged
- * each time we roll the transaction.
- */
-int
-xfs_defer_bjoin(
-	struct xfs_defer_ops		*dop,
-	struct xfs_buf			*bp)
-{
-	int				i;
-
-	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS; i++) {
-		if (dop->dop_bufs[i] == bp)
-			return 0;
-		else if (dop->dop_bufs[i] == NULL) {
-			dop->dop_bufs[i] = bp;
-			return 0;
-		}
-	}
-
-	ASSERT(0);
-	return -EFSCORRUPTED;
-}
-
 /*
  * Reset an already used dfops after finish.
  */

@@ -331,7 +324,6 @@ xfs_defer_reset(
 	ASSERT(!xfs_defer_has_unfinished_work(dop));
 
 	memset(dop->dop_inodes, 0, sizeof(dop->dop_inodes));
-	memset(dop->dop_bufs, 0, sizeof(dop->dop_bufs));
 
 	/*
 	 * Low mode state transfers across transaction rolls to mirror dfops

@@ -594,7 +586,6 @@ xfs_defer_move(
 	list_splice_init(&src->dop_pending, &dst->dop_pending);
 
 	memcpy(dst->dop_inodes, src->dop_inodes, sizeof(dst->dop_inodes));
-	memcpy(dst->dop_bufs, src->dop_bufs, sizeof(dst->dop_bufs));
 
 	/*
 	 * Low free space mode was historically controlled by a dfops field.

fs/xfs/libxfs/xfs_defer.h +0 -1

index 8908a2716774..4a8bb838adf2 100644
@@ -43,7 +43,6 @@ void xfs_defer_cancel(struct xfs_trans *);
 void xfs_defer_init(struct xfs_trans *tp, struct xfs_defer_ops *dop);
 bool xfs_defer_has_unfinished_work(struct xfs_defer_ops *dop);
 int xfs_defer_ijoin(struct xfs_defer_ops *dop, struct xfs_inode *ip);
-int xfs_defer_bjoin(struct xfs_defer_ops *dop, struct xfs_buf *bp);
 void xfs_defer_move(struct xfs_trans *dtp, struct xfs_trans *stp);
 
 /* Description of a deferred type. */

fs/xfs/xfs_dquot.c +0 -1

index da5c55cec966..e1196854dbcd 100644
@@ -362,7 +362,6 @@ xfs_dquot_disk_alloc(
 	 * manually or by committing the transaction.
 	 */
 	xfs_trans_bhold(tp, bp);
-	error = xfs_defer_bjoin(tp->t_dfops, bp);
 	if (error) {
 		xfs_trans_bhold_release(tp, bp);
 		xfs_trans_brelse(tp, bp);

fs/xfs/xfs_trans.h +0 -1

index 7e493221160e..581456c79197 100644
@@ -100,7 +100,6 @@ struct xfs_defer_ops {
 
 	/* relog these with each roll */
 	struct xfs_inode	*dop_inodes[XFS_DEFER_OPS_NR_INODES];
-	struct xfs_buf		*dop_bufs[XFS_DEFER_OPS_NR_BUFS];
 };
 
 /*

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 20 lines · Brian Foster

Buffers that are held across deferred operations are explicitly joined to the dfops structure to ensure appropriate relogging. While buffers are currently joined explicitly, we can detect the conditions that require relogging at dfops finish time by inspecting the transaction item list for held buffers.

Replace the xfs_defer_bjoin() infrastructure with such detection and automatic relogging of held buffers. This eliminates the need for the per-dfops buffer list, replaced by an on-stack variant in xfs_defer_trans_roll().

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_attr.c  |  1 -
 fs/xfs/libxfs/xfs_defer.c | 55 ++++++++++++++++-----------------------
 fs/xfs/libxfs/xfs_defer.h |  1 -
 fs/xfs/xfs_dquot.c        |  1 -
 fs/xfs/xfs_trans.h        |  1 -
 5 files changed, 23 insertions(+), 36 deletions(-)

Quoted 4 lines

fs/xfs/libxfs/xfs_defer.c +0 -0

index 0306187b5f56..d0ee55048a7a 100644

Quoted 4 lines

fs/xfs/libxfs/xfs_defer.c +0 -0

@@ -235,9 +240,21 @@ xfs_defer_trans_roll(
 	for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)
 		xfs_trans_log_inode(*tp, dop->dop_inodes[i], XFS_ILOG_CORE);
 

fs/xfs/libxfs/xfs_defer.c xfs_defer_trans_roll( · line 249

Quoted 10 lines

fs/xfs/libxfs/xfs_defer.c +7 -3

-	/* Hold the (previously bjoin'd) buffer locked across the roll. */
-	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++)
-		xfs_trans_dirty_buf(*tp, dop->dop_bufs[i]);
+	list_for_each_entry(lip, &(*tp)->t_items, li_trans) {
+		switch (lip->li_type) {
+		case XFS_LI_BUF:
+			bli = container_of(lip, struct xfs_buf_log_item,
+					   bli_item);
+			if (bli->bli_flags & XFS_BLI_HOLD) {
+				ASSERT(bpcount < XFS_DEFER_OPS_NR_BUFS);

This is a bit of a behavior change -- previously if we oveflowed the array we'd bail out with -EFSCORRUPTED rather than go corrupting whatever came after dop_bufs. Granted, we were pretty terrible about checking the return values, but I think this ought to be:

if (bpcount >= XFS_DEFER_OPS_NR_BUFS) {
	ASSERT(0);
	return -EFSCORRUPTED;
}

rather than corrupting the on-stack array if we happen to bhold too many buffers to the transaction.

--D

Quoted 114 lines

fs/xfs/libxfs/xfs_defer.c +11 -29

+				xfs_trans_dirty_buf(*tp, bli->bli_buf);
+				bplist[bpcount++] = bli->bli_buf;
+			}
+			break;
+		default:
+			break;
+		}
+	}
 
 	trace_xfs_defer_trans_roll((*tp)->t_mountp, dop, _RET_IP_);
 

@@ -255,9 +272,9 @@ xfs_defer_trans_roll(
 		xfs_trans_ijoin(*tp, dop->dop_inodes[i], 0);
 
 	/* Rejoin the buffers and dirty them so the log moves forward. */
-	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++) {
-		xfs_trans_bjoin(*tp, dop->dop_bufs[i]);
-		xfs_trans_bhold(*tp, dop->dop_bufs[i]);
+	for (i = 0; i < bpcount; i++) {
+		xfs_trans_bjoin(*tp, bplist[i]);
+		xfs_trans_bhold(*tp, bplist[i]);
 	}
 
 	return error;

@@ -295,30 +312,6 @@ xfs_defer_ijoin(
 	return -EFSCORRUPTED;
 }
 
-/*
- * Add this buffer to the deferred op.  Each joined buffer is relogged
- * each time we roll the transaction.
- */
-int
-xfs_defer_bjoin(
-	struct xfs_defer_ops		*dop,
-	struct xfs_buf			*bp)
-{
-	int				i;
-
-	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS; i++) {
-		if (dop->dop_bufs[i] == bp)
-			return 0;
-		else if (dop->dop_bufs[i] == NULL) {
-			dop->dop_bufs[i] = bp;
-			return 0;
-		}
-	}
-
-	ASSERT(0);
-	return -EFSCORRUPTED;
-}
-
 /*
  * Reset an already used dfops after finish.
  */

@@ -331,7 +324,6 @@ xfs_defer_reset(
 	ASSERT(!xfs_defer_has_unfinished_work(dop));
 
 	memset(dop->dop_inodes, 0, sizeof(dop->dop_inodes));
-	memset(dop->dop_bufs, 0, sizeof(dop->dop_bufs));
 
 	/*
 	 * Low mode state transfers across transaction rolls to mirror dfops

@@ -594,7 +586,6 @@ xfs_defer_move(
 	list_splice_init(&src->dop_pending, &dst->dop_pending);
 
 	memcpy(dst->dop_inodes, src->dop_inodes, sizeof(dst->dop_inodes));
-	memcpy(dst->dop_bufs, src->dop_bufs, sizeof(dst->dop_bufs));
 
 	/*
 	 * Low free space mode was historically controlled by a dfops field.

fs/xfs/libxfs/xfs_defer.h +0 -1

index 8908a2716774..4a8bb838adf2 100644
@@ -43,7 +43,6 @@ void xfs_defer_cancel(struct xfs_trans *);
 void xfs_defer_init(struct xfs_trans *tp, struct xfs_defer_ops *dop);
 bool xfs_defer_has_unfinished_work(struct xfs_defer_ops *dop);
 int xfs_defer_ijoin(struct xfs_defer_ops *dop, struct xfs_inode *ip);
-int xfs_defer_bjoin(struct xfs_defer_ops *dop, struct xfs_buf *bp);
 void xfs_defer_move(struct xfs_trans *dtp, struct xfs_trans *stp);
 
 /* Description of a deferred type. */

fs/xfs/xfs_dquot.c +0 -1

index da5c55cec966..e1196854dbcd 100644
@@ -362,7 +362,6 @@ xfs_dquot_disk_alloc(
 	 * manually or by committing the transaction.
 	 */
 	xfs_trans_bhold(tp, bp);
-	error = xfs_defer_bjoin(tp->t_dfops, bp);
 	if (error) {
 		xfs_trans_bhold_release(tp, bp);
 		xfs_trans_brelse(tp, bp);

fs/xfs/xfs_trans.h +0 -1

index 7e493221160e..581456c79197 100644
@@ -100,7 +100,6 @@ struct xfs_defer_ops {
 
 	/* relog these with each roll */
 	struct xfs_inode	*dop_inodes[XFS_DEFER_OPS_NR_INODES];
-	struct xfs_buf		*dop_bufs[XFS_DEFER_OPS_NR_BUFS];
 };
 
 /*

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Buffers that are held across deferred operations are explicitly joined to the dfops structure to ensure appropriate relogging. While buffers are currently joined explicitly, we can detect the conditions that require relogging at dfops finish time by inspecting the transaction item list for held buffers.

Replace the xfs_defer_bjoin() infrastructure with such detection and automatic relogging of held buffers. This eliminates the need for the per-dfops buffer list, replaced by an on-stack variant in xfs_defer_trans_roll().

Inodes that are held across deferred operations are explicitly joined to the dfops structure to ensure appropriate relogging. While inodes are currently joined explicitly, we can detect the conditions that require relogging at dfops finish time by inspecting the transaction item list for inodes with ili_lock_flags == 0.

Replace the xfs_defer_ijoin() infrastructure with such detection and automatic relogging of held inodes. This eliminates the need for the per-dfops inode list, replaced by an on-stack variant in xfs_defer_trans_roll().

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_attr.c        |  9 ------
 fs/xfs/libxfs/xfs_attr_remote.c |  2 --
 fs/xfs/libxfs/xfs_bmap.c        |  8 -----
 fs/xfs/libxfs/xfs_defer.c       | 56 +++++++++++----------------------
 fs/xfs/libxfs/xfs_defer.h       |  1 -
 fs/xfs/xfs_bmap_util.c          |  4 ---
 fs/xfs/xfs_inode.c              |  2 --
 fs/xfs/xfs_iomap.c              |  3 --
 fs/xfs/xfs_reflink.c            |  4 ---
 fs/xfs/xfs_symlink.c            |  1 -
 fs/xfs/xfs_trans.h              |  3 --
 11 files changed, 18 insertions(+), 75 deletions(-)

fs/xfs/libxfs/xfs_attr.c +0 -9

index 227887bee00d..3190dfc21b60 100644
@@ -320,7 +320,6 @@ xfs_attr_set(
 		 * buffer and run into problems with the write verifier.
 		 */
 		xfs_trans_bhold(args.trans, leaf_bp);
-		xfs_defer_ijoin(args.trans->t_dfops, dp);
 		error = xfs_defer_finish(&args.trans);
 		if (error)
 			goto out;

@@ -589,7 +588,6 @@ xfs_attr_leaf_addname(
 		error = xfs_attr3_leaf_to_node(args);
 		if (error)
 			goto out_defer_cancel;
-		xfs_defer_ijoin(args->trans->t_dfops, dp);
 		error = xfs_defer_finish(&args->trans);
 		if (error)
 			goto out_defer_cancel;

@@ -678,7 +676,6 @@ xfs_attr_leaf_addname(
 			/* bp is gone due to xfs_da_shrink_inode */
 			if (error)
 				goto out_defer_cancel;
-			xfs_defer_ijoin(args->trans->t_dfops, dp);
 			error = xfs_defer_finish(&args->trans);
 			if (error)
 				goto out_defer_cancel;

@@ -742,7 +739,6 @@ xfs_attr_leaf_removename(
 		/* bp is gone due to xfs_da_shrink_inode */
 		if (error)
 			goto out_defer_cancel;
-		xfs_defer_ijoin(args->trans->t_dfops, dp);
 		error = xfs_defer_finish(&args->trans);
 		if (error)
 			goto out_defer_cancel;

@@ -869,7 +865,6 @@ xfs_attr_node_addname(
 			error = xfs_attr3_leaf_to_node(args);
 			if (error)
 				goto out_defer_cancel;
-			xfs_defer_ijoin(args->trans->t_dfops, dp);
 			error = xfs_defer_finish(&args->trans);
 			if (error)
 				goto out_defer_cancel;

@@ -894,7 +889,6 @@ xfs_attr_node_addname(
 		error = xfs_da3_split(state);
 		if (error)
 			goto out_defer_cancel;
-		xfs_defer_ijoin(args->trans->t_dfops, dp);
 		error = xfs_defer_finish(&args->trans);
 		if (error)
 			goto out_defer_cancel;

@@ -991,7 +985,6 @@ xfs_attr_node_addname(
 			error = xfs_da3_join(state);
 			if (error)
 				goto out_defer_cancel;
-			xfs_defer_ijoin(args->trans->t_dfops, dp);
 			error = xfs_defer_finish(&args->trans);
 			if (error)
 				goto out_defer_cancel;

@@ -1115,7 +1108,6 @@ xfs_attr_node_removename(
 		error = xfs_da3_join(state);
 		if (error)
 			goto out_defer_cancel;
-		xfs_defer_ijoin(args->trans->t_dfops, dp);
 		error = xfs_defer_finish(&args->trans);
 		if (error)
 			goto out_defer_cancel;

@@ -1147,7 +1139,6 @@ xfs_attr_node_removename(
 			/* bp is gone due to xfs_da_shrink_inode */
 			if (error)
 				goto out_defer_cancel;
-			xfs_defer_ijoin(args->trans->t_dfops, dp);
 			error = xfs_defer_finish(&args->trans);
 			if (error)
 				goto out_defer_cancel;

fs/xfs/libxfs/xfs_attr_remote.c +0 -2

index 77ca38586913..f52552313773 100644
@@ -486,7 +486,6 @@ xfs_attr_rmtval_set(
 				  &nmap);
 		if (error)
 			goto out_defer_cancel;
-		xfs_defer_ijoin(args->trans->t_dfops, dp);
 		error = xfs_defer_finish(&args->trans);
 		if (error)
 			goto out_defer_cancel;

@@ -627,7 +626,6 @@ xfs_attr_rmtval_remove(
 				    XFS_BMAPI_ATTRFORK, 1, &done);
 		if (error)
 			goto out_defer_cancel;
-		xfs_defer_ijoin(args->trans->t_dfops, args->dp);
 		error = xfs_defer_finish(&args->trans);
 		if (error)
 			goto out_defer_cancel;

fs/xfs/libxfs/xfs_bmap.c +0 -8

index 71687d805f79..5cd490dc891a 100644
@@ -1119,7 +1119,6 @@ xfs_bmap_add_attrfork(
 			xfs_log_sb(tp);
 	}
 
-	xfs_defer_ijoin(tp->t_dfops, ip);
 	error = xfs_trans_commit(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	return error;

@@ -5987,7 +5986,6 @@ __xfs_bmap_add(
 	int				whichfork,
 	struct xfs_bmbt_irec		*bmap)
 {
-	int				error;
 	struct xfs_bmap_intent		*bi;
 
 	trace_xfs_bmap_defer(mp,

@@ -6006,12 +6004,6 @@ __xfs_bmap_add(
 	bi->bi_whichfork = whichfork;
 	bi->bi_bmap = *bmap;
 
-	error = xfs_defer_ijoin(dfops, bi->bi_owner);
-	if (error) {
-		kmem_free(bi);
-		return error;
-	}
-
 	xfs_defer_add(dfops, XFS_DEFER_OPS_TYPE_BMAP, &bi->bi_list);
 	return 0;
 }

fs/xfs/libxfs/xfs_defer.c +5 -5

index d0ee55048a7a..6230e74163ac 100644
@@ -15,6 +15,8 @@
 #include "xfs_defer.h"
 #include "xfs_trans.h"
 #include "xfs_buf_item.h"
+#include "xfs_inode.h"
+#include "xfs_inode_item.h"
 #include "xfs_trace.h"
 
 /*

@@ -230,16 +232,14 @@ xfs_defer_trans_roll(
 {
 	struct xfs_defer_ops		*dop = (*tp)->t_dfops;
 	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];
-	int				bpcount = 0;
+	struct xfs_inode		*iplist[XFS_DEFER_OPS_NR_INODES];
+	int				bpcount = 0, ipcount = 0;
 	int				i;
 	int				error;
 
-	/* Log all the joined inodes. */
-	for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)
-		xfs_trans_log_inode(*tp, dop->dop_inodes[i], XFS_ILOG_CORE);
-
 	list_for_each_entry(lip, &(*tp)->t_items, li_trans) {
 		switch (lip->li_type) {
 		case XFS_LI_BUF:

fs/xfs/libxfs/xfs_defer.c +8 -33

+				xfs_trans_log_inode(*tp, ili->ili_inode,
+						    XFS_ILOG_CORE);
+				iplist[ipcount++] = ili->ili_inode;
+			}
+			break;
 		default:
 			break;
 		}

@@ -268,8 +278,8 @@ xfs_defer_trans_roll(
 	}
 
 	/* Rejoin the joined inodes. */
-	for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)
-		xfs_trans_ijoin(*tp, dop->dop_inodes[i], 0);
+	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++) {

@@ -288,30 +298,6 @@ xfs_defer_has_unfinished_work(
 	return !list_empty(&dop->dop_pending) || !list_empty(&dop->dop_intake);
 }
 
-/*
- * Add this inode to the deferred op.  Each joined inode is relogged
- * each time we roll the transaction.
- */
-int
-xfs_defer_ijoin(
-	struct xfs_defer_ops		*dop,
-	struct xfs_inode		*ip)
-{
-	int				i;
-
-	for (i = 0; i < XFS_DEFER_OPS_NR_INODES; i++) {
-		if (dop->dop_inodes[i] == ip)
-			return 0;
-		else if (dop->dop_inodes[i] == NULL) {
-			dop->dop_inodes[i] = ip;
-			return 0;
-		}
-	}
-
-	ASSERT(0);
-	return -EFSCORRUPTED;
-}
-
 /*
  * Reset an already used dfops after finish.
  */

@@ -319,11 +305,7 @@ static void
 xfs_defer_reset(
 	struct xfs_trans	*tp)
 {
-	struct xfs_defer_ops	*dop = tp->t_dfops;
-
-	ASSERT(!xfs_defer_has_unfinished_work(dop));
-
-	memset(dop->dop_inodes, 0, sizeof(dop->dop_inodes));
+	ASSERT(!xfs_defer_has_unfinished_work(tp->t_dfops));
 
 	/*
 	 * Low mode state transfers across transaction rolls to mirror dfops

@@ -585,8 +567,6 @@ xfs_defer_move(
 	list_splice_init(&src->dop_intake, &dst->dop_intake);
 	list_splice_init(&src->dop_pending, &dst->dop_pending);
 
-	memcpy(dst->dop_inodes, src->dop_inodes, sizeof(dst->dop_inodes));
-
 	/*
 	 * Low free space mode was historically controlled by a dfops field.
 	 * This meant that low mode state potentially carried across multiple

fs/xfs/libxfs/xfs_defer.h +0 -1

index 4a8bb838adf2..bf1e9f78561e 100644
@@ -42,7 +42,6 @@ int xfs_defer_finish(struct xfs_trans **tp);
 void xfs_defer_cancel(struct xfs_trans *);
 void xfs_defer_init(struct xfs_trans *tp, struct xfs_defer_ops *dop);
 bool xfs_defer_has_unfinished_work(struct xfs_defer_ops *dop);
-int xfs_defer_ijoin(struct xfs_defer_ops *dop, struct xfs_inode *ip);
 void xfs_defer_move(struct xfs_trans *dtp, struct xfs_trans *stp);
 
 /* Description of a deferred type. */

fs/xfs/xfs_bmap_util.c +0 -4

index 0c58a66b39e5..30ac1300dc49 100644
@@ -979,7 +979,6 @@ xfs_alloc_file_space(
 		/*
 		 * Complete the transaction
 		 */
-		xfs_defer_ijoin(tp->t_dfops, ip);
 		error = xfs_trans_commit(tp);
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
 		if (error)

@@ -1037,8 +1036,6 @@ xfs_unmap_extent(
 	if (error)
 		goto out_trans_cancel;
 
-	xfs_defer_ijoin(tp->t_dfops, ip);
-
 	error = xfs_trans_commit(tp);
 out_unlock:
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);

@@ -1624,7 +1621,6 @@ xfs_swap_extent_rmap(
 			if (error)
 				goto out_defer;
 
-			xfs_defer_ijoin(tp->t_dfops, ip);
 			error = xfs_defer_finish(tpp);
 			tp = *tpp;
 			if (error)

fs/xfs/xfs_inode.c +0 -2

index 441c8593cfd7..7bb46a0eecfc 100644
@@ -1569,7 +1569,6 @@ xfs_itruncate_extents_flags(
 		 * Duplicate the transaction that has the permanent
 		 * reservation and commit the old transaction.
 		 */
-		xfs_defer_ijoin(tp->t_dfops, ip);
 		error = xfs_defer_finish(&tp);
 		if (error)
 			goto out_bmap_cancel;

@@ -1810,7 +1809,6 @@ xfs_inactive_ifree(
 	 * Just ignore errors at this point.  There is nothing we can do except
 	 * to try to keep going. Make sure it's not a silent error.
 	 */
-	xfs_defer_ijoin(tp->t_dfops, ip);
 	error = xfs_trans_commit(tp);
 	if (error)
 		xfs_notice(mp, "%s: xfs_trans_commit returned error %d",

fs/xfs/xfs_iomap.c +0 -3

index 464b4b3225bb..8e8ca9f03f0e 100644
@@ -261,7 +261,6 @@ xfs_iomap_write_direct(
 	/*
 	 * Complete the transaction
 	 */
-	xfs_defer_ijoin(tp->t_dfops, ip);
 	error = xfs_trans_commit(tp);
 	if (error)
 		goto out_unlock;

@@ -763,7 +762,6 @@ xfs_iomap_write_allocate(
 			if (error)
 				goto trans_cancel;
 
-			xfs_defer_ijoin(tp->t_dfops, ip);
 			error = xfs_trans_commit(tp);
 			if (error)
 				goto error0;

@@ -881,7 +879,6 @@ xfs_iomap_write_unwritten(
 			xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 		}
 
-		xfs_defer_ijoin(tp->t_dfops, ip);
 		error = xfs_trans_commit(tp);
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
 		if (error)

fs/xfs/xfs_reflink.c +0 -4

index e986fcf928e5..dce8ba8ab681 100644
@@ -435,7 +435,6 @@ xfs_reflink_allocate_cow(
 	xfs_inode_set_cowblocks_tag(ip);
 
 	/* Finish up. */
-	xfs_defer_ijoin(tp->t_dfops, ip);
 	error = xfs_trans_commit(tp);
 	if (error)
 		return error;

@@ -518,7 +517,6 @@ xfs_reflink_cancel_cow_blocks(
 					NULL);
 
 			/* Roll the transaction */
-			xfs_defer_ijoin((*tpp)->t_dfops, ip);
 			error = xfs_defer_finish(tpp);
 			if (error) {
 				xfs_defer_cancel(*tpp);

@@ -716,7 +714,6 @@ xfs_reflink_end_cow(
 		/* Remove the mapping from the CoW fork. */
 		xfs_bmap_del_extent_cow(ip, &icur, &got, &del);
 
-		xfs_defer_ijoin(tp->t_dfops, ip);
 		error = xfs_defer_finish(&tp);
 		if (error)
 			goto out_cancel;

@@ -1077,7 +1074,6 @@ xfs_reflink_remap_extent(
 
 next_extent:
 		/* Process all the deferred stuff. */
-		xfs_defer_ijoin(tp->t_dfops, ip);
 		error = xfs_defer_finish(&tp);
 		if (error)
 			goto out_cancel;

fs/xfs/xfs_symlink.c +0 -1

index 2bfe7fbbedb2..a3e98c64b6e3 100644
@@ -454,7 +454,6 @@ xfs_inactive_symlink_rmt(
 	 * Commit the transaction. This first logs the EFI and the inode, then
 	 * rolls and commits the transaction that frees the extents.
 	 */
-	xfs_defer_ijoin(tp->t_dfops, ip);
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 	error = xfs_trans_commit(tp);
 	if (error) {

fs/xfs/xfs_trans.h +0 -3

index 581456c79197..8665d45b82c6 100644
@@ -97,9 +97,6 @@ void	xfs_log_item_init(struct xfs_mount *mp, struct xfs_log_item *item,
 struct xfs_defer_ops {
 	struct list_head	dop_intake;	/* unlogged pending work */
 	struct list_head	dop_pending;	/* logged pending work */
-
-	/* relog these with each roll */
-	struct xfs_inode	*dop_inodes[XFS_DEFER_OPS_NR_INODES];
 };
 
 /*

Signature · 6 lines

2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Inodes that are held across deferred operations are explicitly joined to the dfops structure to ensure appropriate relogging. While inodes are currently joined explicitly, we can detect the conditions that require relogging at dfops finish time by inspecting the transaction item list for inodes with ili_lock_flags == 0.

Replace the xfs_defer_ijoin() infrastructure with such detection and automatic relogging of held inodes. This eliminates the need for the per-dfops inode list, replaced by an on-stack variant in xfs_defer_trans_roll().