[PATCH 0/10] xfs: log item and transaction cleanups

[PATCH 0/10] xfs: log item and transaction cleanups

[Dave Chinner]

Hi folks,

This started out as one patch to get rid of log item descriptors,
then turned into an onion peeling exercise as it uncovered other
problems.

The series converts the log item flags to atomic ops so
they can be updated in different locking contexts safely -
XFS_LI_FAILED introduced a requirement for changing and checking
under the AIL lock, and we don't do that in many places.

This gets worse with the last patch in the series where I get rid of
log item descriptors, which moves the XFS_LI_LOGGED flag out of the
descriptor and into the log item itself, meaning the li_flags field
either needs ti's own lock or is updated atomically. I chose the
latter because we've got enough locks already and the available
atomic operations cover all the atomic logic checks we need.

In doing this, I kept hitting assert failures with the AIL lock
held, leading to unrecoverable hangs and a patch to avoid that.
I found inodes multiply joined to single transactions, so there's
patches to fix that. I needed high level transaction tracing to work
out where stuff was going wrong, so there's a patch for that. I
added some more assert checks to log items to catch wacky cases that
occurred but weren't caught while debugging the code.

So, really, the first and last patches in the series were the only
ones I intended to write - the rest were a result of pre-existing
bugs I found and the code I needed to track them down....

Cheers,

Dave.

[PATCH 01/10] xfs: log item flags are racy

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The log item flags contain a field that is protected by the AIL
lock - the XFS_LI_IN_AIL flag. We use non-atomic RMW operations to
set and clear these flags, but most of the updates and checks are
not done with the AIL lock held and so are susceptible to update
races.

Fix this by changing the log item flags to use atomic bitops rather
than be reliant on the AIL lock for update serialisation.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_bmap_item.c     |  4 ++--
 fs/xfs/xfs_buf_item.c      |  4 +++-
 fs/xfs/xfs_dquot.c         |  7 +++----
 fs/xfs/xfs_dquot_item.c    |  2 +-
 fs/xfs/xfs_extfree_item.c  |  4 ++--
 fs/xfs/xfs_icache.c        |  3 ++-
 fs/xfs/xfs_icreate_item.c  |  2 +-
 fs/xfs/xfs_inode.c         |  4 ++--
 fs/xfs/xfs_inode_item.c    |  8 ++++----
 fs/xfs/xfs_log.c           |  2 +-
 fs/xfs/xfs_qm.c            |  2 +-
 fs/xfs/xfs_refcount_item.c |  4 ++--
 fs/xfs/xfs_rmap_item.c     |  4 ++--
 fs/xfs/xfs_trace.h         |  6 +++---
 fs/xfs/xfs_trans.c         |  4 ++--
 fs/xfs/xfs_trans.h         | 19 ++++++++++++-------
 fs/xfs/xfs_trans_ail.c     |  9 ++++-----
 fs/xfs/xfs_trans_buf.c     |  2 +-
 fs/xfs/xfs_trans_priv.h    | 10 ++++------
 19 files changed, 52 insertions(+), 48 deletions(-)

diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 2203465e63ea..618bb71535c8 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -160,7 +160,7 @@ STATIC void
 xfs_bui_item_unlock(
 	struct xfs_log_item	*lip)
 {
-	if (lip->li_flags & XFS_LI_ABORTED)
+	if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
 		xfs_bui_release(BUI_ITEM(lip));
 }
 
@@ -305,7 +305,7 @@ xfs_bud_item_unlock(
 {
 	struct xfs_bud_log_item	*budp = BUD_ITEM(lip);
 
-	if (lip->li_flags & XFS_LI_ABORTED) {
+	if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
 		xfs_bui_release(budp->bud_buip);
 		kmem_zone_free(xfs_bud_zone, budp);
 	}
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 76b21f780af8..53c07a7a14fb 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -545,13 +545,15 @@ xfs_buf_item_unlock(
 {
 	struct xfs_buf_log_item	*bip = BUF_ITEM(lip);
 	struct xfs_buf		*bp = bip->bli_buf;
-	bool			aborted = !!(lip->li_flags & XFS_LI_ABORTED);
+	bool			aborted;
 	bool			hold = !!(bip->bli_flags & XFS_BLI_HOLD);
 	bool			dirty = !!(bip->bli_flags & XFS_BLI_DIRTY);
 #if defined(DEBUG) || defined(XFS_WARN)
 	bool			ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
 #endif
 
+	aborted = test_bit(XFS_LI_ABORTED, &lip->li_flags);
+
 	/* Clear the buffer's association with this transaction. */
 	bp->b_transp = NULL;
 
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index a7daef9e16bf..d0b65679baae 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -913,9 +913,9 @@ xfs_qm_dqflush_done(
 	 * since it's cheaper, and then we recheck while
 	 * holding the lock before removing the dquot from the AIL.
 	 */
-	if ((lip->li_flags & XFS_LI_IN_AIL) &&
+	if (test_bit(XFS_LI_IN_AIL, &lip->li_flags) &&
 	    ((lip->li_lsn == qip->qli_flush_lsn) ||
-	     (lip->li_flags & XFS_LI_FAILED))) {
+	     test_bit(XFS_LI_FAILED, &lip->li_flags))) {
 
 		/* xfs_trans_ail_delete() drops the AIL lock. */
 		spin_lock(&ailp->ail_lock);
@@ -926,8 +926,7 @@ xfs_qm_dqflush_done(
 			 * Clear the failed state since we are about to drop the
 			 * flush lock
 			 */
-			if (lip->li_flags & XFS_LI_FAILED)
-				xfs_clear_li_failed(lip);
+			xfs_clear_li_failed(lip);
 			spin_unlock(&ailp->ail_lock);
 		}
 	}
diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
index 4b331e354da7..57df98122156 100644
--- a/fs/xfs/xfs_dquot_item.c
+++ b/fs/xfs/xfs_dquot_item.c
@@ -173,7 +173,7 @@ xfs_qm_dquot_logitem_push(
 	 * The buffer containing this item failed to be written back
 	 * previously. Resubmit the buffer for IO
 	 */
-	if (lip->li_flags & XFS_LI_FAILED) {
+	if (test_bit(XFS_LI_FAILED, &lip->li_flags)) {
 		if (!xfs_buf_trylock(bp))
 			return XFS_ITEM_LOCKED;
 
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index b5b1e567b9f4..70b7d48af6d6 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -168,7 +168,7 @@ STATIC void
 xfs_efi_item_unlock(
 	struct xfs_log_item	*lip)
 {
-	if (lip->li_flags & XFS_LI_ABORTED)
+	if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
 		xfs_efi_release(EFI_ITEM(lip));
 }
 
@@ -402,7 +402,7 @@ xfs_efd_item_unlock(
 {
 	struct xfs_efd_log_item	*efdp = EFD_ITEM(lip);
 
-	if (lip->li_flags & XFS_LI_ABORTED) {
+	if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
 		xfs_efi_release(efdp->efd_efip);
 		xfs_efd_item_free(efdp);
 	}
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 8cb2ad1cc2ec..fb37ada55710 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -107,7 +107,8 @@ xfs_inode_free_callback(
 		xfs_idestroy_fork(ip, XFS_COW_FORK);
 
 	if (ip->i_itemp) {
-		ASSERT(!(ip->i_itemp->ili_item.li_flags & XFS_LI_IN_AIL));
+		ASSERT(!test_bit(XFS_LI_IN_AIL,
+				 &ip->i_itemp->ili_item.li_flags));
 		xfs_inode_item_destroy(ip);
 		ip->i_itemp = NULL;
 	}
diff --git a/fs/xfs/xfs_icreate_item.c b/fs/xfs/xfs_icreate_item.c
index 865ad1373e5e..a99a0f8aa528 100644
--- a/fs/xfs/xfs_icreate_item.c
+++ b/fs/xfs/xfs_icreate_item.c
@@ -91,7 +91,7 @@ xfs_icreate_item_unlock(
 {
 	struct xfs_icreate_item	*icp = ICR_ITEM(lip);
 
-	if (icp->ic_item.li_flags & XFS_LI_ABORTED)
+	if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
 		kmem_zone_free(xfs_icreate_zone, icp);
 	return;
 }
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index bdd92c18a4f3..2cb294e608a0 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -498,7 +498,7 @@ xfs_lock_inodes(
 		if (!try_lock) {
 			for (j = (i - 1); j >= 0 && !try_lock; j--) {
 				lp = (xfs_log_item_t *)ips[j]->i_itemp;
-				if (lp && (lp->li_flags & XFS_LI_IN_AIL))
+				if (lp && test_bit(XFS_LI_IN_AIL, &lp->li_flags))
 					try_lock++;
 			}
 		}
@@ -598,7 +598,7 @@ xfs_lock_two_inodes(
 	 * and try again.
 	 */
 	lp = (xfs_log_item_t *)ip0->i_itemp;
-	if (lp && (lp->li_flags & XFS_LI_IN_AIL)) {
+	if (lp && test_bit(XFS_LI_IN_AIL, &lp->li_flags)) {
 		if (!xfs_ilock_nowait(ip1, xfs_lock_inumorder(ip1_mode, 1))) {
 			xfs_iunlock(ip0, ip0_mode);
 			if ((++attempts % 5) == 0)
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 34b91b789702..3e5b8574818e 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -518,7 +518,7 @@ xfs_inode_item_push(
 	 * The buffer containing this item failed to be written back
 	 * previously. Resubmit the buffer for IO.
 	 */
-	if (lip->li_flags & XFS_LI_FAILED) {
+	if (test_bit(XFS_LI_FAILED, &lip->li_flags)) {
 		if (!xfs_buf_trylock(bp))
 			return XFS_ITEM_LOCKED;
 
@@ -729,14 +729,14 @@ xfs_iflush_done(
 		 */
 		iip = INODE_ITEM(blip);
 		if ((iip->ili_logged && blip->li_lsn == iip->ili_flush_lsn) ||
-		    (blip->li_flags & XFS_LI_FAILED))
+		    test_bit(XFS_LI_FAILED, &blip->li_flags))
 			need_ail++;
 	}
 
 	/* make sure we capture the state of the initial inode. */
 	iip = INODE_ITEM(lip);
 	if ((iip->ili_logged && lip->li_lsn == iip->ili_flush_lsn) ||
-	    lip->li_flags & XFS_LI_FAILED)
+	    test_bit(XFS_LI_FAILED, &lip->li_flags))
 		need_ail++;
 
 	/*
@@ -803,7 +803,7 @@ xfs_iflush_abort(
 	xfs_inode_log_item_t	*iip = ip->i_itemp;
 
 	if (iip) {
-		if (iip->ili_item.li_flags & XFS_LI_IN_AIL) {
+		if (test_bit(XFS_LI_IN_AIL, &iip->ili_item.li_flags)) {
 			xfs_trans_ail_remove(&iip->ili_item,
 					     stale ? SHUTDOWN_LOG_IO_ERROR :
 						     SHUTDOWN_CORRUPT_INCORE);
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 2fcd9ed5d075..e427864434c1 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2132,7 +2132,7 @@ xlog_print_trans(
 
 		xfs_warn(mp, "log item: ");
 		xfs_warn(mp, "  type	= 0x%x", lip->li_type);
-		xfs_warn(mp, "  flags	= 0x%x", lip->li_flags);
+		xfs_warn(mp, "  flags	= 0x%lx", lip->li_flags);
 		if (!lv)
 			continue;
 		xfs_warn(mp, "  niovecs	= %d", lv->lv_niovecs);
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index aff4924f637f..f93bf427d2da 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -173,7 +173,7 @@ xfs_qm_dqpurge(
 
 	ASSERT(atomic_read(&dqp->q_pincount) == 0);
 	ASSERT(XFS_FORCED_SHUTDOWN(mp) ||
-	       !(dqp->q_logitem.qli_item.li_flags & XFS_LI_IN_AIL));
+		!test_bit(XFS_LI_IN_AIL, &dqp->q_logitem.qli_item.li_flags));
 
 	xfs_dqfunlock(dqp);
 	xfs_dqunlock(dqp);
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 15c9393dd7a7..e5866b714d5f 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -159,7 +159,7 @@ STATIC void
 xfs_cui_item_unlock(
 	struct xfs_log_item	*lip)
 {
-	if (lip->li_flags & XFS_LI_ABORTED)
+	if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
 		xfs_cui_release(CUI_ITEM(lip));
 }
 
@@ -310,7 +310,7 @@ xfs_cud_item_unlock(
 {
 	struct xfs_cud_log_item	*cudp = CUD_ITEM(lip);
 
-	if (lip->li_flags & XFS_LI_ABORTED) {
+	if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
 		xfs_cui_release(cudp->cud_cuip);
 		kmem_zone_free(xfs_cud_zone, cudp);
 	}
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 06a07846c9b3..e5b5b3e7ef82 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -158,7 +158,7 @@ STATIC void
 xfs_rui_item_unlock(
 	struct xfs_log_item	*lip)
 {
-	if (lip->li_flags & XFS_LI_ABORTED)
+	if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
 		xfs_rui_release(RUI_ITEM(lip));
 }
 
@@ -331,7 +331,7 @@ xfs_rud_item_unlock(
 {
 	struct xfs_rud_log_item	*rudp = RUD_ITEM(lip);
 
-	if (lip->li_flags & XFS_LI_ABORTED) {
+	if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
 		xfs_rui_release(rudp->rud_ruip);
 		kmem_zone_free(xfs_rud_zone, rudp);
 	}
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 8955254b900e..7f4d961fae12 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -442,7 +442,7 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
 		__field(int, bli_refcount)
 		__field(unsigned, bli_flags)
 		__field(void *, li_desc)
-		__field(unsigned, li_flags)
+		__field(unsigned long, li_flags)
 	),
 	TP_fast_assign(
 		__entry->dev = bip->bli_buf->b_target->bt_dev;
@@ -1018,7 +1018,7 @@ DECLARE_EVENT_CLASS(xfs_log_item_class,
 		__field(dev_t, dev)
 		__field(void *, lip)
 		__field(uint, type)
-		__field(uint, flags)
+		__field(unsigned long, flags)
 		__field(xfs_lsn_t, lsn)
 	),
 	TP_fast_assign(
@@ -1070,7 +1070,7 @@ DECLARE_EVENT_CLASS(xfs_ail_class,
 		__field(dev_t, dev)
 		__field(void *, lip)
 		__field(uint, type)
-		__field(uint, flags)
+		__field(unsigned long, flags)
 		__field(xfs_lsn_t, old_lsn)
 		__field(xfs_lsn_t, new_lsn)
 	),
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index d6d8f9d129a7..c6a2a3cb9df5 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -790,7 +790,7 @@ xfs_trans_free_items(
 		if (commit_lsn != NULLCOMMITLSN)
 			lip->li_ops->iop_committing(lip, commit_lsn);
 		if (abort)
-			lip->li_flags |= XFS_LI_ABORTED;
+			set_bit(XFS_LI_ABORTED, &lip->li_flags);
 		lip->li_ops->iop_unlock(lip);
 
 		xfs_trans_free_item_desc(lidp);
@@ -861,7 +861,7 @@ xfs_trans_committed_bulk(
 		xfs_lsn_t		item_lsn;
 
 		if (aborted)
-			lip->li_flags |= XFS_LI_ABORTED;
+			set_bit(XFS_LI_ABORTED, &lip->li_flags);
 		item_lsn = lip->li_ops->iop_committed(lip, commit_lsn);
 
 		/* item_lsn of -1 means the item needs no further processing */
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 9d542dfe0052..51145ddf7e5b 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -48,7 +48,7 @@ typedef struct xfs_log_item {
 	struct xfs_mount		*li_mountp;	/* ptr to fs mount */
 	struct xfs_ail			*li_ailp;	/* ptr to AIL */
 	uint				li_type;	/* item type */
-	uint				li_flags;	/* misc flags */
+	unsigned long			li_flags;	/* misc flags */
 	struct xfs_buf			*li_buf;	/* real buffer pointer */
 	struct list_head		li_bio_list;	/* buffer item list */
 	void				(*li_cb)(struct xfs_buf *,
@@ -64,14 +64,19 @@ typedef struct xfs_log_item {
 	xfs_lsn_t			li_seq;		/* CIL commit seq */
 } xfs_log_item_t;
 
-#define	XFS_LI_IN_AIL	0x1
-#define	XFS_LI_ABORTED	0x2
-#define	XFS_LI_FAILED	0x4
+/*
+ * li_flags use the (set/test/clear)_bit atomic interfaces because updates can
+ * race with each other and we don't want to have to use the AIL lock to
+ * serialise all updates.
+ */
+#define	XFS_LI_IN_AIL	0
+#define	XFS_LI_ABORTED	1
+#define	XFS_LI_FAILED	2
 
 #define XFS_LI_FLAGS \
-	{ XFS_LI_IN_AIL,	"IN_AIL" }, \
-	{ XFS_LI_ABORTED,	"ABORTED" }, \
-	{ XFS_LI_FAILED,	"FAILED" }
+	{ (1 << XFS_LI_IN_AIL),		"IN_AIL" }, \
+	{ (1 << XFS_LI_ABORTED),	"ABORTED" }, \
+	{ (1 << XFS_LI_FAILED),		"FAILED" }
 
 struct xfs_item_ops {
 	void (*iop_size)(xfs_log_item_t *, int *, int *);
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index d4a2445215e6..50611d2bcbc2 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -46,7 +46,7 @@ xfs_ail_check(
 	/*
 	 * Check the next and previous entries are valid.
 	 */
-	ASSERT((lip->li_flags & XFS_LI_IN_AIL) != 0);
+	ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags));
 	prev_lip = list_entry(lip->li_ail.prev, xfs_log_item_t, li_ail);
 	if (&prev_lip->li_ail != &ailp->ail_head)
 		ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0);
@@ -684,7 +684,7 @@ xfs_trans_ail_update_bulk(
 
 	for (i = 0; i < nr_items; i++) {
 		struct xfs_log_item *lip = log_items[i];
-		if (lip->li_flags & XFS_LI_IN_AIL) {
+		if (test_and_set_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
 			/* check if we really need to move the item */
 			if (XFS_LSN_CMP(lsn, lip->li_lsn) <= 0)
 				continue;
@@ -694,7 +694,6 @@ xfs_trans_ail_update_bulk(
 			if (mlip == lip)
 				mlip_changed = 1;
 		} else {
-			lip->li_flags |= XFS_LI_IN_AIL;
 			trace_xfs_ail_insert(lip, 0, lsn);
 		}
 		lip->li_lsn = lsn;
@@ -725,7 +724,7 @@ xfs_ail_delete_one(
 	trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn);
 	xfs_ail_delete(ailp, lip);
 	xfs_clear_li_failed(lip);
-	lip->li_flags &= ~XFS_LI_IN_AIL;
+	clear_bit(XFS_LI_IN_AIL, &lip->li_flags);
 	lip->li_lsn = 0;
 
 	return mlip == lip;
@@ -761,7 +760,7 @@ xfs_trans_ail_delete(
 	struct xfs_mount	*mp = ailp->ail_mount;
 	bool			mlip_changed;
 
-	if (!(lip->li_flags & XFS_LI_IN_AIL)) {
+	if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
 		spin_unlock(&ailp->ail_lock);
 		if (!XFS_FORCED_SHUTDOWN(mp)) {
 			xfs_alert_tag(mp, XFS_PTAG_AILDELETE,
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index b26c5973da90..88557dfea21f 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -442,7 +442,7 @@ xfs_trans_brelse(
 		ASSERT(bp->b_pincount == 0);
 ***/
 		ASSERT(atomic_read(&bip->bli_refcount) == 0);
-		ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
+		ASSERT(!test_bit(XFS_LI_IN_AIL, &bip->bli_item.li_flags));
 		ASSERT(!(bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF));
 		xfs_buf_item_relse(bp);
 	}
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index be24b0c8a332..43f773297b9d 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -119,7 +119,7 @@ xfs_trans_ail_remove(
 
 	spin_lock(&ailp->ail_lock);
 	/* xfs_trans_ail_delete() drops the AIL lock */
-	if (lip->li_flags & XFS_LI_IN_AIL)
+	if (test_bit(XFS_LI_IN_AIL, &lip->li_flags))
 		xfs_trans_ail_delete(ailp, lip, shutdown_type);
 	else
 		spin_unlock(&ailp->ail_lock);
@@ -171,11 +171,10 @@ xfs_clear_li_failed(
 {
 	struct xfs_buf	*bp = lip->li_buf;
 
-	ASSERT(lip->li_flags & XFS_LI_IN_AIL);
+	ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags));
 	lockdep_assert_held(&lip->li_ailp->ail_lock);
 
-	if (lip->li_flags & XFS_LI_FAILED) {
-		lip->li_flags &= ~XFS_LI_FAILED;
+	if (test_and_clear_bit(XFS_LI_FAILED, &lip->li_flags)) {
 		lip->li_buf = NULL;
 		xfs_buf_rele(bp);
 	}
@@ -188,9 +187,8 @@ xfs_set_li_failed(
 {
 	lockdep_assert_held(&lip->li_ailp->ail_lock);
 
-	if (!(lip->li_flags & XFS_LI_FAILED)) {
+	if (!test_and_set_bit(XFS_LI_FAILED, &lip->li_flags)) {
 		xfs_buf_hold(bp);
-		lip->li_flags |= XFS_LI_FAILED;
 		lip->li_buf = bp;
 	}
 }
-- 
2.17.0

Re: [PATCH 01/10] xfs: log item flags are racy

[Brian Foster]

On Wed, May 02, 2018 at 06:01:48PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The log item flags contain a field that is protected by the AIL
> lock - the XFS_LI_IN_AIL flag. We use non-atomic RMW operations to
> set and clear these flags, but most of the updates and checks are
> not done with the AIL lock held and so are susceptible to update
> races.
> 
> Fix this by changing the log item flags to use atomic bitops rather
> than be reliant on the AIL lock for update serialisation.
> 
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_bmap_item.c     |  4 ++--
>  fs/xfs/xfs_buf_item.c      |  4 +++-
>  fs/xfs/xfs_dquot.c         |  7 +++----
>  fs/xfs/xfs_dquot_item.c    |  2 +-
>  fs/xfs/xfs_extfree_item.c  |  4 ++--
>  fs/xfs/xfs_icache.c        |  3 ++-
>  fs/xfs/xfs_icreate_item.c  |  2 +-
>  fs/xfs/xfs_inode.c         |  4 ++--
>  fs/xfs/xfs_inode_item.c    |  8 ++++----
>  fs/xfs/xfs_log.c           |  2 +-
>  fs/xfs/xfs_qm.c            |  2 +-
>  fs/xfs/xfs_refcount_item.c |  4 ++--
>  fs/xfs/xfs_rmap_item.c     |  4 ++--
>  fs/xfs/xfs_trace.h         |  6 +++---
>  fs/xfs/xfs_trans.c         |  4 ++--
>  fs/xfs/xfs_trans.h         | 19 ++++++++++++-------
>  fs/xfs/xfs_trans_ail.c     |  9 ++++-----
>  fs/xfs/xfs_trans_buf.c     |  2 +-
>  fs/xfs/xfs_trans_priv.h    | 10 ++++------
>  19 files changed, 52 insertions(+), 48 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index 2203465e63ea..618bb71535c8 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -160,7 +160,7 @@ STATIC void
>  xfs_bui_item_unlock(
>  	struct xfs_log_item	*lip)
>  {
> -	if (lip->li_flags & XFS_LI_ABORTED)
> +	if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
>  		xfs_bui_release(BUI_ITEM(lip));
>  }
>  
> @@ -305,7 +305,7 @@ xfs_bud_item_unlock(
>  {
>  	struct xfs_bud_log_item	*budp = BUD_ITEM(lip);
>  
> -	if (lip->li_flags & XFS_LI_ABORTED) {
> +	if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
>  		xfs_bui_release(budp->bud_buip);
>  		kmem_zone_free(xfs_bud_zone, budp);
>  	}
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 76b21f780af8..53c07a7a14fb 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -545,13 +545,15 @@ xfs_buf_item_unlock(
>  {
>  	struct xfs_buf_log_item	*bip = BUF_ITEM(lip);
>  	struct xfs_buf		*bp = bip->bli_buf;
> -	bool			aborted = !!(lip->li_flags & XFS_LI_ABORTED);
> +	bool			aborted;
>  	bool			hold = !!(bip->bli_flags & XFS_BLI_HOLD);
>  	bool			dirty = !!(bip->bli_flags & XFS_BLI_DIRTY);
>  #if defined(DEBUG) || defined(XFS_WARN)
>  	bool			ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
>  #endif
>  
> +	aborted = test_bit(XFS_LI_ABORTED, &lip->li_flags);
> +
>  	/* Clear the buffer's association with this transaction. */
>  	bp->b_transp = NULL;
>  
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index a7daef9e16bf..d0b65679baae 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -913,9 +913,9 @@ xfs_qm_dqflush_done(
>  	 * since it's cheaper, and then we recheck while
>  	 * holding the lock before removing the dquot from the AIL.
>  	 */
> -	if ((lip->li_flags & XFS_LI_IN_AIL) &&
> +	if (test_bit(XFS_LI_IN_AIL, &lip->li_flags) &&
>  	    ((lip->li_lsn == qip->qli_flush_lsn) ||
> -	     (lip->li_flags & XFS_LI_FAILED))) {
> +	     test_bit(XFS_LI_FAILED, &lip->li_flags))) {
>  
>  		/* xfs_trans_ail_delete() drops the AIL lock. */
>  		spin_lock(&ailp->ail_lock);
> @@ -926,8 +926,7 @@ xfs_qm_dqflush_done(
>  			 * Clear the failed state since we are about to drop the
>  			 * flush lock
>  			 */
> -			if (lip->li_flags & XFS_LI_FAILED)
> -				xfs_clear_li_failed(lip);
> +			xfs_clear_li_failed(lip);
>  			spin_unlock(&ailp->ail_lock);
>  		}
>  	}
> diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
> index 4b331e354da7..57df98122156 100644
> --- a/fs/xfs/xfs_dquot_item.c
> +++ b/fs/xfs/xfs_dquot_item.c
> @@ -173,7 +173,7 @@ xfs_qm_dquot_logitem_push(
>  	 * The buffer containing this item failed to be written back
>  	 * previously. Resubmit the buffer for IO
>  	 */
> -	if (lip->li_flags & XFS_LI_FAILED) {
> +	if (test_bit(XFS_LI_FAILED, &lip->li_flags)) {
>  		if (!xfs_buf_trylock(bp))
>  			return XFS_ITEM_LOCKED;
>  
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index b5b1e567b9f4..70b7d48af6d6 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -168,7 +168,7 @@ STATIC void
>  xfs_efi_item_unlock(
>  	struct xfs_log_item	*lip)
>  {
> -	if (lip->li_flags & XFS_LI_ABORTED)
> +	if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
>  		xfs_efi_release(EFI_ITEM(lip));
>  }
>  
> @@ -402,7 +402,7 @@ xfs_efd_item_unlock(
>  {
>  	struct xfs_efd_log_item	*efdp = EFD_ITEM(lip);
>  
> -	if (lip->li_flags & XFS_LI_ABORTED) {
> +	if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
>  		xfs_efi_release(efdp->efd_efip);
>  		xfs_efd_item_free(efdp);
>  	}
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 8cb2ad1cc2ec..fb37ada55710 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -107,7 +107,8 @@ xfs_inode_free_callback(
>  		xfs_idestroy_fork(ip, XFS_COW_FORK);
>  
>  	if (ip->i_itemp) {
> -		ASSERT(!(ip->i_itemp->ili_item.li_flags & XFS_LI_IN_AIL));
> +		ASSERT(!test_bit(XFS_LI_IN_AIL,
> +				 &ip->i_itemp->ili_item.li_flags));
>  		xfs_inode_item_destroy(ip);
>  		ip->i_itemp = NULL;
>  	}
> diff --git a/fs/xfs/xfs_icreate_item.c b/fs/xfs/xfs_icreate_item.c
> index 865ad1373e5e..a99a0f8aa528 100644
> --- a/fs/xfs/xfs_icreate_item.c
> +++ b/fs/xfs/xfs_icreate_item.c
> @@ -91,7 +91,7 @@ xfs_icreate_item_unlock(
>  {
>  	struct xfs_icreate_item	*icp = ICR_ITEM(lip);
>  
> -	if (icp->ic_item.li_flags & XFS_LI_ABORTED)
> +	if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
>  		kmem_zone_free(xfs_icreate_zone, icp);
>  	return;
>  }
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index bdd92c18a4f3..2cb294e608a0 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -498,7 +498,7 @@ xfs_lock_inodes(
>  		if (!try_lock) {
>  			for (j = (i - 1); j >= 0 && !try_lock; j--) {
>  				lp = (xfs_log_item_t *)ips[j]->i_itemp;
> -				if (lp && (lp->li_flags & XFS_LI_IN_AIL))
> +				if (lp && test_bit(XFS_LI_IN_AIL, &lp->li_flags))
>  					try_lock++;
>  			}
>  		}
> @@ -598,7 +598,7 @@ xfs_lock_two_inodes(
>  	 * and try again.
>  	 */
>  	lp = (xfs_log_item_t *)ip0->i_itemp;
> -	if (lp && (lp->li_flags & XFS_LI_IN_AIL)) {
> +	if (lp && test_bit(XFS_LI_IN_AIL, &lp->li_flags)) {
>  		if (!xfs_ilock_nowait(ip1, xfs_lock_inumorder(ip1_mode, 1))) {
>  			xfs_iunlock(ip0, ip0_mode);
>  			if ((++attempts % 5) == 0)
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 34b91b789702..3e5b8574818e 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -518,7 +518,7 @@ xfs_inode_item_push(
>  	 * The buffer containing this item failed to be written back
>  	 * previously. Resubmit the buffer for IO.
>  	 */
> -	if (lip->li_flags & XFS_LI_FAILED) {
> +	if (test_bit(XFS_LI_FAILED, &lip->li_flags)) {
>  		if (!xfs_buf_trylock(bp))
>  			return XFS_ITEM_LOCKED;
>  
> @@ -729,14 +729,14 @@ xfs_iflush_done(
>  		 */
>  		iip = INODE_ITEM(blip);
>  		if ((iip->ili_logged && blip->li_lsn == iip->ili_flush_lsn) ||
> -		    (blip->li_flags & XFS_LI_FAILED))
> +		    test_bit(XFS_LI_FAILED, &blip->li_flags))
>  			need_ail++;
>  	}
>  
>  	/* make sure we capture the state of the initial inode. */
>  	iip = INODE_ITEM(lip);
>  	if ((iip->ili_logged && lip->li_lsn == iip->ili_flush_lsn) ||
> -	    lip->li_flags & XFS_LI_FAILED)
> +	    test_bit(XFS_LI_FAILED, &lip->li_flags))
>  		need_ail++;
>  
>  	/*
> @@ -803,7 +803,7 @@ xfs_iflush_abort(
>  	xfs_inode_log_item_t	*iip = ip->i_itemp;
>  
>  	if (iip) {
> -		if (iip->ili_item.li_flags & XFS_LI_IN_AIL) {
> +		if (test_bit(XFS_LI_IN_AIL, &iip->ili_item.li_flags)) {
>  			xfs_trans_ail_remove(&iip->ili_item,
>  					     stale ? SHUTDOWN_LOG_IO_ERROR :
>  						     SHUTDOWN_CORRUPT_INCORE);
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 2fcd9ed5d075..e427864434c1 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2132,7 +2132,7 @@ xlog_print_trans(
>  
>  		xfs_warn(mp, "log item: ");
>  		xfs_warn(mp, "  type	= 0x%x", lip->li_type);
> -		xfs_warn(mp, "  flags	= 0x%x", lip->li_flags);
> +		xfs_warn(mp, "  flags	= 0x%lx", lip->li_flags);
>  		if (!lv)
>  			continue;
>  		xfs_warn(mp, "  niovecs	= %d", lv->lv_niovecs);
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index aff4924f637f..f93bf427d2da 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -173,7 +173,7 @@ xfs_qm_dqpurge(
>  
>  	ASSERT(atomic_read(&dqp->q_pincount) == 0);
>  	ASSERT(XFS_FORCED_SHUTDOWN(mp) ||
> -	       !(dqp->q_logitem.qli_item.li_flags & XFS_LI_IN_AIL));
> +		!test_bit(XFS_LI_IN_AIL, &dqp->q_logitem.qli_item.li_flags));
>  
>  	xfs_dqfunlock(dqp);
>  	xfs_dqunlock(dqp);
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index 15c9393dd7a7..e5866b714d5f 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -159,7 +159,7 @@ STATIC void
>  xfs_cui_item_unlock(
>  	struct xfs_log_item	*lip)
>  {
> -	if (lip->li_flags & XFS_LI_ABORTED)
> +	if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
>  		xfs_cui_release(CUI_ITEM(lip));
>  }
>  
> @@ -310,7 +310,7 @@ xfs_cud_item_unlock(
>  {
>  	struct xfs_cud_log_item	*cudp = CUD_ITEM(lip);
>  
> -	if (lip->li_flags & XFS_LI_ABORTED) {
> +	if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
>  		xfs_cui_release(cudp->cud_cuip);
>  		kmem_zone_free(xfs_cud_zone, cudp);
>  	}
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index 06a07846c9b3..e5b5b3e7ef82 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -158,7 +158,7 @@ STATIC void
>  xfs_rui_item_unlock(
>  	struct xfs_log_item	*lip)
>  {
> -	if (lip->li_flags & XFS_LI_ABORTED)
> +	if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
>  		xfs_rui_release(RUI_ITEM(lip));
>  }
>  
> @@ -331,7 +331,7 @@ xfs_rud_item_unlock(
>  {
>  	struct xfs_rud_log_item	*rudp = RUD_ITEM(lip);
>  
> -	if (lip->li_flags & XFS_LI_ABORTED) {
> +	if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
>  		xfs_rui_release(rudp->rud_ruip);
>  		kmem_zone_free(xfs_rud_zone, rudp);
>  	}
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 8955254b900e..7f4d961fae12 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -442,7 +442,7 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
>  		__field(int, bli_refcount)
>  		__field(unsigned, bli_flags)
>  		__field(void *, li_desc)
> -		__field(unsigned, li_flags)
> +		__field(unsigned long, li_flags)
>  	),
>  	TP_fast_assign(
>  		__entry->dev = bip->bli_buf->b_target->bt_dev;
> @@ -1018,7 +1018,7 @@ DECLARE_EVENT_CLASS(xfs_log_item_class,
>  		__field(dev_t, dev)
>  		__field(void *, lip)
>  		__field(uint, type)
> -		__field(uint, flags)
> +		__field(unsigned long, flags)
>  		__field(xfs_lsn_t, lsn)
>  	),
>  	TP_fast_assign(
> @@ -1070,7 +1070,7 @@ DECLARE_EVENT_CLASS(xfs_ail_class,
>  		__field(dev_t, dev)
>  		__field(void *, lip)
>  		__field(uint, type)
> -		__field(uint, flags)
> +		__field(unsigned long, flags)
>  		__field(xfs_lsn_t, old_lsn)
>  		__field(xfs_lsn_t, new_lsn)
>  	),
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index d6d8f9d129a7..c6a2a3cb9df5 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -790,7 +790,7 @@ xfs_trans_free_items(
>  		if (commit_lsn != NULLCOMMITLSN)
>  			lip->li_ops->iop_committing(lip, commit_lsn);
>  		if (abort)
> -			lip->li_flags |= XFS_LI_ABORTED;
> +			set_bit(XFS_LI_ABORTED, &lip->li_flags);
>  		lip->li_ops->iop_unlock(lip);
>  
>  		xfs_trans_free_item_desc(lidp);
> @@ -861,7 +861,7 @@ xfs_trans_committed_bulk(
>  		xfs_lsn_t		item_lsn;
>  
>  		if (aborted)
> -			lip->li_flags |= XFS_LI_ABORTED;
> +			set_bit(XFS_LI_ABORTED, &lip->li_flags);
>  		item_lsn = lip->li_ops->iop_committed(lip, commit_lsn);
>  
>  		/* item_lsn of -1 means the item needs no further processing */
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 9d542dfe0052..51145ddf7e5b 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -48,7 +48,7 @@ typedef struct xfs_log_item {
>  	struct xfs_mount		*li_mountp;	/* ptr to fs mount */
>  	struct xfs_ail			*li_ailp;	/* ptr to AIL */
>  	uint				li_type;	/* item type */
> -	uint				li_flags;	/* misc flags */
> +	unsigned long			li_flags;	/* misc flags */
>  	struct xfs_buf			*li_buf;	/* real buffer pointer */
>  	struct list_head		li_bio_list;	/* buffer item list */
>  	void				(*li_cb)(struct xfs_buf *,
> @@ -64,14 +64,19 @@ typedef struct xfs_log_item {
>  	xfs_lsn_t			li_seq;		/* CIL commit seq */
>  } xfs_log_item_t;
>  
> -#define	XFS_LI_IN_AIL	0x1
> -#define	XFS_LI_ABORTED	0x2
> -#define	XFS_LI_FAILED	0x4
> +/*
> + * li_flags use the (set/test/clear)_bit atomic interfaces because updates can
> + * race with each other and we don't want to have to use the AIL lock to
> + * serialise all updates.
> + */
> +#define	XFS_LI_IN_AIL	0
> +#define	XFS_LI_ABORTED	1
> +#define	XFS_LI_FAILED	2
>  
>  #define XFS_LI_FLAGS \
> -	{ XFS_LI_IN_AIL,	"IN_AIL" }, \
> -	{ XFS_LI_ABORTED,	"ABORTED" }, \
> -	{ XFS_LI_FAILED,	"FAILED" }
> +	{ (1 << XFS_LI_IN_AIL),		"IN_AIL" }, \
> +	{ (1 << XFS_LI_ABORTED),	"ABORTED" }, \
> +	{ (1 << XFS_LI_FAILED),		"FAILED" }
>  
>  struct xfs_item_ops {
>  	void (*iop_size)(xfs_log_item_t *, int *, int *);
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index d4a2445215e6..50611d2bcbc2 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -46,7 +46,7 @@ xfs_ail_check(
>  	/*
>  	 * Check the next and previous entries are valid.
>  	 */
> -	ASSERT((lip->li_flags & XFS_LI_IN_AIL) != 0);
> +	ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags));
>  	prev_lip = list_entry(lip->li_ail.prev, xfs_log_item_t, li_ail);
>  	if (&prev_lip->li_ail != &ailp->ail_head)
>  		ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0);
> @@ -684,7 +684,7 @@ xfs_trans_ail_update_bulk(
>  
>  	for (i = 0; i < nr_items; i++) {
>  		struct xfs_log_item *lip = log_items[i];
> -		if (lip->li_flags & XFS_LI_IN_AIL) {
> +		if (test_and_set_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
>  			/* check if we really need to move the item */
>  			if (XFS_LSN_CMP(lsn, lip->li_lsn) <= 0)
>  				continue;
> @@ -694,7 +694,6 @@ xfs_trans_ail_update_bulk(
>  			if (mlip == lip)
>  				mlip_changed = 1;
>  		} else {
> -			lip->li_flags |= XFS_LI_IN_AIL;
>  			trace_xfs_ail_insert(lip, 0, lsn);
>  		}
>  		lip->li_lsn = lsn;
> @@ -725,7 +724,7 @@ xfs_ail_delete_one(
>  	trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn);
>  	xfs_ail_delete(ailp, lip);
>  	xfs_clear_li_failed(lip);
> -	lip->li_flags &= ~XFS_LI_IN_AIL;
> +	clear_bit(XFS_LI_IN_AIL, &lip->li_flags);
>  	lip->li_lsn = 0;
>  
>  	return mlip == lip;
> @@ -761,7 +760,7 @@ xfs_trans_ail_delete(
>  	struct xfs_mount	*mp = ailp->ail_mount;
>  	bool			mlip_changed;
>  
> -	if (!(lip->li_flags & XFS_LI_IN_AIL)) {
> +	if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
>  		spin_unlock(&ailp->ail_lock);
>  		if (!XFS_FORCED_SHUTDOWN(mp)) {
>  			xfs_alert_tag(mp, XFS_PTAG_AILDELETE,
> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> index b26c5973da90..88557dfea21f 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -442,7 +442,7 @@ xfs_trans_brelse(
>  		ASSERT(bp->b_pincount == 0);
>  ***/
>  		ASSERT(atomic_read(&bip->bli_refcount) == 0);
> -		ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
> +		ASSERT(!test_bit(XFS_LI_IN_AIL, &bip->bli_item.li_flags));
>  		ASSERT(!(bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF));
>  		xfs_buf_item_relse(bp);
>  	}
> diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
> index be24b0c8a332..43f773297b9d 100644
> --- a/fs/xfs/xfs_trans_priv.h
> +++ b/fs/xfs/xfs_trans_priv.h
> @@ -119,7 +119,7 @@ xfs_trans_ail_remove(
>  
>  	spin_lock(&ailp->ail_lock);
>  	/* xfs_trans_ail_delete() drops the AIL lock */
> -	if (lip->li_flags & XFS_LI_IN_AIL)
> +	if (test_bit(XFS_LI_IN_AIL, &lip->li_flags))
>  		xfs_trans_ail_delete(ailp, lip, shutdown_type);
>  	else
>  		spin_unlock(&ailp->ail_lock);
> @@ -171,11 +171,10 @@ xfs_clear_li_failed(
>  {
>  	struct xfs_buf	*bp = lip->li_buf;
>  
> -	ASSERT(lip->li_flags & XFS_LI_IN_AIL);
> +	ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags));
>  	lockdep_assert_held(&lip->li_ailp->ail_lock);
>  
> -	if (lip->li_flags & XFS_LI_FAILED) {
> -		lip->li_flags &= ~XFS_LI_FAILED;
> +	if (test_and_clear_bit(XFS_LI_FAILED, &lip->li_flags)) {
>  		lip->li_buf = NULL;
>  		xfs_buf_rele(bp);
>  	}
> @@ -188,9 +187,8 @@ xfs_set_li_failed(
>  {
>  	lockdep_assert_held(&lip->li_ailp->ail_lock);
>  
> -	if (!(lip->li_flags & XFS_LI_FAILED)) {
> +	if (!test_and_set_bit(XFS_LI_FAILED, &lip->li_flags)) {
>  		xfs_buf_hold(bp);
> -		lip->li_flags |= XFS_LI_FAILED;
>  		lip->li_buf = bp;
>  	}
>  }
> -- 
> 2.17.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/10] xfs: log item flags are racy

[Christoph Hellwig]

Looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 02/10] xfs: catch log items multiply joined to a transaction

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

I've comes across a series of subtle bugs in development code that
are a result of inodes being joined to the same transaction more
than once. This means the transaction has multiple log item
descriptors pointing to the same log item, and so processes them
multiple times during transaction commit processing. Further, the
log item can only store a single log item descriptor back pointer,
so this means all but one of the log item descriptors pointing
to the log item can't be found from the log item.

Add a log item flag to say the item has been joined to a transaction
and assert that it's not set when adding a log item to a
transaction. Clear it when the log item is disconnected from the log
item descriptor. This will tell us if there are other log items that
transactions are referencing multiple times.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_trans.c | 6 ++++--
 fs/xfs/xfs_trans.h | 4 +++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index c6a2a3cb9df5..33c40788cffa 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -741,6 +741,7 @@ xfs_trans_add_item(
 
 	ASSERT(lip->li_mountp == tp->t_mountp);
 	ASSERT(lip->li_ailp == tp->t_mountp->m_ail);
+	ASSERT(!test_bit(XFS_LI_TRANS, &lip->li_flags));
 
 	lidp = kmem_zone_zalloc(xfs_log_item_desc_zone, KM_SLEEP | KM_NOFS);
 
@@ -749,6 +750,7 @@ xfs_trans_add_item(
 	list_add_tail(&lidp->lid_trans, &tp->t_items);
 
 	lip->li_desc = lidp;
+	set_bit(XFS_LI_TRANS, &lip->li_flags);
 }
 
 STATIC void
@@ -766,6 +768,7 @@ void
 xfs_trans_del_item(
 	struct xfs_log_item	*lip)
 {
+	clear_bit(XFS_LI_TRANS, &lip->li_flags);
 	xfs_trans_free_item_desc(lip->li_desc);
 	lip->li_desc = NULL;
 }
@@ -785,7 +788,7 @@ xfs_trans_free_items(
 	list_for_each_entry_safe(lidp, next, &tp->t_items, lid_trans) {
 		struct xfs_log_item	*lip = lidp->lid_item;
 
-		lip->li_desc = NULL;
+		xfs_trans_del_item(lip);
 
 		if (commit_lsn != NULLCOMMITLSN)
 			lip->li_ops->iop_committing(lip, commit_lsn);
@@ -793,7 +796,6 @@ xfs_trans_free_items(
 			set_bit(XFS_LI_ABORTED, &lip->li_flags);
 		lip->li_ops->iop_unlock(lip);
 
-		xfs_trans_free_item_desc(lidp);
 	}
 }
 
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 51145ddf7e5b..af52107adffc 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -72,11 +72,13 @@ typedef struct xfs_log_item {
 #define	XFS_LI_IN_AIL	0
 #define	XFS_LI_ABORTED	1
 #define	XFS_LI_FAILED	2
+#define	XFS_LI_TRANS	3	/* attached to an active transaction */
 
 #define XFS_LI_FLAGS \
 	{ (1 << XFS_LI_IN_AIL),		"IN_AIL" }, \
 	{ (1 << XFS_LI_ABORTED),	"ABORTED" }, \
-	{ (1 << XFS_LI_FAILED),		"FAILED" }
+	{ (1 << XFS_LI_FAILED),		"FAILED" }, \
+	{ (1 << XFS_LI_TRANS),		"TRANS" }
 
 struct xfs_item_ops {
 	void (*iop_size)(xfs_log_item_t *, int *, int *);
-- 
2.17.0

Re: [PATCH 02/10] xfs: catch log items multiply joined to a transaction

[Brian Foster]

On Wed, May 02, 2018 at 06:01:49PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> I've comes across a series of subtle bugs in development code that
> are a result of inodes being joined to the same transaction more
> than once. This means the transaction has multiple log item
> descriptors pointing to the same log item, and so processes them
> multiple times during transaction commit processing. Further, the
> log item can only store a single log item descriptor back pointer,
> so this means all but one of the log item descriptors pointing
> to the log item can't be found from the log item.
> 
> Add a log item flag to say the item has been joined to a transaction
> and assert that it's not set when adding a log item to a
> transaction. Clear it when the log item is disconnected from the log
> item descriptor. This will tell us if there are other log items that
> transactions are referencing multiple times.
> 
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_trans.c | 6 ++++--
>  fs/xfs/xfs_trans.h | 4 +++-
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index c6a2a3cb9df5..33c40788cffa 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -741,6 +741,7 @@ xfs_trans_add_item(
>  
>  	ASSERT(lip->li_mountp == tp->t_mountp);
>  	ASSERT(lip->li_ailp == tp->t_mountp->m_ail);
> +	ASSERT(!test_bit(XFS_LI_TRANS, &lip->li_flags));
>  
>  	lidp = kmem_zone_zalloc(xfs_log_item_desc_zone, KM_SLEEP | KM_NOFS);
>  
> @@ -749,6 +750,7 @@ xfs_trans_add_item(
>  	list_add_tail(&lidp->lid_trans, &tp->t_items);
>  
>  	lip->li_desc = lidp;
> +	set_bit(XFS_LI_TRANS, &lip->li_flags);
>  }
>  
>  STATIC void
> @@ -766,6 +768,7 @@ void
>  xfs_trans_del_item(
>  	struct xfs_log_item	*lip)
>  {
> +	clear_bit(XFS_LI_TRANS, &lip->li_flags);
>  	xfs_trans_free_item_desc(lip->li_desc);
>  	lip->li_desc = NULL;
>  }
> @@ -785,7 +788,7 @@ xfs_trans_free_items(
>  	list_for_each_entry_safe(lidp, next, &tp->t_items, lid_trans) {
>  		struct xfs_log_item	*lip = lidp->lid_item;
>  
> -		lip->li_desc = NULL;
> +		xfs_trans_del_item(lip);
>  
>  		if (commit_lsn != NULLCOMMITLSN)
>  			lip->li_ops->iop_committing(lip, commit_lsn);
> @@ -793,7 +796,6 @@ xfs_trans_free_items(
>  			set_bit(XFS_LI_ABORTED, &lip->li_flags);
>  		lip->li_ops->iop_unlock(lip);
>  
> -		xfs_trans_free_item_desc(lidp);
>  	}
>  }
>  
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 51145ddf7e5b..af52107adffc 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -72,11 +72,13 @@ typedef struct xfs_log_item {
>  #define	XFS_LI_IN_AIL	0
>  #define	XFS_LI_ABORTED	1
>  #define	XFS_LI_FAILED	2
> +#define	XFS_LI_TRANS	3	/* attached to an active transaction */
>  
>  #define XFS_LI_FLAGS \
>  	{ (1 << XFS_LI_IN_AIL),		"IN_AIL" }, \
>  	{ (1 << XFS_LI_ABORTED),	"ABORTED" }, \
> -	{ (1 << XFS_LI_FAILED),		"FAILED" }
> +	{ (1 << XFS_LI_FAILED),		"FAILED" }, \
> +	{ (1 << XFS_LI_TRANS),		"TRANS" }
>  
>  struct xfs_item_ops {
>  	void (*iop_size)(xfs_log_item_t *, int *, int *);
> -- 
> 2.17.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/10] xfs: catch log items multiply joined to a transaction

[Christoph Hellwig]

Looks good, but I think this should be moved later in the series
until you have actually fixed the existing issues that it did catch.

Then again once the log item descriptor is removed later in the series
the flag added here becomes rather pointless as the core linked list
debugging would also find these issues.

In case it stays in:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 02/10] xfs: catch log items multiply joined to a transaction

[Dave Chinner]

On Mon, May 07, 2018 at 07:48:25AM -0700, Christoph Hellwig wrote:
> Looks good, but I think this should be moved later in the series
> until you have actually fixed the existing issues that it did catch.
> 
> Then again once the log item descriptor is removed later in the series
> the flag added here becomes rather pointless as the core linked list
> debugging would also find these issues.

Yeah, you're right, it does become redundant. I'll drop this, and
add the extra check this patch does to the final "remove the lid"
patch.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 03/10] xfs: add tracing to high level transaction operations

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Because currently we have no idea what the transaction context we
are operating in is, and I need to know that information to track
down bugs in multiple log item joins to transactions.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log_cil.c |  1 +
 fs/xfs/xfs_trace.h   | 37 +++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_trans.c   | 19 ++++++++++++++++++-
 3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index de9da33866eb..8342f4ee83e7 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -1026,6 +1026,7 @@ xfs_log_commit_cil(
 		*commit_lsn = xc_commit_lsn;
 
 	xfs_log_done(mp, tp->t_ticket, NULL, regrant);
+	tp->t_ticket = NULL;
 	xfs_trans_unreserve_and_mod_sb(tp);
 
 	/*
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 7f4d961fae12..8136f2280173 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -3346,6 +3346,43 @@ TRACE_EVENT(xfs_trans_resv_calc,
 		  __entry->logflags)
 );
 
+DECLARE_EVENT_CLASS(xfs_trans_class,
+	TP_PROTO(struct xfs_trans *tp, unsigned long caller_ip),
+	TP_ARGS(tp, caller_ip),
+	TP_STRUCT__entry(
+		__field(dev_t, dev)
+		__field(uint32_t, tid)
+		__field(uint32_t, flags)
+		__field(unsigned long, caller_ip)
+	),
+	TP_fast_assign(
+		__entry->dev = tp->t_mountp->m_super->s_dev;
+		__entry->tid = 0;
+		if (tp->t_ticket)
+			__entry->tid = tp->t_ticket->t_tid;
+		__entry->flags = tp->t_flags;
+		__entry->caller_ip = caller_ip;
+	),
+	TP_printk("dev %d:%d trans %x flags 0x%x caller %pS",
+		  MAJOR(__entry->dev), MINOR(__entry->dev),
+		  __entry->tid,
+		  __entry->flags,
+		  (char *)__entry->caller_ip)
+)
+
+#define DEFINE_TRANS_EVENT(name) \
+DEFINE_EVENT(xfs_trans_class, name, \
+	TP_PROTO(struct xfs_trans *tp, unsigned long caller_ip), \
+	TP_ARGS(tp, caller_ip))
+DEFINE_TRANS_EVENT(xfs_trans_alloc);
+DEFINE_TRANS_EVENT(xfs_trans_cancel);
+DEFINE_TRANS_EVENT(xfs_trans_commit);
+DEFINE_TRANS_EVENT(xfs_trans_dup);
+DEFINE_TRANS_EVENT(xfs_trans_free);
+DEFINE_TRANS_EVENT(xfs_trans_roll);
+DEFINE_TRANS_EVENT(xfs_trans_add_item);
+DEFINE_TRANS_EVENT(xfs_trans_free_items);
+
 #endif /* _TRACE_XFS_H */
 
 #undef TRACE_INCLUDE_PATH
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 33c40788cffa..63c22aa02cd8 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -79,6 +79,7 @@ xfs_trans_free(
 	xfs_extent_busy_sort(&tp->t_busy);
 	xfs_extent_busy_clear(tp->t_mountp, &tp->t_busy, false);
 
+	trace_xfs_trans_free(tp, _RET_IP_);
 	atomic_dec(&tp->t_mountp->m_active_trans);
 	if (!(tp->t_flags & XFS_TRANS_NO_WRITECOUNT))
 		sb_end_intwrite(tp->t_mountp->m_super);
@@ -100,6 +101,8 @@ xfs_trans_dup(
 {
 	xfs_trans_t	*ntp;
 
+	trace_xfs_trans_dup(tp, _RET_IP_);
+
 	ntp = kmem_zone_zalloc(xfs_trans_zone, KM_SLEEP);
 
 	/*
@@ -283,6 +286,8 @@ xfs_trans_alloc(
 		return error;
 	}
 
+	trace_xfs_trans_alloc(tp, _RET_IP_);
+
 	*tpp = tp;
 	return 0;
 }
@@ -751,6 +756,7 @@ xfs_trans_add_item(
 
 	lip->li_desc = lidp;
 	set_bit(XFS_LI_TRANS, &lip->li_flags);
+	trace_xfs_trans_add_item(tp, _RET_IP_);
 }
 
 STATIC void
@@ -785,6 +791,8 @@ xfs_trans_free_items(
 {
 	struct xfs_log_item_desc *lidp, *next;
 
+	trace_xfs_trans_free_items(tp, _RET_IP_);
+
 	list_for_each_entry_safe(lidp, next, &tp->t_items, lid_trans) {
 		struct xfs_log_item	*lip = lidp->lid_item;
 
@@ -938,6 +946,8 @@ __xfs_trans_commit(
 	int			error = 0;
 	int			sync = tp->t_flags & XFS_TRANS_SYNC;
 
+	trace_xfs_trans_commit(tp, _RET_IP_);
+
 	/*
 	 * If there is nothing to be logged by the transaction,
 	 * then unlock all of the items associated with the
@@ -993,6 +1003,7 @@ __xfs_trans_commit(
 		commit_lsn = xfs_log_done(mp, tp->t_ticket, NULL, regrant);
 		if (commit_lsn == -1 && !error)
 			error = -EIO;
+		tp->t_ticket = NULL;
 	}
 	current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
 	xfs_trans_free_items(tp, NULLCOMMITLSN, !!error);
@@ -1024,6 +1035,8 @@ xfs_trans_cancel(
 	struct xfs_mount	*mp = tp->t_mountp;
 	bool			dirty = (tp->t_flags & XFS_TRANS_DIRTY);
 
+	trace_xfs_trans_cancel(tp, _RET_IP_);
+
 	/*
 	 * See if the caller is relying on us to shut down the
 	 * filesystem.  This happens in paths where we detect
@@ -1044,8 +1057,10 @@ xfs_trans_cancel(
 	xfs_trans_unreserve_and_mod_sb(tp);
 	xfs_trans_unreserve_and_mod_dquots(tp);
 
-	if (tp->t_ticket)
+	if (tp->t_ticket) {
 		xfs_log_done(mp, tp->t_ticket, NULL, false);
+		tp->t_ticket = NULL;
+	}
 
 	/* mark this thread as no longer being in a transaction */
 	current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
@@ -1069,6 +1084,8 @@ xfs_trans_roll(
 	struct xfs_trans_res	tres;
 	int			error;
 
+	trace_xfs_trans_roll(trans, _RET_IP_);
+
 	/*
 	 * Copy the critical parameters from one trans to the next.
 	 */
-- 
2.17.0

Re: [PATCH 03/10] xfs: add tracing to high level transaction operations

[Brian Foster]

On Wed, May 02, 2018 at 06:01:50PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Because currently we have no idea what the transaction context we
> are operating in is, and I need to know that information to track
> down bugs in multiple log item joins to transactions.
> 
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log_cil.c |  1 +
>  fs/xfs/xfs_trace.h   | 37 +++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_trans.c   | 19 ++++++++++++++++++-
>  3 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index de9da33866eb..8342f4ee83e7 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -1026,6 +1026,7 @@ xfs_log_commit_cil(
>  		*commit_lsn = xc_commit_lsn;
>  
>  	xfs_log_done(mp, tp->t_ticket, NULL, regrant);
> +	tp->t_ticket = NULL;
>  	xfs_trans_unreserve_and_mod_sb(tp);
>  
>  	/*
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 7f4d961fae12..8136f2280173 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -3346,6 +3346,43 @@ TRACE_EVENT(xfs_trans_resv_calc,
>  		  __entry->logflags)
>  );
>  
> +DECLARE_EVENT_CLASS(xfs_trans_class,
> +	TP_PROTO(struct xfs_trans *tp, unsigned long caller_ip),
> +	TP_ARGS(tp, caller_ip),
> +	TP_STRUCT__entry(
> +		__field(dev_t, dev)
> +		__field(uint32_t, tid)
> +		__field(uint32_t, flags)
> +		__field(unsigned long, caller_ip)
> +	),
> +	TP_fast_assign(
> +		__entry->dev = tp->t_mountp->m_super->s_dev;
> +		__entry->tid = 0;
> +		if (tp->t_ticket)
> +			__entry->tid = tp->t_ticket->t_tid;
> +		__entry->flags = tp->t_flags;
> +		__entry->caller_ip = caller_ip;
> +	),
> +	TP_printk("dev %d:%d trans %x flags 0x%x caller %pS",
> +		  MAJOR(__entry->dev), MINOR(__entry->dev),
> +		  __entry->tid,
> +		  __entry->flags,
> +		  (char *)__entry->caller_ip)
> +)
> +
> +#define DEFINE_TRANS_EVENT(name) \
> +DEFINE_EVENT(xfs_trans_class, name, \
> +	TP_PROTO(struct xfs_trans *tp, unsigned long caller_ip), \
> +	TP_ARGS(tp, caller_ip))
> +DEFINE_TRANS_EVENT(xfs_trans_alloc);
> +DEFINE_TRANS_EVENT(xfs_trans_cancel);
> +DEFINE_TRANS_EVENT(xfs_trans_commit);
> +DEFINE_TRANS_EVENT(xfs_trans_dup);
> +DEFINE_TRANS_EVENT(xfs_trans_free);
> +DEFINE_TRANS_EVENT(xfs_trans_roll);
> +DEFINE_TRANS_EVENT(xfs_trans_add_item);
> +DEFINE_TRANS_EVENT(xfs_trans_free_items);
> +
>  #endif /* _TRACE_XFS_H */
>  
>  #undef TRACE_INCLUDE_PATH
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 33c40788cffa..63c22aa02cd8 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -79,6 +79,7 @@ xfs_trans_free(
>  	xfs_extent_busy_sort(&tp->t_busy);
>  	xfs_extent_busy_clear(tp->t_mountp, &tp->t_busy, false);
>  
> +	trace_xfs_trans_free(tp, _RET_IP_);
>  	atomic_dec(&tp->t_mountp->m_active_trans);
>  	if (!(tp->t_flags & XFS_TRANS_NO_WRITECOUNT))
>  		sb_end_intwrite(tp->t_mountp->m_super);
> @@ -100,6 +101,8 @@ xfs_trans_dup(
>  {
>  	xfs_trans_t	*ntp;
>  
> +	trace_xfs_trans_dup(tp, _RET_IP_);
> +
>  	ntp = kmem_zone_zalloc(xfs_trans_zone, KM_SLEEP);
>  
>  	/*
> @@ -283,6 +286,8 @@ xfs_trans_alloc(
>  		return error;
>  	}
>  
> +	trace_xfs_trans_alloc(tp, _RET_IP_);
> +
>  	*tpp = tp;
>  	return 0;
>  }
> @@ -751,6 +756,7 @@ xfs_trans_add_item(
>  
>  	lip->li_desc = lidp;
>  	set_bit(XFS_LI_TRANS, &lip->li_flags);
> +	trace_xfs_trans_add_item(tp, _RET_IP_);
>  }
>  
>  STATIC void
> @@ -785,6 +791,8 @@ xfs_trans_free_items(
>  {
>  	struct xfs_log_item_desc *lidp, *next;
>  
> +	trace_xfs_trans_free_items(tp, _RET_IP_);
> +
>  	list_for_each_entry_safe(lidp, next, &tp->t_items, lid_trans) {
>  		struct xfs_log_item	*lip = lidp->lid_item;
>  
> @@ -938,6 +946,8 @@ __xfs_trans_commit(
>  	int			error = 0;
>  	int			sync = tp->t_flags & XFS_TRANS_SYNC;
>  
> +	trace_xfs_trans_commit(tp, _RET_IP_);
> +
>  	/*
>  	 * If there is nothing to be logged by the transaction,
>  	 * then unlock all of the items associated with the
> @@ -993,6 +1003,7 @@ __xfs_trans_commit(
>  		commit_lsn = xfs_log_done(mp, tp->t_ticket, NULL, regrant);
>  		if (commit_lsn == -1 && !error)
>  			error = -EIO;
> +		tp->t_ticket = NULL;
>  	}
>  	current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
>  	xfs_trans_free_items(tp, NULLCOMMITLSN, !!error);
> @@ -1024,6 +1035,8 @@ xfs_trans_cancel(
>  	struct xfs_mount	*mp = tp->t_mountp;
>  	bool			dirty = (tp->t_flags & XFS_TRANS_DIRTY);
>  
> +	trace_xfs_trans_cancel(tp, _RET_IP_);
> +
>  	/*
>  	 * See if the caller is relying on us to shut down the
>  	 * filesystem.  This happens in paths where we detect
> @@ -1044,8 +1057,10 @@ xfs_trans_cancel(
>  	xfs_trans_unreserve_and_mod_sb(tp);
>  	xfs_trans_unreserve_and_mod_dquots(tp);
>  
> -	if (tp->t_ticket)
> +	if (tp->t_ticket) {
>  		xfs_log_done(mp, tp->t_ticket, NULL, false);
> +		tp->t_ticket = NULL;
> +	}
>  
>  	/* mark this thread as no longer being in a transaction */
>  	current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
> @@ -1069,6 +1084,8 @@ xfs_trans_roll(
>  	struct xfs_trans_res	tres;
>  	int			error;
>  
> +	trace_xfs_trans_roll(trans, _RET_IP_);
> +
>  	/*
>  	 * Copy the critical parameters from one trans to the next.
>  	 */
> -- 
> 2.17.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/10] xfs: add tracing to high level transaction operations

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 04/10] xfs: adder caller IP to xfs_defer* tracepoints

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

So it's clear in the trace where they are being called from.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_defer.c | 12 ++++++------
 fs/xfs/xfs_trace.h        | 17 +++++++++++------
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 087fea02c389..51821af5d653 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -220,7 +220,7 @@ xfs_defer_trans_abort(
 {
 	struct xfs_defer_pending	*dfp;
 
-	trace_xfs_defer_trans_abort(tp->t_mountp, dop);
+	trace_xfs_defer_trans_abort(tp->t_mountp, dop, _RET_IP_);
 
 	/* Abort intent items that don't have a done item. */
 	list_for_each_entry(dfp, &dop->dop_pending, dfp_list) {
@@ -253,7 +253,7 @@ xfs_defer_trans_roll(
 	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++)
 		xfs_trans_dirty_buf(*tp, dop->dop_bufs[i]);
 
-	trace_xfs_defer_trans_roll((*tp)->t_mountp, dop);
+	trace_xfs_defer_trans_roll((*tp)->t_mountp, dop, _RET_IP_);
 
 	/* Roll the transaction. */
 	error = xfs_trans_roll(tp);
@@ -355,7 +355,7 @@ xfs_defer_finish(
 
 	ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);
 
-	trace_xfs_defer_finish((*tp)->t_mountp, dop);
+	trace_xfs_defer_finish((*tp)->t_mountp, dop, _RET_IP_);
 
 	/* Until we run out of pending work to finish... */
 	while (xfs_defer_has_unfinished_work(dop)) {
@@ -431,7 +431,7 @@ xfs_defer_finish(
 	if (error)
 		trace_xfs_defer_finish_error((*tp)->t_mountp, dop, error);
 	else
-		trace_xfs_defer_finish_done((*tp)->t_mountp, dop);
+		trace_xfs_defer_finish_done((*tp)->t_mountp, dop, _RET_IP_);
 	return error;
 }
 
@@ -447,7 +447,7 @@ xfs_defer_cancel(
 	struct list_head		*pwi;
 	struct list_head		*n;
 
-	trace_xfs_defer_cancel(NULL, dop);
+	trace_xfs_defer_cancel(NULL, dop, _RET_IP_);
 
 	/*
 	 * Free the pending items.  Caller should already have arranged
@@ -532,5 +532,5 @@ xfs_defer_init(
 	*fbp = NULLFSBLOCK;
 	INIT_LIST_HEAD(&dop->dop_intake);
 	INIT_LIST_HEAD(&dop->dop_pending);
-	trace_xfs_defer_init(NULL, dop);
+	trace_xfs_defer_init(NULL, dop, _RET_IP_);
 }
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 8136f2280173..947ab6e32c18 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -2243,30 +2243,35 @@ struct xfs_defer_pending;
 struct xfs_defer_ops;
 
 DECLARE_EVENT_CLASS(xfs_defer_class,
-	TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop),
-	TP_ARGS(mp, dop),
+	TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop,
+		 unsigned long caller_ip),
+	TP_ARGS(mp, dop, caller_ip),
 	TP_STRUCT__entry(
 		__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->committed = dop->dop_committed;
 		__entry->low = dop->dop_low;
+		__entry->caller_ip = caller_ip;
 	),
-	TP_printk("dev %d:%d ops %p committed %d low %d",
+	TP_printk("dev %d:%d ops %p committed %d low %d, caller %pS",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->dop,
 		  __entry->committed,
-		  __entry->low)
+		  __entry->low,
+		  (char *)__entry->caller_ip)
 )
 #define DEFINE_DEFER_EVENT(name) \
 DEFINE_EVENT(xfs_defer_class, name, \
-	TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop), \
-	TP_ARGS(mp, dop))
+	TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop, \
+		 unsigned long caller_ip), \
+	TP_ARGS(mp, dop, caller_ip))
 
 DECLARE_EVENT_CLASS(xfs_defer_error_class,
 	TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop, int error),
-- 
2.17.0

Re: [PATCH 04/10] xfs: adder caller IP to xfs_defer* tracepoints

[Brian Foster]

On Wed, May 02, 2018 at 06:01:51PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> So it's clear in the trace where they are being called from.
> 
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_defer.c | 12 ++++++------
>  fs/xfs/xfs_trace.h        | 17 +++++++++++------
>  2 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index 087fea02c389..51821af5d653 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -220,7 +220,7 @@ xfs_defer_trans_abort(
>  {
>  	struct xfs_defer_pending	*dfp;
>  
> -	trace_xfs_defer_trans_abort(tp->t_mountp, dop);
> +	trace_xfs_defer_trans_abort(tp->t_mountp, dop, _RET_IP_);
>  
>  	/* Abort intent items that don't have a done item. */
>  	list_for_each_entry(dfp, &dop->dop_pending, dfp_list) {
> @@ -253,7 +253,7 @@ xfs_defer_trans_roll(
>  	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++)
>  		xfs_trans_dirty_buf(*tp, dop->dop_bufs[i]);
>  
> -	trace_xfs_defer_trans_roll((*tp)->t_mountp, dop);
> +	trace_xfs_defer_trans_roll((*tp)->t_mountp, dop, _RET_IP_);
>  
>  	/* Roll the transaction. */
>  	error = xfs_trans_roll(tp);
> @@ -355,7 +355,7 @@ xfs_defer_finish(
>  
>  	ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);
>  
> -	trace_xfs_defer_finish((*tp)->t_mountp, dop);
> +	trace_xfs_defer_finish((*tp)->t_mountp, dop, _RET_IP_);
>  
>  	/* Until we run out of pending work to finish... */
>  	while (xfs_defer_has_unfinished_work(dop)) {
> @@ -431,7 +431,7 @@ xfs_defer_finish(
>  	if (error)
>  		trace_xfs_defer_finish_error((*tp)->t_mountp, dop, error);
>  	else
> -		trace_xfs_defer_finish_done((*tp)->t_mountp, dop);
> +		trace_xfs_defer_finish_done((*tp)->t_mountp, dop, _RET_IP_);
>  	return error;
>  }
>  
> @@ -447,7 +447,7 @@ xfs_defer_cancel(
>  	struct list_head		*pwi;
>  	struct list_head		*n;
>  
> -	trace_xfs_defer_cancel(NULL, dop);
> +	trace_xfs_defer_cancel(NULL, dop, _RET_IP_);
>  
>  	/*
>  	 * Free the pending items.  Caller should already have arranged
> @@ -532,5 +532,5 @@ xfs_defer_init(
>  	*fbp = NULLFSBLOCK;
>  	INIT_LIST_HEAD(&dop->dop_intake);
>  	INIT_LIST_HEAD(&dop->dop_pending);
> -	trace_xfs_defer_init(NULL, dop);
> +	trace_xfs_defer_init(NULL, dop, _RET_IP_);
>  }
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 8136f2280173..947ab6e32c18 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -2243,30 +2243,35 @@ struct xfs_defer_pending;
>  struct xfs_defer_ops;
>  
>  DECLARE_EVENT_CLASS(xfs_defer_class,
> -	TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop),
> -	TP_ARGS(mp, dop),
> +	TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop,
> +		 unsigned long caller_ip),
> +	TP_ARGS(mp, dop, caller_ip),
>  	TP_STRUCT__entry(
>  		__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->committed = dop->dop_committed;
>  		__entry->low = dop->dop_low;
> +		__entry->caller_ip = caller_ip;
>  	),
> -	TP_printk("dev %d:%d ops %p committed %d low %d",
> +	TP_printk("dev %d:%d ops %p committed %d low %d, caller %pS",
>  		  MAJOR(__entry->dev), MINOR(__entry->dev),
>  		  __entry->dop,
>  		  __entry->committed,
> -		  __entry->low)
> +		  __entry->low,
> +		  (char *)__entry->caller_ip)
>  )
>  #define DEFINE_DEFER_EVENT(name) \
>  DEFINE_EVENT(xfs_defer_class, name, \
> -	TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop), \
> -	TP_ARGS(mp, dop))
> +	TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop, \
> +		 unsigned long caller_ip), \
> +	TP_ARGS(mp, dop, caller_ip))
>  
>  DECLARE_EVENT_CLASS(xfs_defer_error_class,
>  	TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop, int error),
> -- 
> 2.17.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/10] xfs: adder caller IP to xfs_defer* tracepoints

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 05/10] xfs: don't assert fail with AIL lock held

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Been hitting AIL ordering assert failures recently, but been unable
to trace them down because the system immediately hangs up onteh
spinlock that was held when this assert fires:

XFS: Assertion failed: XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0, file: fs/xfs/xfs_trans_ail.c, line: 52

Move the assertions outside of the spinlock so the corpse can
be dissected.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_trans_ail.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 50611d2bcbc2..58a2cf6fd4d9 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -32,13 +32,19 @@
 #ifdef DEBUG
 /*
  * Check that the list is sorted as it should be.
+ *
+ * Called with the ail lock held, but we don't want to assert fail with it
+ * held otherwise we'll lock everything up and won't be able to debug the
+ * cause. Hence jump through hoops to drop teh lock before assert failing.
+ * Asserts may not be fatal, so pick teh lock back up and continue onwards.
  */
 STATIC void
 xfs_ail_check(
-	struct xfs_ail	*ailp,
-	xfs_log_item_t	*lip)
+	struct xfs_ail		*ailp,
+	struct xfs_log_item	*lip)
 {
-	xfs_log_item_t	*prev_lip;
+	struct xfs_log_item	*prev_lip;
+	struct xfs_log_item	*next_lip;
 
 	if (list_empty(&ailp->ail_head))
 		return;
@@ -46,15 +52,29 @@ xfs_ail_check(
 	/*
 	 * Check the next and previous entries are valid.
 	 */
-	ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags));
-	prev_lip = list_entry(lip->li_ail.prev, xfs_log_item_t, li_ail);
-	if (&prev_lip->li_ail != &ailp->ail_head)
-		ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0);
+	if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
+		spin_unlock(&ailp->ail_lock);
+		ASSERT(0);
+		spin_lock(&ailp->ail_lock);
+	}
 
-	prev_lip = list_entry(lip->li_ail.next, xfs_log_item_t, li_ail);
-	if (&prev_lip->li_ail != &ailp->ail_head)
-		ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) >= 0);
+	prev_lip = list_entry(lip->li_ail.prev, xfs_log_item_t, li_ail);
+	if (&prev_lip->li_ail != &ailp->ail_head) {
+		if (XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) > 0) {
+			spin_unlock(&ailp->ail_lock);
+			ASSERT(0);
+			spin_lock(&ailp->ail_lock);
+		}
+	}
 
+	next_lip = list_entry(lip->li_ail.next, xfs_log_item_t, li_ail);
+	if (&next_lip->li_ail != &ailp->ail_head) {
+		if (XFS_LSN_CMP(next_lip->li_lsn, lip->li_lsn) < 0) {
+			spin_unlock(&ailp->ail_lock);
+			ASSERT(0);
+			spin_lock(&ailp->ail_lock);
+		}
+	}
 
 }
 #else /* !DEBUG */
-- 
2.17.0

Re: [PATCH 05/10] xfs: don't assert fail with AIL lock held

[Brian Foster]

On Wed, May 02, 2018 at 06:01:52PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Been hitting AIL ordering assert failures recently, but been unable
> to trace them down because the system immediately hangs up onteh

							     on the

> spinlock that was held when this assert fires:
> 
> XFS: Assertion failed: XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0, file: fs/xfs/xfs_trans_ail.c, line: 52
> 
> Move the assertions outside of the spinlock so the corpse can
> be dissected.
> 
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_trans_ail.c | 40 ++++++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 50611d2bcbc2..58a2cf6fd4d9 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -32,13 +32,19 @@
>  #ifdef DEBUG
>  /*
>   * Check that the list is sorted as it should be.
> + *
> + * Called with the ail lock held, but we don't want to assert fail with it
> + * held otherwise we'll lock everything up and won't be able to debug the
> + * cause. Hence jump through hoops to drop teh lock before assert failing.

					      the

> + * Asserts may not be fatal, so pick teh lock back up and continue onwards.

					the

>   */
>  STATIC void
>  xfs_ail_check(
> -	struct xfs_ail	*ailp,
> -	xfs_log_item_t	*lip)
> +	struct xfs_ail		*ailp,
> +	struct xfs_log_item	*lip)
>  {
> -	xfs_log_item_t	*prev_lip;
> +	struct xfs_log_item	*prev_lip;
> +	struct xfs_log_item	*next_lip;
>  
>  	if (list_empty(&ailp->ail_head))
>  		return;
> @@ -46,15 +52,29 @@ xfs_ail_check(
>  	/*
>  	 * Check the next and previous entries are valid.
>  	 */
> -	ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags));
> -	prev_lip = list_entry(lip->li_ail.prev, xfs_log_item_t, li_ail);
> -	if (&prev_lip->li_ail != &ailp->ail_head)
> -		ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0);
> +	if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
> +		spin_unlock(&ailp->ail_lock);
> +		ASSERT(0);
> +		spin_lock(&ailp->ail_lock);
> +	}
>  
> -	prev_lip = list_entry(lip->li_ail.next, xfs_log_item_t, li_ail);
> -	if (&prev_lip->li_ail != &ailp->ail_head)
> -		ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) >= 0);
> +	prev_lip = list_entry(lip->li_ail.prev, xfs_log_item_t, li_ail);
> +	if (&prev_lip->li_ail != &ailp->ail_head) {
> +		if (XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) > 0) {
> +			spin_unlock(&ailp->ail_lock);
> +			ASSERT(0);
> +			spin_lock(&ailp->ail_lock);
> +		}
> +	}
>  
> +	next_lip = list_entry(lip->li_ail.next, xfs_log_item_t, li_ail);
> +	if (&next_lip->li_ail != &ailp->ail_head) {
> +		if (XFS_LSN_CMP(next_lip->li_lsn, lip->li_lsn) < 0) {
> +			spin_unlock(&ailp->ail_lock);
> +			ASSERT(0);
> +			spin_lock(&ailp->ail_lock);
> +		}
> +	}

Otherwise seems Ok, but kind of ugly. What about something like the
following diff (applied on top of this patch)? Still hacky, but it
avoids the multiple lock cycles for each check failure and preserves the
actual assert strings. (Untested and probably could use comment
updates..).

Brian

--- 8< ---

diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 58a2cf6fd4d9..98798d15b863 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -45,37 +45,36 @@ xfs_ail_check(
 {
 	struct xfs_log_item	*prev_lip;
 	struct xfs_log_item	*next_lip;
+	xfs_lsn_t		prev_lsn = NULLCOMMITLSN;
+	xfs_lsn_t		next_lsn = NULLCOMMITLSN;
+	xfs_lsn_t		lsn;
+	bool			in_ail;
 
 	if (list_empty(&ailp->ail_head))
 		return;
 
+	in_ail = test_bit(XFS_LI_IN_AIL, &lip->li_flags);
+	prev_lip = list_entry(lip->li_ail.prev, struct xfs_log_item, li_ail);
+	if (&prev_lip->li_ail != &ailp->ail_head)
+		prev_lsn = prev_lip->li_lsn;
+	next_lip = list_entry(lip->li_ail.next, struct xfs_log_item, li_ail);
+	if (&next_lip->li_ail != &ailp->ail_head)
+		next_lsn = next_lip->li_lsn;
+	lsn = lip->li_lsn;
+
 	/*
 	 * Check the next and previous entries are valid.
 	 */
-	if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
-		spin_unlock(&ailp->ail_lock);
-		ASSERT(0);
-		spin_lock(&ailp->ail_lock);
-	}
-
-	prev_lip = list_entry(lip->li_ail.prev, xfs_log_item_t, li_ail);
-	if (&prev_lip->li_ail != &ailp->ail_head) {
-		if (XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) > 0) {
-			spin_unlock(&ailp->ail_lock);
-			ASSERT(0);
-			spin_lock(&ailp->ail_lock);
-		}
-	}
-
-	next_lip = list_entry(lip->li_ail.next, xfs_log_item_t, li_ail);
-	if (&next_lip->li_ail != &ailp->ail_head) {
-		if (XFS_LSN_CMP(next_lip->li_lsn, lip->li_lsn) < 0) {
-			spin_unlock(&ailp->ail_lock);
-			ASSERT(0);
-			spin_lock(&ailp->ail_lock);
-		}
-	}
+	if (in_ail &&
+	    (prev_lsn == NULLCOMMITLSN || XFS_LSN_CMP(prev_lsn, lsn) <= 0) &&
+	    (next_lsn == NULLCOMMITLSN || XFS_LSN_CMP(next_lsn, lsn) >= 0))
+		return;
 
+	spin_unlock(&ailp->ail_lock);
+	ASSERT(in_ail);
+	ASSERT(prev_lsn == NULLCOMMITLSN || XFS_LSN_CMP(prev_lsn, lsn) <= 0);
+	ASSERT(next_lsn == NULLCOMMITLSN || XFS_LSN_CMP(next_lsn, lsn) >= 0);
+	spin_lock(&ailp->ail_lock);
 }
 #else /* !DEBUG */
 #define	xfs_ail_check(a,l)

Re: [PATCH 05/10] xfs: don't assert fail with AIL lock held

[Christoph Hellwig]

> Otherwise seems Ok, but kind of ugly. What about something like the
> following diff (applied on top of this patch)? Still hacky, but it
> avoids the multiple lock cycles for each check failure and preserves the
> actual assert strings. (Untested and probably could use comment
> updates..).

This looks a little better.  But maybe we should just replace the
ASSERT statements with WARN_ON_ONCE calls to make them non-fatal
but otherwise leave things as-is?

Re: [PATCH 05/10] xfs: don't assert fail with AIL lock held

[Dave Chinner]

On Mon, May 07, 2018 at 07:50:48AM -0700, Christoph Hellwig wrote:
> > Otherwise seems Ok, but kind of ugly. What about something like the
> > following diff (applied on top of this patch)? Still hacky, but it
> > avoids the multiple lock cycles for each check failure and preserves the
> > actual assert strings. (Untested and probably could use comment
> > updates..).
> 
> This looks a little better.  But maybe we should just replace the
> ASSERT statements with WARN_ON_ONCE calls to make them non-fatal
> but otherwise leave things as-is?

Yup, it's cleaner than my change, but I still asserts here as it's
debug-only code and asserts stop tracing instantly. That makes it
much easier to debug problems this code detects because you don't
need to do any work to find where in the tracing output the error
was first detected....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 06/10] xfs: fix double ijoin in xfs_inactive_symlink_rmt()

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

xfs_inactive_symlink_rmt() does something nasty - it joins an inode
into a transaction it is already joined to. This means the inode can
have multiple log item descriptors attached to the transaction for
it. This breaks teh 1:1 mapping that is supposed to exist
between the log item and log item descriptor.

This results in the log item being processed twice during
transaction commit and CIL formatting, and there are lots of other
potential issues tha arise from double processing of log items in
the transaction commit state machine.

In this case, the inode is already held by the rolling transaction
returned from xfs_defer_finish(), so there's no need to join it
again.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_symlink.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index 5b66ac12913c..27870e5cd259 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -488,16 +488,11 @@ xfs_inactive_symlink_rmt(
 	error = xfs_defer_finish(&tp, &dfops);
 	if (error)
 		goto error_bmap_cancel;
-	/*
-	 * The first xact was committed, so add the inode to the new one.
-	 * Mark it dirty so it will be logged and moved forward in the log as
-	 * part of every commit.
-	 */
-	xfs_trans_ijoin(tp, ip, 0);
-	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+
 	/*
 	 * Commit the transaction containing extent freeing and EFDs.
 	 */
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 	error = xfs_trans_commit(tp);
 	if (error) {
 		ASSERT(XFS_FORCED_SHUTDOWN(mp));
-- 
2.17.0

Re: [PATCH 06/10] xfs: fix double ijoin in xfs_inactive_symlink_rmt()

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 07/10] xfs: fix double ijoin in xfs_reflink_cancel_cow_range

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

AN inode is joined to teh same transaction twice in
xfs_reflink_cancel_cow_range() resulting in the following assert
failure:

[   30.180485] XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740
[   30.183435] ------------[ cut here ]------------
......
[   30.209264] Call Trace:
[   30.209935]  xfs_trans_add_item+0xcc/0xe0
[   30.210968]  xfs_reflink_cancel_cow_blocks+0xab/0x290
[   30.212249]  ? xfs_trans_reserve+0x1b4/0x2b0
[   30.213320]  ? kmem_zone_alloc+0x61/0xe0
[   30.214321]  xfs_reflink_cancel_cow_range+0xb2/0x1f0
[   30.215616]  xfs_fs_destroy_inode+0x1bd/0x280
[   30.216757]  dispose_list+0x35/0x40
[   30.217656]  evict_inodes+0x132/0x160
[   30.218620]  generic_shutdown_super+0x3a/0x110
[   30.219771]  kill_block_super+0x21/0x50
[   30.220762]  deactivate_locked_super+0x39/0x70
[   30.221909]  cleanup_mnt+0x3b/0x70
[   30.222819]  task_work_run+0x7f/0xa0
[   30.223762]  exit_to_usermode_loop+0x9b/0xa0
[   30.224884]  do_syscall_64+0x18f/0x1a0

Fix it and document that the callers of
xfs_reflink_cancel_cow_blocks() must have already joined the inode
to the permanent transaction passed in.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_reflink.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index cdbd342a5249..bce2b5351d64 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -552,6 +552,9 @@ xfs_reflink_trim_irec_to_next_cow(
  *
  * If cancel_real is true this function cancels all COW fork extents for the
  * inode; if cancel_real is false, real extents are not cleared.
+ *
+ * Caller must have already joined the inode to the current transaction. The
+ * inode will be joined to the transaction returned to the caller.
  */
 int
 xfs_reflink_cancel_cow_blocks(
@@ -592,7 +595,6 @@ xfs_reflink_cancel_cow_blocks(
 			if (error)
 				break;
 		} else if (del.br_state == XFS_EXT_UNWRITTEN || cancel_real) {
-			xfs_trans_ijoin(*tpp, ip, 0);
 			xfs_defer_init(&dfops, &firstfsb);
 
 			/* Free the CoW orphan record. */
@@ -1570,6 +1572,7 @@ xfs_reflink_clear_inode_flag(
 	 * We didn't find any shared blocks so turn off the reflink flag.
 	 * First, get rid of any leftover CoW mappings.
 	 */
+	xfs_trans_ijoin(*tpp, ip, 0);
 	error = xfs_reflink_cancel_cow_blocks(ip, tpp, 0, NULLFILEOFF, true);
 	if (error)
 		return error;
@@ -1578,7 +1581,6 @@ xfs_reflink_clear_inode_flag(
 	trace_xfs_reflink_unset_inode_flag(ip);
 	ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
 	xfs_inode_clear_cowblocks_tag(ip);
-	xfs_trans_ijoin(*tpp, ip, 0);
 	xfs_trans_log_inode(*tpp, ip, XFS_ILOG_CORE);
 
 	return error;
-- 
2.17.0

Re: [PATCH 07/10] xfs: fix double ijoin in xfs_reflink_cancel_cow_range

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 08/10] xfs: fix double ijoin in xfs_reflink_clear_inode_flag()

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Another assert failure:

XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740
....
xfs_trans_add_item+0xcc/0xe0
xfs_reflink_clear_inode_flag+0x53/0x120
xfs_reflink_try_clear_inode_flag+0x5b/0xa0
? filemap_write_and_wait+0x4f/0x70
xfs_reflink_unshare+0x18e/0x19d
xfs_file_fallocate+0x241/0x310
? selinux_file_permission+0xd4/0x140
vfs_fallocate+0x13d/0x260
SyS_fallocate+0x43/0x80

Another fix.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_reflink.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index bce2b5351d64..12d441a73b53 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1553,7 +1553,12 @@ xfs_reflink_inode_has_shared_extents(
 	return 0;
 }
 
-/* Clear the inode reflink flag if there are no shared extents. */
+/*
+ * Clear the inode reflink flag if there are no shared extents.
+ *
+ * The caller is responsible for joining the inode to the transaction passed in.
+ * The inode will be joined to the transaction that is returned to the caller.
+ */
 int
 xfs_reflink_clear_inode_flag(
 	struct xfs_inode	*ip,
@@ -1572,7 +1577,6 @@ xfs_reflink_clear_inode_flag(
 	 * We didn't find any shared blocks so turn off the reflink flag.
 	 * First, get rid of any leftover CoW mappings.
 	 */
-	xfs_trans_ijoin(*tpp, ip, 0);
 	error = xfs_reflink_cancel_cow_blocks(ip, tpp, 0, NULLFILEOFF, true);
 	if (error)
 		return error;
-- 
2.17.0

Re: [PATCH 08/10] xfs: fix double ijoin in xfs_reflink_clear_inode_flag()

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 09/10] xfs: add some more debug checks to buffer log item reuse

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Just to make sure the item isn't associated with another
transaction when we try to reuse it.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_buf_item.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 53c07a7a14fb..3656f7cb18ad 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -722,8 +722,11 @@ xfs_buf_item_init(
 	 * nothing to do here so return.
 	 */
 	ASSERT(bp->b_target->bt_mount == mp);
-	if (bip != NULL) {
+	if (bip) {
 		ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
+		ASSERT(!test_bit(XFS_LI_TRANS, &bip->bli_item.li_flags));
+		ASSERT(!bp->b_transp);
+		ASSERT(bip->bli_buf == bp);
 		return 0;
 	}
 
-- 
2.17.0

Re: [PATCH 09/10] xfs: add some more debug checks to buffer log item reuse

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 10/10] xfs: get rid of the log item descriptor

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

It's just a connector between a transaction and a log item. There's
a 1:1 relationship between a log item descriptor and a log item,
and a 1:1 relationship between a log item descriptor and a
transaction. Both relationships are created and terminated at the
same time, so why do we even have the descriptor?

Replace it with a specific list_head in the log item and a new
log item dirtied flag to replace the XFS_LID_DIRTY flag.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.c    |  8 +++----
 fs/xfs/libxfs/xfs_shared.h  | 15 ------------
 fs/xfs/xfs_buf_item.c       |  2 +-
 fs/xfs/xfs_icreate_item.c   |  2 +-
 fs/xfs/xfs_log.c            |  9 ++++---
 fs/xfs/xfs_log_cil.c        | 25 +++++++++----------
 fs/xfs/xfs_super.c          | 10 +-------
 fs/xfs/xfs_trace.h          |  5 +---
 fs/xfs/xfs_trans.c          | 48 ++++++++++---------------------------
 fs/xfs/xfs_trans.h          |  8 +++----
 fs/xfs/xfs_trans_bmap.c     |  4 ++--
 fs/xfs/xfs_trans_buf.c      | 22 +++++++----------
 fs/xfs/xfs_trans_dquot.c    |  4 ++--
 fs/xfs/xfs_trans_extfree.c  |  4 ++--
 fs/xfs/xfs_trans_inode.c    |  3 +--
 fs/xfs/xfs_trans_priv.h     |  1 -
 fs/xfs/xfs_trans_refcount.c |  4 ++--
 fs/xfs/xfs_trans_rmap.c     |  4 ++--
 18 files changed, 60 insertions(+), 118 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 040eeda8426f..ab90998c1867 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -246,7 +246,7 @@ xfs_bmap_get_bp(
 	struct xfs_btree_cur	*cur,
 	xfs_fsblock_t		bno)
 {
-	struct xfs_log_item_desc *lidp;
+	struct xfs_log_item	*lip;
 	int			i;
 
 	if (!cur)
@@ -260,9 +260,9 @@ xfs_bmap_get_bp(
 	}
 
 	/* Chase down all the log items to see if the bp is there */
-	list_for_each_entry(lidp, &cur->bc_tp->t_items, lid_trans) {
-		struct xfs_buf_log_item	*bip;
-		bip = (struct xfs_buf_log_item *)lidp->lid_item;
+	list_for_each_entry(lip, &cur->bc_tp->t_items, li_trans) {
+		struct xfs_buf_log_item	*bip = (struct xfs_buf_log_item *)lip;
+
 		if (bip->bli_item.li_type == XFS_LI_BUF &&
 		    XFS_BUF_ADDR(bip->bli_buf) == bno)
 			return bip->bli_buf;
diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
index d0b84da0cb1e..8efc06e62b13 100644
--- a/fs/xfs/libxfs/xfs_shared.h
+++ b/fs/xfs/libxfs/xfs_shared.h
@@ -57,21 +57,6 @@ extern const struct xfs_buf_ops xfs_sb_quiet_buf_ops;
 extern const struct xfs_buf_ops xfs_symlink_buf_ops;
 extern const struct xfs_buf_ops xfs_rtbuf_ops;
 
-/*
- * This structure is used to track log items associated with
- * a transaction.  It points to the log item and keeps some
- * flags to track the state of the log item.  It also tracks
- * the amount of space needed to log the item it describes
- * once we get to commit processing (see xfs_trans_commit()).
- */
-struct xfs_log_item_desc {
-	struct xfs_log_item	*lid_item;
-	struct list_head	lid_trans;
-	unsigned char		lid_flags;
-};
-
-#define XFS_LID_DIRTY		0x1
-
 /* log size calculation functions */
 int	xfs_log_calc_unit_res(struct xfs_mount *mp, int unit_bytes);
 int	xfs_log_calc_minimum_size(struct xfs_mount *);
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 3656f7cb18ad..b2c00c18fd8e 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -415,7 +415,7 @@ xfs_buf_item_unpin(
 			 * xfs_trans_uncommit() will try to reference the
 			 * buffer which we no longer have a hold on.
 			 */
-			if (lip->li_desc)
+			if (!list_empty(&lip->li_trans))
 				xfs_trans_del_item(lip);
 
 			/*
diff --git a/fs/xfs/xfs_icreate_item.c b/fs/xfs/xfs_icreate_item.c
index a99a0f8aa528..5da9599156ed 100644
--- a/fs/xfs/xfs_icreate_item.c
+++ b/fs/xfs/xfs_icreate_item.c
@@ -184,5 +184,5 @@ xfs_icreate_log(
 
 	xfs_trans_add_item(tp, &icp->ic_item);
 	tp->t_flags |= XFS_TRANS_DIRTY;
-	icp->ic_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+	set_bit(XFS_LI_DIRTY, &icp->ic_item.li_flags);
 }
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index e427864434c1..703027a6a0fb 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2110,10 +2110,10 @@ xlog_print_tic_res(
  */
 void
 xlog_print_trans(
-	struct xfs_trans		*tp)
+	struct xfs_trans	*tp)
 {
-	struct xfs_mount		*mp = tp->t_mountp;
-	struct xfs_log_item_desc	*lidp;
+	struct xfs_mount	*mp = tp->t_mountp;
+	struct xfs_log_item	*lip;
 
 	/* dump core transaction and ticket info */
 	xfs_warn(mp, "transaction summary:");
@@ -2124,8 +2124,7 @@ xlog_print_trans(
 	xlog_print_tic_res(mp, tp->t_ticket);
 
 	/* dump each log item */
-	list_for_each_entry(lidp, &tp->t_items, lid_trans) {
-		struct xfs_log_item	*lip = lidp->lid_item;
+	list_for_each_entry(lip, &tp->t_items, li_trans) {
 		struct xfs_log_vec	*lv = lip->li_lv;
 		struct xfs_log_iovec	*vec;
 		int			i;
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 8342f4ee83e7..5c66347f78a4 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -141,10 +141,9 @@ xlog_cil_alloc_shadow_bufs(
 	struct xlog		*log,
 	struct xfs_trans	*tp)
 {
-	struct xfs_log_item_desc *lidp;
+	struct xfs_log_item	*lip;
 
-	list_for_each_entry(lidp, &tp->t_items, lid_trans) {
-		struct xfs_log_item *lip = lidp->lid_item;
+	list_for_each_entry(lip, &tp->t_items, li_trans) {
 		struct xfs_log_vec *lv;
 		int	niovecs = 0;
 		int	nbytes = 0;
@@ -152,7 +151,7 @@ xlog_cil_alloc_shadow_bufs(
 		bool	ordered = false;
 
 		/* Skip items which aren't dirty in this transaction. */
-		if (!(lidp->lid_flags & XFS_LID_DIRTY))
+		if (!test_bit(XFS_LI_DIRTY, &lip->li_flags))
 			continue;
 
 		/* get number of vecs and size of data to be stored */
@@ -317,7 +316,7 @@ xlog_cil_insert_format_items(
 	int			*diff_len,
 	int			*diff_iovecs)
 {
-	struct xfs_log_item_desc *lidp;
+	struct xfs_log_item	*lip;
 
 
 	/* Bail out if we didn't find a log item.  */
@@ -326,15 +325,14 @@ xlog_cil_insert_format_items(
 		return;
 	}
 
-	list_for_each_entry(lidp, &tp->t_items, lid_trans) {
-		struct xfs_log_item *lip = lidp->lid_item;
+	list_for_each_entry(lip, &tp->t_items, li_trans) {
 		struct xfs_log_vec *lv;
 		struct xfs_log_vec *old_lv = NULL;
 		struct xfs_log_vec *shadow;
 		bool	ordered = false;
 
 		/* Skip items which aren't dirty in this transaction. */
-		if (!(lidp->lid_flags & XFS_LID_DIRTY))
+		if (!test_bit(XFS_LI_DIRTY, &lip->li_flags))
 			continue;
 
 		/*
@@ -348,11 +346,11 @@ xlog_cil_insert_format_items(
 		/*
 		 * Skip items that do not have any vectors for writing. These
 		 * log items must now be considered clean in this transaction,
-		 * so clear the XFS_LID_DIRTY flag to ensure it doesn't get
+		 * so clear the XFS_LI_DIRTY flag to ensure it doesn't get
 		 * added to the CIL by mistake.
 		 */
 		if (!shadow->lv_niovecs && !ordered) {
-			lidp->lid_flags &= ~XFS_LID_DIRTY;
+			clear_bit(XFS_LI_DIRTY, &lip->li_flags);
 			continue;
 		}
 
@@ -413,7 +411,7 @@ xlog_cil_insert_items(
 {
 	struct xfs_cil		*cil = log->l_cilp;
 	struct xfs_cil_ctx	*ctx = cil->xc_ctx;
-	struct xfs_log_item_desc *lidp;
+	struct xfs_log_item	*lip;
 	int			len = 0;
 	int			diff_iovecs = 0;
 	int			iclog_space;
@@ -486,11 +484,10 @@ xlog_cil_insert_items(
 	 * We do this here so we only need to take the CIL lock once during
 	 * the transaction commit.
 	 */
-	list_for_each_entry(lidp, &tp->t_items, lid_trans) {
-		struct xfs_log_item	*lip = lidp->lid_item;
+	list_for_each_entry(lip, &tp->t_items, li_trans) {
 
 		/* Skip items which aren't dirty in this transaction. */
-		if (!(lidp->lid_flags & XFS_LID_DIRTY))
+		if (!test_bit(XFS_LI_DIRTY, &lip->li_flags))
 			continue;
 
 		/*
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index d71424052917..5726ef496980 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1880,11 +1880,6 @@ xfs_init_zones(void)
 	if (!xfs_trans_zone)
 		goto out_destroy_ifork_zone;
 
-	xfs_log_item_desc_zone =
-		kmem_zone_init(sizeof(struct xfs_log_item_desc),
-			       "xfs_log_item_desc");
-	if (!xfs_log_item_desc_zone)
-		goto out_destroy_trans_zone;
 
 	/*
 	 * The size of the zone allocated buf log item is the maximum
@@ -1894,7 +1889,7 @@ xfs_init_zones(void)
 	xfs_buf_item_zone = kmem_zone_init(sizeof(struct xfs_buf_log_item),
 					   "xfs_buf_item");
 	if (!xfs_buf_item_zone)
-		goto out_destroy_log_item_desc_zone;
+		goto out_destroy_trans_zone;
 
 	xfs_efd_zone = kmem_zone_init((sizeof(xfs_efd_log_item_t) +
 			((XFS_EFD_MAX_FAST_EXTENTS - 1) *
@@ -1982,8 +1977,6 @@ xfs_init_zones(void)
 	kmem_zone_destroy(xfs_efd_zone);
  out_destroy_buf_item_zone:
 	kmem_zone_destroy(xfs_buf_item_zone);
- out_destroy_log_item_desc_zone:
-	kmem_zone_destroy(xfs_log_item_desc_zone);
  out_destroy_trans_zone:
 	kmem_zone_destroy(xfs_trans_zone);
  out_destroy_ifork_zone:
@@ -2022,7 +2015,6 @@ xfs_destroy_zones(void)
 	kmem_zone_destroy(xfs_efi_zone);
 	kmem_zone_destroy(xfs_efd_zone);
 	kmem_zone_destroy(xfs_buf_item_zone);
-	kmem_zone_destroy(xfs_log_item_desc_zone);
 	kmem_zone_destroy(xfs_trans_zone);
 	kmem_zone_destroy(xfs_ifork_zone);
 	kmem_zone_destroy(xfs_da_state_zone);
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 947ab6e32c18..28a5b3f876ad 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -441,7 +441,6 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
 		__field(unsigned, bli_recur)
 		__field(int, bli_refcount)
 		__field(unsigned, bli_flags)
-		__field(void *, li_desc)
 		__field(unsigned long, li_flags)
 	),
 	TP_fast_assign(
@@ -455,12 +454,11 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
 		__entry->buf_hold = atomic_read(&bip->bli_buf->b_hold);
 		__entry->buf_pincount = atomic_read(&bip->bli_buf->b_pin_count);
 		__entry->buf_lockval = bip->bli_buf->b_sema.count;
-		__entry->li_desc = bip->bli_item.li_desc;
 		__entry->li_flags = bip->bli_item.li_flags;
 	),
 	TP_printk("dev %d:%d bno 0x%llx len 0x%zx hold %d pincount %d "
 		  "lock %d flags %s recur %d refcount %d bliflags %s "
-		  "lidesc %p liflags %s",
+		  "liflags %s",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  (unsigned long long)__entry->buf_bno,
 		  __entry->buf_len,
@@ -471,7 +469,6 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
 		  __entry->bli_recur,
 		  __entry->bli_refcount,
 		  __print_flags(__entry->bli_flags, "|", XFS_BLI_FLAGS),
-		  __entry->li_desc,
 		  __print_flags(__entry->li_flags, "|", XFS_LI_FLAGS))
 )
 
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 63c22aa02cd8..8d83efbc01c3 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -33,7 +33,6 @@
 #include "xfs_error.h"
 
 kmem_zone_t	*xfs_trans_zone;
-kmem_zone_t	*xfs_log_item_desc_zone;
 
 #if defined(CONFIG_TRACEPOINTS)
 static void
@@ -734,70 +733,49 @@ xfs_trans_unreserve_and_mod_sb(
 
 /*
  * Add the given log item to the transaction's list of log items.
- *
- * The log item will now point to its new descriptor with its li_desc field.
  */
 void
 xfs_trans_add_item(
 	struct xfs_trans	*tp,
 	struct xfs_log_item	*lip)
 {
-	struct xfs_log_item_desc *lidp;
-
 	ASSERT(lip->li_mountp == tp->t_mountp);
 	ASSERT(lip->li_ailp == tp->t_mountp->m_ail);
 	ASSERT(!test_bit(XFS_LI_TRANS, &lip->li_flags));
+	ASSERT(!test_bit(XFS_LI_DIRTY, &lip->li_flags));
 
-	lidp = kmem_zone_zalloc(xfs_log_item_desc_zone, KM_SLEEP | KM_NOFS);
-
-	lidp->lid_item = lip;
-	lidp->lid_flags = 0;
-	list_add_tail(&lidp->lid_trans, &tp->t_items);
-
-	lip->li_desc = lidp;
+	list_add_tail(&lip->li_trans, &tp->t_items);
 	set_bit(XFS_LI_TRANS, &lip->li_flags);
 	trace_xfs_trans_add_item(tp, _RET_IP_);
 }
 
-STATIC void
-xfs_trans_free_item_desc(
-	struct xfs_log_item_desc *lidp)
-{
-	list_del_init(&lidp->lid_trans);
-	kmem_zone_free(xfs_log_item_desc_zone, lidp);
-}
-
 /*
- * Unlink and free the given descriptor.
+ * Unlink the log item from the transaction. The log item is no longer
+ * considered dirty in this transaction, as the linked transaction has
+ * finished, either by abort or commit completion.
  */
 void
 xfs_trans_del_item(
 	struct xfs_log_item	*lip)
 {
 	clear_bit(XFS_LI_TRANS, &lip->li_flags);
-	xfs_trans_free_item_desc(lip->li_desc);
-	lip->li_desc = NULL;
+	clear_bit(XFS_LI_DIRTY, &lip->li_flags);
+	list_del_init(&lip->li_trans);
 }
 
-/*
- * Unlock all of the items of a transaction and free all the descriptors
- * of that transaction.
- */
+/* Detach and unlock all of the items in a transaction */
 void
 xfs_trans_free_items(
 	struct xfs_trans	*tp,
 	xfs_lsn_t		commit_lsn,
 	bool			abort)
 {
-	struct xfs_log_item_desc *lidp, *next;
+	struct xfs_log_item	*lip, *next;
 
 	trace_xfs_trans_free_items(tp, _RET_IP_);
 
-	list_for_each_entry_safe(lidp, next, &tp->t_items, lid_trans) {
-		struct xfs_log_item	*lip = lidp->lid_item;
-
+	list_for_each_entry_safe(lip, next, &tp->t_items, li_trans) {
 		xfs_trans_del_item(lip);
-
 		if (commit_lsn != NULLCOMMITLSN)
 			lip->li_ops->iop_committing(lip, commit_lsn);
 		if (abort)
@@ -1048,10 +1026,10 @@ xfs_trans_cancel(
 	}
 #ifdef DEBUG
 	if (!dirty && !XFS_FORCED_SHUTDOWN(mp)) {
-		struct xfs_log_item_desc *lidp;
+		struct xfs_log_item *lip;
 
-		list_for_each_entry(lidp, &tp->t_items, lid_trans)
-			ASSERT(!(lidp->lid_item->li_type == XFS_LI_EFD));
+		list_for_each_entry(lip, &tp->t_items, li_trans)
+			ASSERT(!(lip->li_type == XFS_LI_EFD));
 	}
 #endif
 	xfs_trans_unreserve_and_mod_sb(tp);
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index af52107adffc..05b1e1d0a318 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -27,7 +27,6 @@ struct xfs_efi_log_item;
 struct xfs_inode;
 struct xfs_item_ops;
 struct xfs_log_iovec;
-struct xfs_log_item_desc;
 struct xfs_mount;
 struct xfs_trans;
 struct xfs_trans_res;
@@ -43,8 +42,8 @@ struct xfs_bud_log_item;
 
 typedef struct xfs_log_item {
 	struct list_head		li_ail;		/* AIL pointers */
+	struct list_head		li_trans;	/* transaction list */
 	xfs_lsn_t			li_lsn;		/* last on-disk lsn */
-	struct xfs_log_item_desc	*li_desc;	/* ptr to current desc*/
 	struct xfs_mount		*li_mountp;	/* ptr to fs mount */
 	struct xfs_ail			*li_ailp;	/* ptr to AIL */
 	uint				li_type;	/* item type */
@@ -73,12 +72,14 @@ typedef struct xfs_log_item {
 #define	XFS_LI_ABORTED	1
 #define	XFS_LI_FAILED	2
 #define	XFS_LI_TRANS	3	/* attached to an active transaction */
+#define	XFS_LI_DIRTY	4	/* log item dirty in transaction */
 
 #define XFS_LI_FLAGS \
 	{ (1 << XFS_LI_IN_AIL),		"IN_AIL" }, \
 	{ (1 << XFS_LI_ABORTED),	"ABORTED" }, \
 	{ (1 << XFS_LI_FAILED),		"FAILED" }, \
-	{ (1 << XFS_LI_TRANS),		"TRANS" }
+	{ (1 << XFS_LI_TRANS),		"TRANS" }, \
+	{ (1 << XFS_LI_DIRTY),		"DIRTY" }
 
 struct xfs_item_ops {
 	void (*iop_size)(xfs_log_item_t *, int *, int *);
@@ -249,7 +250,6 @@ void		xfs_trans_buf_copy_type(struct xfs_buf *dst_bp,
 					struct xfs_buf *src_bp);
 
 extern kmem_zone_t	*xfs_trans_zone;
-extern kmem_zone_t	*xfs_log_item_desc_zone;
 
 /* rmap updates */
 enum xfs_rmap_intent_type;
diff --git a/fs/xfs/xfs_trans_bmap.c b/fs/xfs/xfs_trans_bmap.c
index 14543d93cd4b..230a21df4b12 100644
--- a/fs/xfs/xfs_trans_bmap.c
+++ b/fs/xfs/xfs_trans_bmap.c
@@ -79,7 +79,7 @@ xfs_trans_log_finish_bmap_update(
 	 * 2.) shuts down the filesystem
 	 */
 	tp->t_flags |= XFS_TRANS_DIRTY;
-	budp->bud_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+	set_bit(XFS_LI_DIRTY, &budp->bud_item.li_flags);
 
 	return error;
 }
@@ -158,7 +158,7 @@ xfs_bmap_update_log_item(
 	bmap = container_of(item, struct xfs_bmap_intent, bi_list);
 
 	tp->t_flags |= XFS_TRANS_DIRTY;
-	buip->bui_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+	set_bit(XFS_LI_DIRTY, &buip->bui_item.li_flags);
 
 	/*
 	 * atomic_inc_return gives us the value after the increment;
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index 88557dfea21f..3e9e2f6d8215 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -40,7 +40,7 @@ xfs_trans_buf_item_match(
 	struct xfs_buf_map	*map,
 	int			nmaps)
 {
-	struct xfs_log_item_desc *lidp;
+	struct xfs_log_item	*lip;
 	struct xfs_buf_log_item	*blip;
 	int			len = 0;
 	int			i;
@@ -48,8 +48,8 @@ xfs_trans_buf_item_match(
 	for (i = 0; i < nmaps; i++)
 		len += map[i].bm_len;
 
-	list_for_each_entry(lidp, &tp->t_items, lid_trans) {
-		blip = (struct xfs_buf_log_item *)lidp->lid_item;
+	list_for_each_entry(lip, &tp->t_items, li_trans) {
+		blip = (struct xfs_buf_log_item *)lip;
 		if (blip->bli_item.li_type == XFS_LI_BUF &&
 		    blip->bli_buf->b_target == target &&
 		    XFS_BUF_ADDR(blip->bli_buf) == map[0].bm_bn &&
@@ -100,14 +100,10 @@ _xfs_trans_bjoin(
 	atomic_inc(&bip->bli_refcount);
 
 	/*
-	 * Get a log_item_desc to point at the new item.
+	 * Attach the item to the transaction so we can find it in
+	 * xfs_trans_get_buf() and friends.
 	 */
 	xfs_trans_add_item(tp, &bip->bli_item);
-
-	/*
-	 * Initialize b_fsprivate2 so we can find it with incore_match()
-	 * in xfs_trans_get_buf() and friends above.
-	 */
 	bp->b_transp = tp;
 
 }
@@ -391,7 +387,7 @@ xfs_trans_brelse(
 	 * If the buffer is dirty within this transaction, we can't
 	 * release it until we commit.
 	 */
-	if (bip->bli_item.li_desc->lid_flags & XFS_LID_DIRTY)
+	if (test_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags))
 		return;
 
 	/*
@@ -542,7 +538,7 @@ xfs_trans_dirty_buf(
 	bip->bli_flags |= XFS_BLI_DIRTY | XFS_BLI_LOGGED;
 
 	tp->t_flags |= XFS_TRANS_DIRTY;
-	bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+	set_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags);
 }
 
 /*
@@ -623,7 +619,7 @@ xfs_trans_binval(
 		ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_INODE_BUF));
 		ASSERT(!(bip->__bli_format.blf_flags & XFS_BLFT_MASK));
 		ASSERT(bip->__bli_format.blf_flags & XFS_BLF_CANCEL);
-		ASSERT(bip->bli_item.li_desc->lid_flags & XFS_LID_DIRTY);
+		ASSERT(test_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags));
 		ASSERT(tp->t_flags & XFS_TRANS_DIRTY);
 		return;
 	}
@@ -633,7 +629,7 @@ xfs_trans_binval(
 	bip->bli_flags |= XFS_BLI_STALE;
 
 	/* Mark the log item as dirty. */
-	bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+	set_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags);
 	tp->t_flags |= XFS_TRANS_DIRTY;
 
 	/*
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index c3d547211d16..c381c02cca45 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -77,7 +77,7 @@ xfs_trans_log_dquot(
 	ASSERT(XFS_DQ_IS_LOCKED(dqp));
 
 	tp->t_flags |= XFS_TRANS_DIRTY;
-	dqp->q_logitem.qli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+	set_bit(XFS_LI_DIRTY, &dqp->q_logitem.qli_item.li_flags);
 }
 
 /*
@@ -879,7 +879,7 @@ xfs_trans_log_quotaoff_item(
 	xfs_qoff_logitem_t	*qlp)
 {
 	tp->t_flags |= XFS_TRANS_DIRTY;
-	qlp->qql_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+	set_bit(XFS_LI_DIRTY, &qlp->qql_item.li_flags);
 }
 
 STATIC void
diff --git a/fs/xfs/xfs_trans_extfree.c b/fs/xfs/xfs_trans_extfree.c
index ab438647592a..fb631081d829 100644
--- a/fs/xfs/xfs_trans_extfree.c
+++ b/fs/xfs/xfs_trans_extfree.c
@@ -90,7 +90,7 @@ xfs_trans_free_extent(
 	 * 2.) shuts down the filesystem
 	 */
 	tp->t_flags |= XFS_TRANS_DIRTY;
-	efdp->efd_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+	set_bit(XFS_LI_DIRTY, &efdp->efd_item.li_flags);
 
 	next_extent = efdp->efd_next_extent;
 	ASSERT(next_extent < efdp->efd_format.efd_nextents);
@@ -155,7 +155,7 @@ xfs_extent_free_log_item(
 	free = container_of(item, struct xfs_extent_free_item, xefi_list);
 
 	tp->t_flags |= XFS_TRANS_DIRTY;
-	efip->efi_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+	set_bit(XFS_LI_DIRTY, &efip->efi_item.li_flags);
 
 	/*
 	 * atomic_inc_return gives us the value after the increment;
diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
index 07cea592dc01..f7bd7960a90f 100644
--- a/fs/xfs/xfs_trans_inode.c
+++ b/fs/xfs/xfs_trans_inode.c
@@ -133,14 +133,13 @@ xfs_trans_log_inode(
 	 * set however, then go ahead and bump the i_version counter
 	 * unconditionally.
 	 */
-	if (!(ip->i_itemp->ili_item.li_desc->lid_flags & XFS_LID_DIRTY) &&
+	if (!test_and_set_bit(XFS_LI_DIRTY, &ip->i_itemp->ili_item.li_flags) &&
 	    IS_I_VERSION(VFS_I(ip))) {
 		if (inode_maybe_inc_iversion(VFS_I(ip), flags & XFS_ILOG_CORE))
 			flags |= XFS_ILOG_CORE;
 	}
 
 	tp->t_flags |= XFS_TRANS_DIRTY;
-	ip->i_itemp->ili_item.li_desc->lid_flags |= XFS_LID_DIRTY;
 
 	/*
 	 * Always OR in the bits from the ili_last_fields field.
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index 43f773297b9d..9717ae74b36d 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -19,7 +19,6 @@
 #define	__XFS_TRANS_PRIV_H__
 
 struct xfs_log_item;
-struct xfs_log_item_desc;
 struct xfs_mount;
 struct xfs_trans;
 struct xfs_ail;
diff --git a/fs/xfs/xfs_trans_refcount.c b/fs/xfs/xfs_trans_refcount.c
index 94c1877af834..c7f8e82f5bda 100644
--- a/fs/xfs/xfs_trans_refcount.c
+++ b/fs/xfs/xfs_trans_refcount.c
@@ -77,7 +77,7 @@ xfs_trans_log_finish_refcount_update(
 	 * 2.) shuts down the filesystem
 	 */
 	tp->t_flags |= XFS_TRANS_DIRTY;
-	cudp->cud_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+	set_bit(XFS_LI_DIRTY, &cudp->cud_item.li_flags);
 
 	return error;
 }
@@ -154,7 +154,7 @@ xfs_refcount_update_log_item(
 	refc = container_of(item, struct xfs_refcount_intent, ri_list);
 
 	tp->t_flags |= XFS_TRANS_DIRTY;
-	cuip->cui_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+	set_bit(XFS_LI_DIRTY, &cuip->cui_item.li_flags);
 
 	/*
 	 * atomic_inc_return gives us the value after the increment;
diff --git a/fs/xfs/xfs_trans_rmap.c b/fs/xfs/xfs_trans_rmap.c
index 9b577beb43d7..5831ca0c270b 100644
--- a/fs/xfs/xfs_trans_rmap.c
+++ b/fs/xfs/xfs_trans_rmap.c
@@ -117,7 +117,7 @@ xfs_trans_log_finish_rmap_update(
 	 * 2.) shuts down the filesystem
 	 */
 	tp->t_flags |= XFS_TRANS_DIRTY;
-	rudp->rud_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+	set_bit(XFS_LI_DIRTY, &rudp->rud_item.li_flags);
 
 	return error;
 }
@@ -175,7 +175,7 @@ xfs_rmap_update_log_item(
 	rmap = container_of(item, struct xfs_rmap_intent, ri_list);
 
 	tp->t_flags |= XFS_TRANS_DIRTY;
-	ruip->rui_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+	set_bit(XFS_LI_DIRTY, &ruip->rui_item.li_flags);
 
 	/*
 	 * atomic_inc_return gives us the value after the increment;
-- 
2.17.0

Re: [PATCH 10/10] xfs: get rid of the log item descriptor

[Darrick J. Wong]

On Wed, May 02, 2018 at 06:01:57PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> It's just a connector between a transaction and a log item. There's
> a 1:1 relationship between a log item descriptor and a log item,
> and a 1:1 relationship between a log item descriptor and a
> transaction. Both relationships are created and terminated at the
> same time, so why do we even have the descriptor?
> 
> Replace it with a specific list_head in the log item and a new
> log item dirtied flag to replace the XFS_LID_DIRTY flag.
> 
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_bmap.c    |  8 +++----
>  fs/xfs/libxfs/xfs_shared.h  | 15 ------------
>  fs/xfs/xfs_buf_item.c       |  2 +-
>  fs/xfs/xfs_icreate_item.c   |  2 +-
>  fs/xfs/xfs_log.c            |  9 ++++---
>  fs/xfs/xfs_log_cil.c        | 25 +++++++++----------
>  fs/xfs/xfs_super.c          | 10 +-------
>  fs/xfs/xfs_trace.h          |  5 +---
>  fs/xfs/xfs_trans.c          | 48 ++++++++++---------------------------
>  fs/xfs/xfs_trans.h          |  8 +++----
>  fs/xfs/xfs_trans_bmap.c     |  4 ++--
>  fs/xfs/xfs_trans_buf.c      | 22 +++++++----------
>  fs/xfs/xfs_trans_dquot.c    |  4 ++--
>  fs/xfs/xfs_trans_extfree.c  |  4 ++--
>  fs/xfs/xfs_trans_inode.c    |  3 +--
>  fs/xfs/xfs_trans_priv.h     |  1 -
>  fs/xfs/xfs_trans_refcount.c |  4 ++--
>  fs/xfs/xfs_trans_rmap.c     |  4 ++--
>  18 files changed, 60 insertions(+), 118 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 040eeda8426f..ab90998c1867 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -246,7 +246,7 @@ xfs_bmap_get_bp(
>  	struct xfs_btree_cur	*cur,
>  	xfs_fsblock_t		bno)
>  {
> -	struct xfs_log_item_desc *lidp;
> +	struct xfs_log_item	*lip;
>  	int			i;
>  
>  	if (!cur)
> @@ -260,9 +260,9 @@ xfs_bmap_get_bp(
>  	}
>  
>  	/* Chase down all the log items to see if the bp is there */
> -	list_for_each_entry(lidp, &cur->bc_tp->t_items, lid_trans) {
> -		struct xfs_buf_log_item	*bip;
> -		bip = (struct xfs_buf_log_item *)lidp->lid_item;
> +	list_for_each_entry(lip, &cur->bc_tp->t_items, li_trans) {
> +		struct xfs_buf_log_item	*bip = (struct xfs_buf_log_item *)lip;
> +
>  		if (bip->bli_item.li_type == XFS_LI_BUF &&
>  		    XFS_BUF_ADDR(bip->bli_buf) == bno)
>  			return bip->bli_buf;
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index d0b84da0cb1e..8efc06e62b13 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -57,21 +57,6 @@ extern const struct xfs_buf_ops xfs_sb_quiet_buf_ops;
>  extern const struct xfs_buf_ops xfs_symlink_buf_ops;
>  extern const struct xfs_buf_ops xfs_rtbuf_ops;
>  
> -/*
> - * This structure is used to track log items associated with
> - * a transaction.  It points to the log item and keeps some
> - * flags to track the state of the log item.  It also tracks
> - * the amount of space needed to log the item it describes
> - * once we get to commit processing (see xfs_trans_commit()).
> - */
> -struct xfs_log_item_desc {
> -	struct xfs_log_item	*lid_item;
> -	struct list_head	lid_trans;
> -	unsigned char		lid_flags;
> -};
> -
> -#define XFS_LID_DIRTY		0x1
> -
>  /* log size calculation functions */
>  int	xfs_log_calc_unit_res(struct xfs_mount *mp, int unit_bytes);
>  int	xfs_log_calc_minimum_size(struct xfs_mount *);
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 3656f7cb18ad..b2c00c18fd8e 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -415,7 +415,7 @@ xfs_buf_item_unpin(
>  			 * xfs_trans_uncommit() will try to reference the
>  			 * buffer which we no longer have a hold on.
>  			 */
> -			if (lip->li_desc)
> +			if (!list_empty(&lip->li_trans))
>  				xfs_trans_del_item(lip);
>  
>  			/*
> diff --git a/fs/xfs/xfs_icreate_item.c b/fs/xfs/xfs_icreate_item.c
> index a99a0f8aa528..5da9599156ed 100644
> --- a/fs/xfs/xfs_icreate_item.c
> +++ b/fs/xfs/xfs_icreate_item.c
> @@ -184,5 +184,5 @@ xfs_icreate_log(
>  
>  	xfs_trans_add_item(tp, &icp->ic_item);
>  	tp->t_flags |= XFS_TRANS_DIRTY;
> -	icp->ic_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> +	set_bit(XFS_LI_DIRTY, &icp->ic_item.li_flags);
>  }
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index e427864434c1..703027a6a0fb 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2110,10 +2110,10 @@ xlog_print_tic_res(
>   */
>  void
>  xlog_print_trans(
> -	struct xfs_trans		*tp)
> +	struct xfs_trans	*tp)
>  {
> -	struct xfs_mount		*mp = tp->t_mountp;
> -	struct xfs_log_item_desc	*lidp;
> +	struct xfs_mount	*mp = tp->t_mountp;
> +	struct xfs_log_item	*lip;
>  
>  	/* dump core transaction and ticket info */
>  	xfs_warn(mp, "transaction summary:");
> @@ -2124,8 +2124,7 @@ xlog_print_trans(
>  	xlog_print_tic_res(mp, tp->t_ticket);
>  
>  	/* dump each log item */
> -	list_for_each_entry(lidp, &tp->t_items, lid_trans) {
> -		struct xfs_log_item	*lip = lidp->lid_item;
> +	list_for_each_entry(lip, &tp->t_items, li_trans) {
>  		struct xfs_log_vec	*lv = lip->li_lv;
>  		struct xfs_log_iovec	*vec;
>  		int			i;
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 8342f4ee83e7..5c66347f78a4 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -141,10 +141,9 @@ xlog_cil_alloc_shadow_bufs(
>  	struct xlog		*log,
>  	struct xfs_trans	*tp)
>  {
> -	struct xfs_log_item_desc *lidp;
> +	struct xfs_log_item	*lip;
>  
> -	list_for_each_entry(lidp, &tp->t_items, lid_trans) {
> -		struct xfs_log_item *lip = lidp->lid_item;
> +	list_for_each_entry(lip, &tp->t_items, li_trans) {
>  		struct xfs_log_vec *lv;
>  		int	niovecs = 0;
>  		int	nbytes = 0;
> @@ -152,7 +151,7 @@ xlog_cil_alloc_shadow_bufs(
>  		bool	ordered = false;
>  
>  		/* Skip items which aren't dirty in this transaction. */
> -		if (!(lidp->lid_flags & XFS_LID_DIRTY))
> +		if (!test_bit(XFS_LI_DIRTY, &lip->li_flags))
>  			continue;
>  
>  		/* get number of vecs and size of data to be stored */
> @@ -317,7 +316,7 @@ xlog_cil_insert_format_items(
>  	int			*diff_len,
>  	int			*diff_iovecs)
>  {
> -	struct xfs_log_item_desc *lidp;
> +	struct xfs_log_item	*lip;
>  
>  
>  	/* Bail out if we didn't find a log item.  */
> @@ -326,15 +325,14 @@ xlog_cil_insert_format_items(
>  		return;
>  	}
>  
> -	list_for_each_entry(lidp, &tp->t_items, lid_trans) {
> -		struct xfs_log_item *lip = lidp->lid_item;
> +	list_for_each_entry(lip, &tp->t_items, li_trans) {
>  		struct xfs_log_vec *lv;
>  		struct xfs_log_vec *old_lv = NULL;
>  		struct xfs_log_vec *shadow;
>  		bool	ordered = false;
>  
>  		/* Skip items which aren't dirty in this transaction. */
> -		if (!(lidp->lid_flags & XFS_LID_DIRTY))
> +		if (!test_bit(XFS_LI_DIRTY, &lip->li_flags))
>  			continue;
>  
>  		/*
> @@ -348,11 +346,11 @@ xlog_cil_insert_format_items(
>  		/*
>  		 * Skip items that do not have any vectors for writing. These
>  		 * log items must now be considered clean in this transaction,
> -		 * so clear the XFS_LID_DIRTY flag to ensure it doesn't get
> +		 * so clear the XFS_LI_DIRTY flag to ensure it doesn't get
>  		 * added to the CIL by mistake.
>  		 */
>  		if (!shadow->lv_niovecs && !ordered) {
> -			lidp->lid_flags &= ~XFS_LID_DIRTY;
> +			clear_bit(XFS_LI_DIRTY, &lip->li_flags);

So this requires "xfs: handle inconsistent log item formatting state
correctly" as a prerequisite...

>  			continue;
>  		}
>  
> @@ -413,7 +411,7 @@ xlog_cil_insert_items(
>  {
>  	struct xfs_cil		*cil = log->l_cilp;
>  	struct xfs_cil_ctx	*ctx = cil->xc_ctx;
> -	struct xfs_log_item_desc *lidp;
> +	struct xfs_log_item	*lip;
>  	int			len = 0;
>  	int			diff_iovecs = 0;
>  	int			iclog_space;
> @@ -486,11 +484,10 @@ xlog_cil_insert_items(
>  	 * We do this here so we only need to take the CIL lock once during
>  	 * the transaction commit.
>  	 */
> -	list_for_each_entry(lidp, &tp->t_items, lid_trans) {
> -		struct xfs_log_item	*lip = lidp->lid_item;
> +	list_for_each_entry(lip, &tp->t_items, li_trans) {
>  
>  		/* Skip items which aren't dirty in this transaction. */
> -		if (!(lidp->lid_flags & XFS_LID_DIRTY))
> +		if (!test_bit(XFS_LI_DIRTY, &lip->li_flags))
>  			continue;
>  
>  		/*
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index d71424052917..5726ef496980 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1880,11 +1880,6 @@ xfs_init_zones(void)
>  	if (!xfs_trans_zone)
>  		goto out_destroy_ifork_zone;
>  
> -	xfs_log_item_desc_zone =
> -		kmem_zone_init(sizeof(struct xfs_log_item_desc),
> -			       "xfs_log_item_desc");
> -	if (!xfs_log_item_desc_zone)
> -		goto out_destroy_trans_zone;
>  
>  	/*
>  	 * The size of the zone allocated buf log item is the maximum
> @@ -1894,7 +1889,7 @@ xfs_init_zones(void)
>  	xfs_buf_item_zone = kmem_zone_init(sizeof(struct xfs_buf_log_item),
>  					   "xfs_buf_item");
>  	if (!xfs_buf_item_zone)
> -		goto out_destroy_log_item_desc_zone;
> +		goto out_destroy_trans_zone;
>  
>  	xfs_efd_zone = kmem_zone_init((sizeof(xfs_efd_log_item_t) +
>  			((XFS_EFD_MAX_FAST_EXTENTS - 1) *
> @@ -1982,8 +1977,6 @@ xfs_init_zones(void)
>  	kmem_zone_destroy(xfs_efd_zone);
>   out_destroy_buf_item_zone:
>  	kmem_zone_destroy(xfs_buf_item_zone);
> - out_destroy_log_item_desc_zone:
> -	kmem_zone_destroy(xfs_log_item_desc_zone);
>   out_destroy_trans_zone:
>  	kmem_zone_destroy(xfs_trans_zone);
>   out_destroy_ifork_zone:
> @@ -2022,7 +2015,6 @@ xfs_destroy_zones(void)
>  	kmem_zone_destroy(xfs_efi_zone);
>  	kmem_zone_destroy(xfs_efd_zone);
>  	kmem_zone_destroy(xfs_buf_item_zone);
> -	kmem_zone_destroy(xfs_log_item_desc_zone);
>  	kmem_zone_destroy(xfs_trans_zone);
>  	kmem_zone_destroy(xfs_ifork_zone);
>  	kmem_zone_destroy(xfs_da_state_zone);
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 947ab6e32c18..28a5b3f876ad 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -441,7 +441,6 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
>  		__field(unsigned, bli_recur)
>  		__field(int, bli_refcount)
>  		__field(unsigned, bli_flags)
> -		__field(void *, li_desc)
>  		__field(unsigned long, li_flags)
>  	),
>  	TP_fast_assign(
> @@ -455,12 +454,11 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
>  		__entry->buf_hold = atomic_read(&bip->bli_buf->b_hold);
>  		__entry->buf_pincount = atomic_read(&bip->bli_buf->b_pin_count);
>  		__entry->buf_lockval = bip->bli_buf->b_sema.count;
> -		__entry->li_desc = bip->bli_item.li_desc;
>  		__entry->li_flags = bip->bli_item.li_flags;
>  	),
>  	TP_printk("dev %d:%d bno 0x%llx len 0x%zx hold %d pincount %d "
>  		  "lock %d flags %s recur %d refcount %d bliflags %s "
> -		  "lidesc %p liflags %s",
> +		  "liflags %s",
>  		  MAJOR(__entry->dev), MINOR(__entry->dev),
>  		  (unsigned long long)__entry->buf_bno,
>  		  __entry->buf_len,
> @@ -471,7 +469,6 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
>  		  __entry->bli_recur,
>  		  __entry->bli_refcount,
>  		  __print_flags(__entry->bli_flags, "|", XFS_BLI_FLAGS),
> -		  __entry->li_desc,
>  		  __print_flags(__entry->li_flags, "|", XFS_LI_FLAGS))
>  )
>  
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 63c22aa02cd8..8d83efbc01c3 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -33,7 +33,6 @@
>  #include "xfs_error.h"
>  
>  kmem_zone_t	*xfs_trans_zone;
> -kmem_zone_t	*xfs_log_item_desc_zone;
>  
>  #if defined(CONFIG_TRACEPOINTS)
>  static void
> @@ -734,70 +733,49 @@ xfs_trans_unreserve_and_mod_sb(
>  
>  /*
>   * Add the given log item to the transaction's list of log items.
> - *
> - * The log item will now point to its new descriptor with its li_desc field.
>   */
>  void
>  xfs_trans_add_item(
>  	struct xfs_trans	*tp,
>  	struct xfs_log_item	*lip)
>  {
> -	struct xfs_log_item_desc *lidp;
> -
>  	ASSERT(lip->li_mountp == tp->t_mountp);
>  	ASSERT(lip->li_ailp == tp->t_mountp->m_ail);
>  	ASSERT(!test_bit(XFS_LI_TRANS, &lip->li_flags));
> +	ASSERT(!test_bit(XFS_LI_DIRTY, &lip->li_flags));
>  
> -	lidp = kmem_zone_zalloc(xfs_log_item_desc_zone, KM_SLEEP | KM_NOFS);
> -
> -	lidp->lid_item = lip;
> -	lidp->lid_flags = 0;
> -	list_add_tail(&lidp->lid_trans, &tp->t_items);
> -
> -	lip->li_desc = lidp;
> +	list_add_tail(&lip->li_trans, &tp->t_items);
>  	set_bit(XFS_LI_TRANS, &lip->li_flags);
>  	trace_xfs_trans_add_item(tp, _RET_IP_);
>  }
>  
> -STATIC void
> -xfs_trans_free_item_desc(
> -	struct xfs_log_item_desc *lidp)
> -{
> -	list_del_init(&lidp->lid_trans);
> -	kmem_zone_free(xfs_log_item_desc_zone, lidp);
> -}
> -
>  /*
> - * Unlink and free the given descriptor.
> + * Unlink the log item from the transaction. The log item is no longer
> + * considered dirty in this transaction, as the linked transaction has
> + * finished, either by abort or commit completion.
>   */
>  void
>  xfs_trans_del_item(
>  	struct xfs_log_item	*lip)
>  {
>  	clear_bit(XFS_LI_TRANS, &lip->li_flags);
> -	xfs_trans_free_item_desc(lip->li_desc);
> -	lip->li_desc = NULL;
> +	clear_bit(XFS_LI_DIRTY, &lip->li_flags);
> +	list_del_init(&lip->li_trans);
>  }
>  
> -/*
> - * Unlock all of the items of a transaction and free all the descriptors
> - * of that transaction.
> - */
> +/* Detach and unlock all of the items in a transaction */
>  void
>  xfs_trans_free_items(
>  	struct xfs_trans	*tp,
>  	xfs_lsn_t		commit_lsn,
>  	bool			abort)
>  {
> -	struct xfs_log_item_desc *lidp, *next;
> +	struct xfs_log_item	*lip, *next;
>  
>  	trace_xfs_trans_free_items(tp, _RET_IP_);
>  
> -	list_for_each_entry_safe(lidp, next, &tp->t_items, lid_trans) {
> -		struct xfs_log_item	*lip = lidp->lid_item;
> -
> +	list_for_each_entry_safe(lip, next, &tp->t_items, li_trans) {
>  		xfs_trans_del_item(lip);
> -
>  		if (commit_lsn != NULLCOMMITLSN)
>  			lip->li_ops->iop_committing(lip, commit_lsn);
>  		if (abort)
> @@ -1048,10 +1026,10 @@ xfs_trans_cancel(
>  	}
>  #ifdef DEBUG
>  	if (!dirty && !XFS_FORCED_SHUTDOWN(mp)) {
> -		struct xfs_log_item_desc *lidp;
> +		struct xfs_log_item *lip;
>  
> -		list_for_each_entry(lidp, &tp->t_items, lid_trans)
> -			ASSERT(!(lidp->lid_item->li_type == XFS_LI_EFD));
> +		list_for_each_entry(lip, &tp->t_items, li_trans)
> +			ASSERT(!(lip->li_type == XFS_LI_EFD));
>  	}
>  #endif
>  	xfs_trans_unreserve_and_mod_sb(tp);
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index af52107adffc..05b1e1d0a318 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -27,7 +27,6 @@ struct xfs_efi_log_item;
>  struct xfs_inode;
>  struct xfs_item_ops;
>  struct xfs_log_iovec;
> -struct xfs_log_item_desc;
>  struct xfs_mount;
>  struct xfs_trans;
>  struct xfs_trans_res;
> @@ -43,8 +42,8 @@ struct xfs_bud_log_item;
>  
>  typedef struct xfs_log_item {
>  	struct list_head		li_ail;		/* AIL pointers */
> +	struct list_head		li_trans;	/* transaction list */
>  	xfs_lsn_t			li_lsn;		/* last on-disk lsn */
> -	struct xfs_log_item_desc	*li_desc;	/* ptr to current desc*/
>  	struct xfs_mount		*li_mountp;	/* ptr to fs mount */
>  	struct xfs_ail			*li_ailp;	/* ptr to AIL */
>  	uint				li_type;	/* item type */
> @@ -73,12 +72,14 @@ typedef struct xfs_log_item {
>  #define	XFS_LI_ABORTED	1
>  #define	XFS_LI_FAILED	2
>  #define	XFS_LI_TRANS	3	/* attached to an active transaction */
> +#define	XFS_LI_DIRTY	4	/* log item dirty in transaction */
>  
>  #define XFS_LI_FLAGS \
>  	{ (1 << XFS_LI_IN_AIL),		"IN_AIL" }, \
>  	{ (1 << XFS_LI_ABORTED),	"ABORTED" }, \
>  	{ (1 << XFS_LI_FAILED),		"FAILED" }, \
> -	{ (1 << XFS_LI_TRANS),		"TRANS" }
> +	{ (1 << XFS_LI_TRANS),		"TRANS" }, \
> +	{ (1 << XFS_LI_DIRTY),		"DIRTY" }
>  
>  struct xfs_item_ops {
>  	void (*iop_size)(xfs_log_item_t *, int *, int *);
> @@ -249,7 +250,6 @@ void		xfs_trans_buf_copy_type(struct xfs_buf *dst_bp,
>  					struct xfs_buf *src_bp);
>  
>  extern kmem_zone_t	*xfs_trans_zone;
> -extern kmem_zone_t	*xfs_log_item_desc_zone;
>  
>  /* rmap updates */
>  enum xfs_rmap_intent_type;
> diff --git a/fs/xfs/xfs_trans_bmap.c b/fs/xfs/xfs_trans_bmap.c
> index 14543d93cd4b..230a21df4b12 100644
> --- a/fs/xfs/xfs_trans_bmap.c
> +++ b/fs/xfs/xfs_trans_bmap.c
> @@ -79,7 +79,7 @@ xfs_trans_log_finish_bmap_update(
>  	 * 2.) shuts down the filesystem
>  	 */
>  	tp->t_flags |= XFS_TRANS_DIRTY;
> -	budp->bud_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> +	set_bit(XFS_LI_DIRTY, &budp->bud_item.li_flags);
>  
>  	return error;
>  }
> @@ -158,7 +158,7 @@ xfs_bmap_update_log_item(
>  	bmap = container_of(item, struct xfs_bmap_intent, bi_list);
>  
>  	tp->t_flags |= XFS_TRANS_DIRTY;
> -	buip->bui_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> +	set_bit(XFS_LI_DIRTY, &buip->bui_item.li_flags);
>  
>  	/*
>  	 * atomic_inc_return gives us the value after the increment;
> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> index 88557dfea21f..3e9e2f6d8215 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -40,7 +40,7 @@ xfs_trans_buf_item_match(
>  	struct xfs_buf_map	*map,
>  	int			nmaps)
>  {
> -	struct xfs_log_item_desc *lidp;
> +	struct xfs_log_item	*lip;
>  	struct xfs_buf_log_item	*blip;
>  	int			len = 0;
>  	int			i;
> @@ -48,8 +48,8 @@ xfs_trans_buf_item_match(
>  	for (i = 0; i < nmaps; i++)
>  		len += map[i].bm_len;
>  
> -	list_for_each_entry(lidp, &tp->t_items, lid_trans) {
> -		blip = (struct xfs_buf_log_item *)lidp->lid_item;
> +	list_for_each_entry(lip, &tp->t_items, li_trans) {
> +		blip = (struct xfs_buf_log_item *)lip;
>  		if (blip->bli_item.li_type == XFS_LI_BUF &&
>  		    blip->bli_buf->b_target == target &&
>  		    XFS_BUF_ADDR(blip->bli_buf) == map[0].bm_bn &&
> @@ -100,14 +100,10 @@ _xfs_trans_bjoin(
>  	atomic_inc(&bip->bli_refcount);
>  
>  	/*
> -	 * Get a log_item_desc to point at the new item.
> +	 * Attach the item to the transaction so we can find it in
> +	 * xfs_trans_get_buf() and friends.
>  	 */
>  	xfs_trans_add_item(tp, &bip->bli_item);
> -
> -	/*
> -	 * Initialize b_fsprivate2 so we can find it with incore_match()
> -	 * in xfs_trans_get_buf() and friends above.
> -	 */
>  	bp->b_transp = tp;
>  
>  }
> @@ -391,7 +387,7 @@ xfs_trans_brelse(
>  	 * If the buffer is dirty within this transaction, we can't
>  	 * release it until we commit.
>  	 */
> -	if (bip->bli_item.li_desc->lid_flags & XFS_LID_DIRTY)
> +	if (test_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags))
>  		return;
>  
>  	/*
> @@ -542,7 +538,7 @@ xfs_trans_dirty_buf(
>  	bip->bli_flags |= XFS_BLI_DIRTY | XFS_BLI_LOGGED;
>  
>  	tp->t_flags |= XFS_TRANS_DIRTY;
> -	bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> +	set_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags);
>  }
>  
>  /*
> @@ -623,7 +619,7 @@ xfs_trans_binval(
>  		ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_INODE_BUF));
>  		ASSERT(!(bip->__bli_format.blf_flags & XFS_BLFT_MASK));
>  		ASSERT(bip->__bli_format.blf_flags & XFS_BLF_CANCEL);
> -		ASSERT(bip->bli_item.li_desc->lid_flags & XFS_LID_DIRTY);
> +		ASSERT(test_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags));
>  		ASSERT(tp->t_flags & XFS_TRANS_DIRTY);
>  		return;
>  	}
> @@ -633,7 +629,7 @@ xfs_trans_binval(
>  	bip->bli_flags |= XFS_BLI_STALE;
>  
>  	/* Mark the log item as dirty. */
> -	bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> +	set_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags);
>  	tp->t_flags |= XFS_TRANS_DIRTY;

...but even with that patch, this hunk doesn't apply.  I've never seen
the comment in any patch.  What are the prerequisites for this series?

--D

>  
>  	/*
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index c3d547211d16..c381c02cca45 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -77,7 +77,7 @@ xfs_trans_log_dquot(
>  	ASSERT(XFS_DQ_IS_LOCKED(dqp));
>  
>  	tp->t_flags |= XFS_TRANS_DIRTY;
> -	dqp->q_logitem.qli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> +	set_bit(XFS_LI_DIRTY, &dqp->q_logitem.qli_item.li_flags);
>  }
>  
>  /*
> @@ -879,7 +879,7 @@ xfs_trans_log_quotaoff_item(
>  	xfs_qoff_logitem_t	*qlp)
>  {
>  	tp->t_flags |= XFS_TRANS_DIRTY;
> -	qlp->qql_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> +	set_bit(XFS_LI_DIRTY, &qlp->qql_item.li_flags);
>  }
>  
>  STATIC void
> diff --git a/fs/xfs/xfs_trans_extfree.c b/fs/xfs/xfs_trans_extfree.c
> index ab438647592a..fb631081d829 100644
> --- a/fs/xfs/xfs_trans_extfree.c
> +++ b/fs/xfs/xfs_trans_extfree.c
> @@ -90,7 +90,7 @@ xfs_trans_free_extent(
>  	 * 2.) shuts down the filesystem
>  	 */
>  	tp->t_flags |= XFS_TRANS_DIRTY;
> -	efdp->efd_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> +	set_bit(XFS_LI_DIRTY, &efdp->efd_item.li_flags);
>  
>  	next_extent = efdp->efd_next_extent;
>  	ASSERT(next_extent < efdp->efd_format.efd_nextents);
> @@ -155,7 +155,7 @@ xfs_extent_free_log_item(
>  	free = container_of(item, struct xfs_extent_free_item, xefi_list);
>  
>  	tp->t_flags |= XFS_TRANS_DIRTY;
> -	efip->efi_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> +	set_bit(XFS_LI_DIRTY, &efip->efi_item.li_flags);
>  
>  	/*
>  	 * atomic_inc_return gives us the value after the increment;
> diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
> index 07cea592dc01..f7bd7960a90f 100644
> --- a/fs/xfs/xfs_trans_inode.c
> +++ b/fs/xfs/xfs_trans_inode.c
> @@ -133,14 +133,13 @@ xfs_trans_log_inode(
>  	 * set however, then go ahead and bump the i_version counter
>  	 * unconditionally.
>  	 */
> -	if (!(ip->i_itemp->ili_item.li_desc->lid_flags & XFS_LID_DIRTY) &&
> +	if (!test_and_set_bit(XFS_LI_DIRTY, &ip->i_itemp->ili_item.li_flags) &&
>  	    IS_I_VERSION(VFS_I(ip))) {
>  		if (inode_maybe_inc_iversion(VFS_I(ip), flags & XFS_ILOG_CORE))
>  			flags |= XFS_ILOG_CORE;
>  	}
>  
>  	tp->t_flags |= XFS_TRANS_DIRTY;
> -	ip->i_itemp->ili_item.li_desc->lid_flags |= XFS_LID_DIRTY;
>  
>  	/*
>  	 * Always OR in the bits from the ili_last_fields field.
> diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
> index 43f773297b9d..9717ae74b36d 100644
> --- a/fs/xfs/xfs_trans_priv.h
> +++ b/fs/xfs/xfs_trans_priv.h
> @@ -19,7 +19,6 @@
>  #define	__XFS_TRANS_PRIV_H__
>  
>  struct xfs_log_item;
> -struct xfs_log_item_desc;
>  struct xfs_mount;
>  struct xfs_trans;
>  struct xfs_ail;
> diff --git a/fs/xfs/xfs_trans_refcount.c b/fs/xfs/xfs_trans_refcount.c
> index 94c1877af834..c7f8e82f5bda 100644
> --- a/fs/xfs/xfs_trans_refcount.c
> +++ b/fs/xfs/xfs_trans_refcount.c
> @@ -77,7 +77,7 @@ xfs_trans_log_finish_refcount_update(
>  	 * 2.) shuts down the filesystem
>  	 */
>  	tp->t_flags |= XFS_TRANS_DIRTY;
> -	cudp->cud_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> +	set_bit(XFS_LI_DIRTY, &cudp->cud_item.li_flags);
>  
>  	return error;
>  }
> @@ -154,7 +154,7 @@ xfs_refcount_update_log_item(
>  	refc = container_of(item, struct xfs_refcount_intent, ri_list);
>  
>  	tp->t_flags |= XFS_TRANS_DIRTY;
> -	cuip->cui_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> +	set_bit(XFS_LI_DIRTY, &cuip->cui_item.li_flags);
>  
>  	/*
>  	 * atomic_inc_return gives us the value after the increment;
> diff --git a/fs/xfs/xfs_trans_rmap.c b/fs/xfs/xfs_trans_rmap.c
> index 9b577beb43d7..5831ca0c270b 100644
> --- a/fs/xfs/xfs_trans_rmap.c
> +++ b/fs/xfs/xfs_trans_rmap.c
> @@ -117,7 +117,7 @@ xfs_trans_log_finish_rmap_update(
>  	 * 2.) shuts down the filesystem
>  	 */
>  	tp->t_flags |= XFS_TRANS_DIRTY;
> -	rudp->rud_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> +	set_bit(XFS_LI_DIRTY, &rudp->rud_item.li_flags);
>  
>  	return error;
>  }
> @@ -175,7 +175,7 @@ xfs_rmap_update_log_item(
>  	rmap = container_of(item, struct xfs_rmap_intent, ri_list);
>  
>  	tp->t_flags |= XFS_TRANS_DIRTY;
> -	ruip->rui_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> +	set_bit(XFS_LI_DIRTY, &ruip->rui_item.li_flags);
>  
>  	/*
>  	 * atomic_inc_return gives us the value after the increment;
> -- 
> 2.17.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/10] xfs: get rid of the log item descriptor

[Dave Chinner]

On Wed, May 02, 2018 at 01:05:10PM -0700, Darrick J. Wong wrote:
> On Wed, May 02, 2018 at 06:01:57PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > It's just a connector between a transaction and a log item. There's
> > a 1:1 relationship between a log item descriptor and a log item,
> > and a 1:1 relationship between a log item descriptor and a
> > transaction. Both relationships are created and terminated at the
> > same time, so why do we even have the descriptor?
> > 
> > Replace it with a specific list_head in the log item and a new
> > log item dirtied flag to replace the XFS_LID_DIRTY flag.
> > 
> > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
.....
> >  		/*
> >  		 * Skip items that do not have any vectors for writing. These
> >  		 * log items must now be considered clean in this transaction,
> > -		 * so clear the XFS_LID_DIRTY flag to ensure it doesn't get
> > +		 * so clear the XFS_LI_DIRTY flag to ensure it doesn't get
> >  		 * added to the CIL by mistake.
> >  		 */
> >  		if (!shadow->lv_niovecs && !ordered) {
> > -			lidp->lid_flags &= ~XFS_LID_DIRTY;
> > +			clear_bit(XFS_LI_DIRTY, &lip->li_flags);
> 
> So this requires "xfs: handle inconsistent log item formatting state
> correctly" as a prerequisite...

Ah, forgot that was sitting high up in the patch stack. It's not
dependent on it - and I can't remember exactly what the state of
that patch was.

...
> > @@ -633,7 +629,7 @@ xfs_trans_binval(
> >  	bip->bli_flags |= XFS_BLI_STALE;
> >  
> >  	/* Mark the log item as dirty. */
> > -	bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> > +	set_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags);
> >  	tp->t_flags |= XFS_TRANS_DIRTY;
> 
> ...but even with that patch, this hunk doesn't apply.  I've never seen
> the comment in any patch.  What are the prerequisites for this series?

Ah, I also have the buffer range logging patch in the series up near
the top with the other patch. Again, not dependent, just got so many
stalled patches in my tree that have been sitting around for ages
that I've forgotten where there are dependencies. 

I'll have to do some more reordering...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 10/10] xfs: get rid of the log item descriptor

[Darrick J. Wong]

On Thu, May 03, 2018 at 07:53:43AM +1000, Dave Chinner wrote:
> On Wed, May 02, 2018 at 01:05:10PM -0700, Darrick J. Wong wrote:
> > On Wed, May 02, 2018 at 06:01:57PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > It's just a connector between a transaction and a log item. There's
> > > a 1:1 relationship between a log item descriptor and a log item,
> > > and a 1:1 relationship between a log item descriptor and a
> > > transaction. Both relationships are created and terminated at the
> > > same time, so why do we even have the descriptor?
> > > 
> > > Replace it with a specific list_head in the log item and a new
> > > log item dirtied flag to replace the XFS_LID_DIRTY flag.
> > > 
> > > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> .....
> > >  		/*
> > >  		 * Skip items that do not have any vectors for writing. These
> > >  		 * log items must now be considered clean in this transaction,
> > > -		 * so clear the XFS_LID_DIRTY flag to ensure it doesn't get
> > > +		 * so clear the XFS_LI_DIRTY flag to ensure it doesn't get
> > >  		 * added to the CIL by mistake.
> > >  		 */
> > >  		if (!shadow->lv_niovecs && !ordered) {
> > > -			lidp->lid_flags &= ~XFS_LID_DIRTY;
> > > +			clear_bit(XFS_LI_DIRTY, &lip->li_flags);
> > 
> > So this requires "xfs: handle inconsistent log item formatting state
> > correctly" as a prerequisite...
> 
> Ah, forgot that was sitting high up in the patch stack. It's not
> dependent on it - and I can't remember exactly what the state of
> that patch was.
> 
> ...
> > > @@ -633,7 +629,7 @@ xfs_trans_binval(
> > >  	bip->bli_flags |= XFS_BLI_STALE;
> > >  
> > >  	/* Mark the log item as dirty. */
> > > -	bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> > > +	set_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags);
> > >  	tp->t_flags |= XFS_TRANS_DIRTY;
> > 
> > ...but even with that patch, this hunk doesn't apply.  I've never seen
> > the comment in any patch.  What are the prerequisites for this series?
> 
> Ah, I also have the buffer range logging patch in the series up near
> the top with the other patch. Again, not dependent, just got so many
> stalled patches in my tree that have been sitting around for ages
> that I've forgotten where there are dependencies. 
> 
> I'll have to do some more reordering...

Ok.  The series looks ok so far at a quick glance.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/10] xfs: get rid of the log item descriptor

[Christoph Hellwig]

Looks sensible.  Note that the LI_TRANS flag added earlier is now
redundant.