xfs: log item and transaction cleanups

From: Dave Chinner <[email protected]>

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 <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Reviewed-by Brian Foster <[email protected]>

---
 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(-)

fs/xfs/xfs_bmap_item.c +2 -2

index 2203465e63ea..618bb71535c8 100644
@@ -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);
 	}

fs/xfs/xfs_buf_item.c +3 -1

index 82ad270e390e..df62082f2204 100644
@@ -568,13 +568,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;
 

fs/xfs/xfs_dquot.c +3 -4

index a7daef9e16bf..d0b65679baae 100644
@@ -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);
 		}
 	}

fs/xfs/xfs_dquot_item.c +1 -1

index 4b331e354da7..57df98122156 100644
@@ -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;
 

fs/xfs/xfs_extfree_item.c +2 -2

index b5b1e567b9f4..70b7d48af6d6 100644
@@ -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);
 	}

fs/xfs/xfs_icache.c +2 -1

index 9a18f69f6e96..98b7a4ae15e4 100644
@@ -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;
 	}

fs/xfs/xfs_icreate_item.c +1 -1

index 865ad1373e5e..a99a0f8aa528 100644
@@ -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;
 }

fs/xfs/xfs_inode.c +2 -2

index 2b70c8b4cee2..16a43ba884cc 100644
@@ -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)

fs/xfs/xfs_inode_item.c +4 -4

index 34b91b789702..3e5b8574818e 100644
@@ -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);

fs/xfs/xfs_log.c +1 -1

index 2fcd9ed5d075..e427864434c1 100644
@@ -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);

fs/xfs/xfs_qm.c +1 -1

index ec39ae274c78..4438fc446d18 100644
@@ -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);

fs/xfs/xfs_refcount_item.c +2 -2

index 15c9393dd7a7..e5866b714d5f 100644
@@ -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);
 	}

fs/xfs/xfs_rmap_item.c +2 -2

index 06a07846c9b3..e5b5b3e7ef82 100644
@@ -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);
 	}

fs/xfs/xfs_trace.h +3 -3

index 8955254b900e..7f4d961fae12 100644
@@ -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)
 	),

fs/xfs/xfs_trans.c +2 -2

index d6d8f9d129a7..c6a2a3cb9df5 100644
@@ -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 */

fs/xfs/xfs_trans.h +12 -7

index 9d542dfe0052..51145ddf7e5b 100644
@@ -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 *);

fs/xfs/xfs_trans_ail.c +4 -5

index d4a2445215e6..50611d2bcbc2 100644
@@ -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,

fs/xfs/xfs_trans_buf.c +1 -1

index a5d9dfc45d98..0081e9b3decf 100644
@@ -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);
 	}

fs/xfs/xfs_trans_priv.h +4 -6

index be24b0c8a332..43f773297b9d 100644
@@ -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;
 	}
 }

Signature · 6 lines

2.17.0

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

From: Dave Chinner <[email protected]>

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 <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Reviewed-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_log_cil.c |  1 +
 fs/xfs/xfs_trace.h   | 37 +++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_trans.c   | 20 +++++++++++++++++++-
 3 files changed, 57 insertions(+), 1 deletion(-)

fs/xfs/xfs_log_cil.c +1 -0

index 4668403b1741..a8675c631438 100644
@@ -1013,6 +1013,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);
 
 	/*

fs/xfs/xfs_trace.h +37 -0

index 7f4d961fae12..8136f2280173 100644
@@ -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

fs/xfs/xfs_trans.c +19 -1

index c6a2a3cb9df5..24744915357b 100644
@@ -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;
 }

@@ -749,6 +754,8 @@ xfs_trans_add_item(
 	list_add_tail(&lidp->lid_trans, &tp->t_items);
 
 	lip->li_desc = lidp;
+
+	trace_xfs_trans_add_item(tp, _RET_IP_);
 }
 
 STATIC void

@@ -782,6 +789,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;
 

@@ -936,6 +945,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

@@ -991,6 +1002,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);

@@ -1022,6 +1034,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

@@ -1042,8 +1056,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);

@@ -1067,6 +1083,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.
 	 */

Signature · 6 lines

2.17.0

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

From: Dave Chinner <[email protected]>

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

Signed-off-by Dave Chinner <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Reviewed-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_defer.c | 12 ++++++------
 fs/xfs/xfs_trace.h        | 17 +++++++++++------
 2 files changed, 17 insertions(+), 12 deletions(-)

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

index 087fea02c389..51821af5d653 100644
@@ -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_);
 }

fs/xfs/xfs_trace.h +11 -6

index 8136f2280173..947ab6e32c18 100644
@@ -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),

Signature · 6 lines

2.17.0

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

From: Dave Chinner <[email protected]>

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. Thanks to Brian Foster for supplying a clean way of doing this.

Signed-off-by Dave Chinner <[email protected]>

---

 fs/xfs/xfs_trans_ail.c | 43 +++++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 11 deletions(-)

fs/xfs/xfs_trans_ail.c +32 -11

index 50611d2bcbc2..41e280ef1483 100644
@@ -32,30 +32,51 @@
 #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 we sample and check the state under the AIL lock and return if
+ * everything is fine, otherwise we drop the lock and run the ASSERT checks.
+ * Asserts may not be fatal, so pick the 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;
+	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;
 
 	/*
-	 * Check the next and previous entries are valid.
+	 * Sample then 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);
+	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)
-		ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0);
-
-	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_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;
 
+	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)

Signature · 6 lines

2.17.0

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

From: Dave Chinner <[email protected]>

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. Thanks to Brian Foster for supplying a clean way of doing this.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_trans_ail.c | 43 +++++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 11 deletions(-)

fs/xfs/xfs_trans_ail.c +32 -11

index 50611d2bcbc2..41e280ef1483 100644
@@ -32,30 +32,51 @@
 #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 we sample and check the state under the AIL lock and return if
+ * everything is fine, otherwise we drop the lock and run the ASSERT checks.
+ * Asserts may not be fatal, so pick the 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;
+	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;
 
 	/*
-	 * Check the next and previous entries are valid.
+	 * Sample then 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);
+	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)
-		ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0);
-
-	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_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;
 
+	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)

Signature · 6 lines

2.17.0

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

From: Dave Chinner <[email protected]>

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 <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_symlink.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

fs/xfs/xfs_symlink.c +0 -2

index 5b66ac12913c..27870e5cd259 100644
@@ -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.

fs/xfs/xfs_symlink.c +0 -0

 	error = xfs_trans_commit(tp);
 	if (error) {
 		ASSERT(XFS_FORCED_SHUTDOWN(mp));

Signature · 6 lines

2.17.0

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

Quoted 38 lines · Dave Chinner

From: Dave Chinner <[email protected]>

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 <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_symlink.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

fs/xfs/xfs_symlink.c +0 -7

index 5b66ac12913c..27870e5cd259 100644
@@ -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);

fs/xfs/xfs_symlink.c xfs_inactive_symlink_rmt( · line 495

Quoted 5 lines

fs/xfs/xfs_symlink.c +2 -0

+
 	/*
 	 * Commit the transaction containing extent freeing and EFDs.
 	 */
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

Seems fine.. but do we even need this call? We're about to commit the transaction and unlock the inode...

Quoted 39 lines · Brian Foster

Quoted 38 lines · Dave Chinner

From: Dave Chinner <[email protected]>

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 <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_symlink.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

fs/xfs/xfs_symlink.c +0 -7

index 5b66ac12913c..27870e5cd259 100644
@@ -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);

Quoted 8 lines

fs/xfs/xfs_symlink.c xfs_inactive_symlink_rmt( · line 495

Quoted 5 lines

fs/xfs/xfs_symlink.c +2 -0

+
 	/*
 	 * Commit the transaction containing extent freeing and EFDs.
 	 */
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

Seems fine.. but do we even need this call? We're about to commit the transaction and unlock the inode...

Yes, I think we do. We need it to be committed in each of the rolling transactions so that the inode doesn't get written/replayed before any of the other dependent metadata changes in this final transaction.

Cheers,

Dave.

Signature · 6 lines

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

Quoted 49 lines · Dave Chinner

Quoted 39 lines · Brian Foster

Quoted 38 lines · Dave Chinner

From: Dave Chinner <[email protected]>

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 <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_symlink.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

fs/xfs/xfs_symlink.c +0 -7

index 5b66ac12913c..27870e5cd259 100644
@@ -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);

Quoted 8 lines

fs/xfs/xfs_symlink.c xfs_inactive_symlink_rmt( · line 495

Quoted 5 lines

fs/xfs/xfs_symlink.c +2 -0

+
 	/*
 	 * Commit the transaction containing extent freeing and EFDs.
 	 */
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

Seems fine.. but do we even need this call? We're about to commit the transaction and unlock the inode...

Quoted 5 lines

Yes, I think we do. We need it to be committed in each of the rolling transactions so that the inode doesn't get written/replayed before any of the other dependent metadata changes in this final transaction.

Hmm, I don't follow what that means. IIUC the act of logging it again simply moves it forward in the log. That makes sense down in the dfops code but seems unecessary here given that we are about to complete the chain of transactions.

xfs_inactive_symlink_rmt() makes changes to the inode, invals/unmaps the remote bufs, joins the inode to the dfops and finishes the dfops. We return from xfs_defer_finish() having committed the (still locked) inode modifications and have a new/rolled transaction that covers the free of the associated blocks (EFDs). I could certainly be missing something, but from that point what difference does it make whether the final transaction relogs the inode before it commits? The in-core inode is still locked and the previous inode modifications may very well already be in the on-disk log.

That said, it seems harmless despite not understanding what it's for. So with or without the xfs_trans_log_inode() call:

Reviewed-by Brian Foster <[email protected]>

Quoted 11 lines

Cheers,

Dave.

Signature · 6 lines

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

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

Quoted 61 lines · Brian Foster

Quoted 49 lines · Dave Chinner

Quoted 39 lines · Brian Foster

Quoted 38 lines · Dave Chinner

From: Dave Chinner <[email protected]>

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 <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_symlink.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

fs/xfs/xfs_symlink.c +0 -7

index 5b66ac12913c..27870e5cd259 100644
@@ -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);

Quoted 8 lines

fs/xfs/xfs_symlink.c xfs_inactive_symlink_rmt( · line 495

Quoted 5 lines

fs/xfs/xfs_symlink.c +2 -0

+
 	/*
 	 * Commit the transaction containing extent freeing and EFDs.
 	 */
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

Seems fine.. but do we even need this call? We're about to commit the transaction and unlock the inode...

Quoted 5 lines

Yes, I think we do. We need it to be committed in each of the rolling transactions so that the inode doesn't get written/replayed before any of the other dependent metadata changes in this final transaction.

Hmm, I don't follow what that means. IIUC the act of logging it again simply moves it forward in the log. That makes sense down in the dfops code but seems unecessary here given that we are about to complete the chain of transactions.

Quoted 7 lines

xfs_inactive_symlink_rmt() makes changes to the inode, invals/unmaps the remote bufs, joins the inode to the dfops and finishes the dfops. We return from xfs_defer_finish() having committed the (still locked) inode modifications and have a new/rolled transaction that covers the free of the associated blocks (EFDs). I could certainly be missing something, but from that point what difference does it make whether the final transaction relogs the inode before it commits?

Quoted 6 lines

The in-core inode is
still locked and the previous inode modifications may very well already be in the on-disk log.

That said, it seems harmless despite not understanding what it's for. So with or without the xfs_trans_log_inode() call:

<shrug> I interpreted this as Dave being fastidious about relogging inodes after every transaction roll even if it's not strictly necessary (but not otherwise harmful)

Quoted 62 lines · Darrick J. Wong

Quoted 61 lines · Brian Foster

Quoted 49 lines · Dave Chinner

Quoted 39 lines · Brian Foster

Quoted 38 lines · Dave Chinner

From: Dave Chinner <[email protected]>

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 <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_symlink.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

fs/xfs/xfs_symlink.c +0 -7

index 5b66ac12913c..27870e5cd259 100644
@@ -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);

Quoted 8 lines

fs/xfs/xfs_symlink.c xfs_inactive_symlink_rmt( · line 495

Quoted 5 lines

fs/xfs/xfs_symlink.c +2 -0

+
 	/*
 	 * Commit the transaction containing extent freeing and EFDs.
 	 */
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

Seems fine.. but do we even need this call? We're about to commit the transaction and unlock the inode...

Quoted 5 lines

Yes, I think we do. We need it to be committed in each of the rolling transactions so that the inode doesn't get written/replayed before any of the other dependent metadata changes in this final transaction.

Hmm, I don't follow what that means. IIUC the act of logging it again simply moves it forward in the log. That makes sense down in the dfops code but seems unecessary here given that we are about to complete the chain of transactions.

Quoted 7 lines

Quoted 7 lines

xfs_inactive_symlink_rmt() makes changes to the inode, invals/unmaps the remote bufs, joins the inode to the dfops and finishes the dfops. We return from xfs_defer_finish() having committed the (still locked) inode modifications and have a new/rolled transaction that covers the free of the associated blocks (EFDs). I could certainly be missing something, but from that point what difference does it make whether the final transaction relogs the inode before it commits?

It ensures the inode changes are sanely ordered. i.e. we don't write the inode to disk before all the other changes made in the rolling transaction are written to disk. And the same goes for recovery.

Quoted 10 lines

Quoted 6 lines

The in-core inode is
still locked and the previous inode modifications may very well already be in the on-disk log.

That said, it seems harmless despite not understanding what it's for. So with or without the xfs_trans_log_inode() call:

<shrug> I interpreted this as Dave being fastidious about relogging inodes after every transaction roll even if it's not strictly necessary (but not otherwise harmful)

That's wrong. It is a requirement of rolling transactions that we log every object that is held locked across xfs_trans_commit() regardless of whether it was dirtied in that specific transaction or not.

That's because we call xfs_trans_reserve() when rolling the transaction, and that means the locked objects cannot be written to disk during that rolling transaction sequence. That means we can deadlock if the object we hold locked pins the tail of the log and there isn't space for the re-reservation of log space for the next transaction in the sequence.

IOWs, the simple rule of thumb is that objects held locked across xfs_trans_commit() should be logged in that transaction. Follow that simple rule everywhere, and we don't leave nasty landmines around to step on when we change code around....

Cheers,

Dave.

Signature · 6 lines

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

Quoted 71 lines · Dave Chinner

Quoted 62 lines · Darrick J. Wong

Quoted 61 lines · Brian Foster

Quoted 49 lines · Dave Chinner

Quoted 39 lines · Brian Foster

Quoted 38 lines · Dave Chinner

From: Dave Chinner <[email protected]>

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 <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_symlink.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

fs/xfs/xfs_symlink.c +0 -7

index 5b66ac12913c..27870e5cd259 100644
@@ -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);

Quoted 8 lines

fs/xfs/xfs_symlink.c xfs_inactive_symlink_rmt( · line 495

Quoted 5 lines

fs/xfs/xfs_symlink.c +2 -0

+
 	/*
 	 * Commit the transaction containing extent freeing and EFDs.
 	 */
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

Seems fine.. but do we even need this call? We're about to commit the transaction and unlock the inode...

Quoted 5 lines

Yes, I think we do. We need it to be committed in each of the rolling transactions so that the inode doesn't get written/replayed before any of the other dependent metadata changes in this final transaction.

Hmm, I don't follow what that means. IIUC the act of logging it again simply moves it forward in the log. That makes sense down in the dfops code but seems unecessary here given that we are about to complete the chain of transactions.

Quoted 7 lines

Quoted 7 lines

xfs_inactive_symlink_rmt() makes changes to the inode, invals/unmaps the remote bufs, joins the inode to the dfops and finishes the dfops. We return from xfs_defer_finish() having committed the (still locked) inode modifications and have a new/rolled transaction that covers the free of the associated blocks (EFDs). I could certainly be missing something, but from that point what difference does it make whether the final transaction relogs the inode before it commits?

Quoted 5 lines

It ensures the inode changes are sanely ordered. i.e. we don't write the inode to disk before all the other changes made in the rolling transaction are written to disk. And the same goes for recovery.

But what exactly does that accomplish? The inode changes, block unmap and and EFIs are all logged in the same transaction. Given the inode remains locked, what difference does it make whether it is relogged in the transaction that processes the EFIs? IOW, what can go wrong here without the additional inode relog?

Also, what do you mean by "sanely ordered?" AFAICT, there is no such writeback ordering guarantee even if the log items are part of the same transaction.

Suppose we return from xfs_inactive_symlink_rmt(), everything is committed/unlocked and an AIL push occurs.

Nothing
prevents a completely unrelated transaction from locking those same allocation btree buffers that were in the same (rolled) transaction as the EFD (plus relogged inode) before xfsaild gets to them, which means the associated items could be written back in arbitrary order anyways.

Quoted 84 lines · Brian Foster

Quoted 71 lines · Dave Chinner

Quoted 62 lines · Darrick J. Wong

Quoted 61 lines · Brian Foster

Quoted 49 lines · Dave Chinner

Quoted 39 lines · Brian Foster

Quoted 38 lines · Dave Chinner

From: Dave Chinner <[email protected]>

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 <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_symlink.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

fs/xfs/xfs_symlink.c +0 -7

index 5b66ac12913c..27870e5cd259 100644
@@ -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);

Quoted 8 lines

fs/xfs/xfs_symlink.c xfs_inactive_symlink_rmt( · line 495

Quoted 5 lines

fs/xfs/xfs_symlink.c +2 -0

+
 	/*
 	 * Commit the transaction containing extent freeing and EFDs.
 	 */
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

Seems fine.. but do we even need this call? We're about to commit the transaction and unlock the inode...

Quoted 5 lines

Yes, I think we do. We need it to be committed in each of the rolling transactions so that the inode doesn't get written/replayed before any of the other dependent metadata changes in this final transaction.

Hmm, I don't follow what that means. IIUC the act of logging it again simply moves it forward in the log. That makes sense down in the dfops code but seems unecessary here given that we are about to complete the chain of transactions.

Quoted 7 lines

Quoted 7 lines

xfs_inactive_symlink_rmt() makes changes to the inode, invals/unmaps the remote bufs, joins the inode to the dfops and finishes the dfops. We return from xfs_defer_finish() having committed the (still locked) inode modifications and have a new/rolled transaction that covers the free of the associated blocks (EFDs). I could certainly be missing something, but from that point what difference does it make whether the final transaction relogs the inode before it commits?

Quoted 5 lines

It ensures the inode changes are sanely ordered. i.e. we don't write the inode to disk before all the other changes made in the rolling transaction are written to disk. And the same goes for recovery.

But what exactly does that accomplish? The inode changes, block unmap and and EFIs are all logged in the same transaction. Given the inode remains locked, what difference does it make whether it is relogged in the transaction that processes the EFIs? IOW, what can go wrong here without the additional inode relog?

Quoted 3 lines

Also, what do you mean by "sanely ordered?" AFAICT, there is no such writeback ordering guarantee even if the log items are part of the same transaction.

The rolling tranactions can be committed can be in different checkpoints. If the journal is commited after the last roll but before the final commit, then the inode is unpinned before it is unlocked, while everything in the current transaction is still pinned in memory. When the last transaction commits, the inode gets unlocked and can be written back before the remaining operations in the rolling transaction are committed to stable storage.

i.e. we end up with an inode on disk with a younger LSN in it than a bunch of it's dependent changes have. Strictly speaking, that's an on-disk ordering violation and potentially an on-disk transactional change atomicity violation. We don't get a violation in memory because of the inode locking being held across the transaction commit, but we do get one on disk because the inode is unpinned in memory before all it's dependent changes are in stable journal storage....

Hence by logging the inode in the final transaction, we ensure it stays pinned in memory until the entire transaction is stable in the journal and we've guaranteed the on disk ordering and atomicity matches what is provided by the in-memory locking....

Quoted 2 lines

Suppose we return from xfs_inactive_symlink_rmt(), everything is committed/unlocked and an AIL push occurs.

All the objects are still pinned in memory, so they can't be written back by the AIL until the journal commits. :P

Quoted 5 lines

Nothing
prevents a completely unrelated transaction from locking those same allocation btree buffers that were in the same (rolled) transaction as the EFD (plus relogged inode) before xfsaild gets to them, which means the associated items could be written back in arbitrary order anyways.

Sure, but the combination of locking and journal ordering via relogging in the CIL handles reuse via independent atomic transaction cases just fine. That's different to ordering within a a single atomic transaction chain....

Cheers,

Dave.

Signature · 6 lines

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

From: Dave Chinner <[email protected]>

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 <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_reflink.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

fs/xfs/xfs_reflink.c +4 -2

index cdbd342a5249..bce2b5351d64 100644
@@ -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;

Signature · 6 lines

2.17.0

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

From: Dave Chinner <[email protected]>

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 <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_reflink.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

fs/xfs/xfs_reflink.c +4 -2

index cdbd342a5249..bce2b5351d64 100644
@@ -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;

Signature · 6 lines

2.17.0

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

From: Dave Chinner <[email protected]>

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 <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_reflink.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

fs/xfs/xfs_reflink.c +6 -1

index bce2b5351d64..12d441a73b53 100644
@@ -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,

fs/xfs/xfs_reflink.c +0 -0

 	error = xfs_reflink_cancel_cow_blocks(ip, tpp, 0, NULLFILEOFF, true);
 	if (error)
 		return error;

Signature · 6 lines

2.17.0

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

Quoted 5 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Another assert failure:

XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740

Same assert comment...

Quoted 36 lines

....

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 <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_reflink.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

fs/xfs/xfs_reflink.c +6 -1

index bce2b5351d64..12d441a73b53 100644
@@ -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(

fs/xfs/xfs_reflink.c xfs_reflink_clear_inode_flag( · line 1575

Quoted 6 lines

fs/xfs/xfs_reflink.c +0 -1

 	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);

Wasn't this just added in the previous patch?

This seems a bit
superfluous. If the inode was already joined by the one caller of this function, why not just remove this call in the previous patch rather than move it and remove it?

Quoted 52 lines · Brian Foster

Quoted 5 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Another assert failure:

XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740

Same assert comment...

Quoted 42 lines

....

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 <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_reflink.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

fs/xfs/xfs_reflink.c +6 -2

index bce2b5351d64..12d441a73b53 100644
@@ -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);

Quoted 5 lines

This seems a bit
superfluous. If the inode was already joined by the one caller of this function, why not just remove this call in the previous patch rather than move it and remove it?

Because every time I put multiple independent fixes into a single patch I get told to separate them into individual patches.

Cheers,

Dave.

Signature · 6 lines

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

Quoted 53 lines · Dave Chinner

Quoted 52 lines · Brian Foster

Quoted 5 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Another assert failure:

XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740

Same assert comment...

Quoted 42 lines

....

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 <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_reflink.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

fs/xfs/xfs_reflink.c +6 -2

index bce2b5351d64..12d441a73b53 100644
@@ -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);

...

Quoted 6 lines

Quoted 5 lines

This seems a bit
superfluous. If the inode was already joined by the one caller of this function, why not just remove this call in the previous patch rather than move it and remove it?

Quoted 3 lines

Because every time I put multiple independent fixes into a single patch I get told to separate them into individual patches.

Ok, fair enough:

Reviewed-by Brian Foster <[email protected]>

Quoted 11 lines

Cheers,

Dave.

Signature · 6 lines

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

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

From: Dave Chinner <[email protected]>

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

Signed-off-by Dave Chinner <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---

 fs/xfs/xfs_buf_item.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/xfs/xfs_buf_item.c +3 -1

index df62082f2204..8d6ed045b643 100644
@@ -745,8 +745,10 @@ 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(!bp->b_transp);
+		ASSERT(bip->bli_buf == bp);
 		return 0;
 	}
 

Signature · 6 lines

2.17.0

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

From: Dave Chinner <[email protected]>

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

Signed-off-by Dave Chinner <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_buf_item.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/xfs/xfs_buf_item.c +3 -1

index df62082f2204..8d6ed045b643 100644
@@ -745,8 +745,10 @@ 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(!bp->b_transp);
+		ASSERT(bip->bli_buf == bp);
 		return 0;
 	}
 

Signature · 6 lines

2.17.0

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

From: Dave Chinner <[email protected]>

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 <[email protected]>

---

 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            | 10 +++----
 fs/xfs/xfs_log_cil.c        | 21 ++++++--------
 fs/xfs/xfs_super.c          | 10 +------
 fs/xfs/xfs_trace.h          |  5 +---
 fs/xfs/xfs_trans.c          | 58 ++++++++++---------------------------
 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, 62 insertions(+), 123 deletions(-)

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

index 040eeda8426f..ab90998c1867 100644
@@ -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;

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

index d0b84da0cb1e..8efc06e62b13 100644
@@ -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 *);

fs/xfs/xfs_buf_item.c +1 -1

index 8d6ed045b643..c2311379d1c3 100644
@@ -438,7 +438,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);
 
 			/*

fs/xfs/xfs_icreate_item.c +1 -1

index a99a0f8aa528..5da9599156ed 100644
@@ -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);
 }

fs/xfs/xfs_log.c +5 -5

index e427864434c1..c21039f27e39 100644
@@ -1047,6 +1047,7 @@ xfs_log_item_init(
 	INIT_LIST_HEAD(&item->li_ail);
 	INIT_LIST_HEAD(&item->li_cil);
 	INIT_LIST_HEAD(&item->li_bio_list);
+	INIT_LIST_HEAD(&item->li_trans);
 }
 
 /*

@@ -2110,10 +2111,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 +2125,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;

fs/xfs/xfs_log_cil.c +9 -12

index a8675c631438..c15687724728 100644
@@ -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;
 
 		/*

@@ -406,7 +404,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;

@@ -479,11 +477,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;
 
 		/*

fs/xfs/xfs_super.c +1 -9

index d71424052917..5726ef496980 100644
@@ -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);

fs/xfs/xfs_trace.h +1 -4

index 947ab6e32c18..28a5b3f876ad 100644
@@ -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))
 )
 

fs/xfs/xfs_trans.c +16 -42

index 24744915357b..477c12412d31 100644
@@ -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

@@ -732,77 +731,52 @@ xfs_trans_unreserve_and_mod_sb(
 	return;
 }
 
-/*
- * 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.
- */
+/* Add the given log item to the transaction's list of log items. */
 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(list_empty(&lip->li_trans));
+	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);
 	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)
 {
-	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;
-
-		lip->li_desc = NULL;
-
+	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)
 			set_bit(XFS_LI_ABORTED, &lip->li_flags);
 		lip->li_ops->iop_unlock(lip);
-
-		xfs_trans_free_item_desc(lidp);
 	}
 }
 

@@ -1047,10 +1021,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);

fs/xfs/xfs_trans.h +4 -4

index 51145ddf7e5b..a32da7d7ea97 100644
@@ -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 */

@@ -72,11 +71,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_DIRTY	3	/* 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_FAILED),		"FAILED" }, \
+	{ (1 << XFS_LI_DIRTY),		"DIRTY" }
 
 struct xfs_item_ops {
 	void (*iop_size)(xfs_log_item_t *, int *, int *);

@@ -247,7 +248,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;

fs/xfs/xfs_trans_bmap.c +2 -2

index 14543d93cd4b..230a21df4b12 100644
@@ -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;

fs/xfs/xfs_trans_buf.c +9 -13

index 0081e9b3decf..a8ddb4eed279 100644
@@ -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);
 }
 
 /*

@@ -626,7 +622,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;
 	}

@@ -642,7 +638,7 @@ xfs_trans_binval(
 		memset(bip->bli_formats[i].blf_data_map, 0,
 		       (bip->bli_formats[i].blf_map_size * sizeof(uint)));
 	}
-	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;
 }
 

fs/xfs/xfs_trans_dquot.c +2 -2

index c3d547211d16..c381c02cca45 100644
@@ -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

fs/xfs/xfs_trans_extfree.c +2 -2

index ab438647592a..fb631081d829 100644
@@ -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;

fs/xfs/xfs_trans_inode.c +1 -2

index 07cea592dc01..f7bd7960a90f 100644
@@ -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.

fs/xfs/xfs_trans_priv.h +0 -1

index 43f773297b9d..9717ae74b36d 100644
@@ -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;

fs/xfs/xfs_trans_refcount.c +2 -2

index 94c1877af834..c7f8e82f5bda 100644
@@ -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;

fs/xfs/xfs_trans_rmap.c +2 -2

index 9b577beb43d7..5831ca0c270b 100644
@@ -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;

Signature · 6 lines

2.17.0

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