[PATCH 0/3 V2] Resubmit items failed during writeback

[PATCH 0/3 V2] Resubmit items failed during writeback

[Carlos Maiolino]

Hi,

this is the second version of this patchset, based on the comments on the
previous version.

Each patch has its own changelog, but some of the highlights are:

- Use atomic operations to set/clear xfs_log_item flags (including a patch to
  convert all the existing operations to atomic operations)
- call xfs_buf_do_callbacks_fail() from inside xfS_buf_iodone_error
- Use a helper function to resubmit the failed buffers.

The whole discussion is quite long, so I hope to have addressed all the topics
discussed.

- The patchset has passed xfstests and my reproducer to this specific bug.

-- 
2.9.3

[PATCH 1/3] xfs: use atomic operations to handle xfs_log_item flags

[Carlos Maiolino]

In order to fix a bug during buffer retries, a new flag type will be
added to xfs_log_item, and such operations need to be atomic.

Change all operations in xfs_log_item flags to atomic operations

To use atomic operations, xfs_log_item->li_flags also needed to be
converted to unsigned long type.

There is a small whitespace cleanup in the patch too

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_bmap_item.c     |  4 ++--
 fs/xfs/xfs_buf_item.c      |  4 ++--
 fs/xfs/xfs_dquot.c         |  2 +-
 fs/xfs/xfs_extfree_item.c  |  4 ++--
 fs/xfs/xfs_icache.c        |  2 +-
 fs/xfs/xfs_icreate_item.c  |  2 +-
 fs/xfs/xfs_inode.c         |  4 ++--
 fs/xfs/xfs_inode_item.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         |  4 ++--
 fs/xfs/xfs_trans.c         |  4 ++--
 fs/xfs/xfs_trans.h         |  2 +-
 fs/xfs/xfs_trans_ail.c     | 12 ++++++------
 fs/xfs/xfs_trans_buf.c     |  2 +-
 fs/xfs/xfs_trans_priv.h    |  2 +-
 17 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index d419d23..9ebdca9 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -141,7 +141,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_item_free(BUI_ITEM(lip));
 }
 
@@ -304,7 +304,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 0306168..6ac3816 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -587,7 +587,7 @@ xfs_buf_item_unlock(
 	 * (cancelled) buffers at unpin time, but we'll never go through the
 	 * pin/unpin cycle if we abort inside commit.
 	 */
-	aborted = (lip->li_flags & XFS_LI_ABORTED) ? true : false;
+	aborted = (test_bit(XFS_LI_ABORTED, &lip->li_flags)) ? true : false;
 	/*
 	 * Before possibly freeing the buf item, copy the per-transaction state
 	 * so we can reference it safely later after clearing it from the
@@ -975,7 +975,7 @@ xfs_buf_item_relse(
 	xfs_buf_log_item_t	*bip = bp->b_fspriv;
 
 	trace_xfs_buf_item_relse(bp, _RET_IP_);
-	ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
+	ASSERT(!(test_bit(XFS_LI_IN_AIL, &bip->bli_item.li_flags)));
 
 	bp->b_fspriv = bip->bli_item.li_bio_list;
 	if (bp->b_fspriv == NULL)
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 9d06cc3..e8f2cbc 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -1003,7 +1003,7 @@ 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) {
 
 		/* xfs_trans_ail_delete() drops the AIL lock. */
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 44f8c54..32a519d 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -150,7 +150,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_item_free(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 f61c84f8..23d750f 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -107,7 +107,7 @@ 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 865ad13..e24cf83 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, &icp->ic_item.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 ec9826c..208c8c7 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -504,7 +504,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++;
 			}
 		}
@@ -601,7 +601,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(lock_mode, 1))) {
 			xfs_iunlock(ip0, lock_mode);
 			if ((++attempts % 5) == 0)
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 08cb7d1..eeeadbb 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -783,7 +783,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_qm.c b/fs/xfs/xfs_qm.c
index 5fe6e70..da58263 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -169,7 +169,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 96fe209..5ecfd04 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -139,7 +139,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_item_free(CUI_ITEM(lip));
 }
 
@@ -308,7 +308,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 f3b139c..ada5ec7 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -139,7 +139,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_item_free(RUI_ITEM(lip));
 }
 
@@ -330,7 +330,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 7c5a165..d09e539 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -1031,7 +1031,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(
@@ -1083,7 +1083,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 be86e4e..6c8f492 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -764,7 +764,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);
@@ -835,7 +835,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 a07acbf..7ae04de 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_log_item		*li_bio_list;	/* buffer item list */
 	void				(*li_cb)(struct xfs_buf *,
 						 struct xfs_log_item *);
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 9056c0f..76e0de7 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -45,7 +45,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->xa_ail)
 		ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0);
@@ -653,7 +653,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_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;
@@ -663,7 +663,7 @@ xfs_trans_ail_update_bulk(
 			if (mlip == lip)
 				mlip_changed = 1;
 		} else {
-			lip->li_flags |= XFS_LI_IN_AIL;
+			set_bit(XFS_LI_IN_AIL, &lip->li_flags);
 			trace_xfs_ail_insert(lip, 0, lsn);
 		}
 		lip->li_lsn = lsn;
@@ -687,13 +687,13 @@ xfs_trans_ail_update_bulk(
 bool
 xfs_ail_delete_one(
 	struct xfs_ail		*ailp,
-	struct xfs_log_item 	*lip)
+	struct xfs_log_item	*lip)
 {
 	struct xfs_log_item	*mlip = xfs_ail_min(ailp);
 
 	trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn);
 	xfs_ail_delete(ailp, lip);
-	lip->li_flags &= ~XFS_LI_IN_AIL;
+	clear_bit(XFS_LI_IN_AIL, &lip->li_flags);
 	lip->li_lsn = 0;
 
 	return mlip == lip;
@@ -729,7 +729,7 @@ xfs_trans_ail_delete(
 	struct xfs_mount	*mp = ailp->xa_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->xa_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 8ee29ca..15814b5 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -433,7 +433,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
 		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 d91706c..82ea000 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->xa_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->xa_lock);
-- 
2.9.3

[PATCH 2/3 V2] xfs: Add infrastructure needed for error propagation during buffer IO failure

[Carlos Maiolino]

With the current code, XFS never re-submit a failed buffer for IO,
because the failed item in the buffer is kept in the flush locked state
forever.

To be able to resubmit an log item for IO, we need a way to mark an item
as failed, if, for any reason the buffer which the item belonged to
failed during writeback.

Add a new log item callback to be used after an IO completion failure
and make the needed clean ups.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
V2:
	- Update commit log to include a better description of why this
	  patch is needed and fix spelling mistakes
	- Move xfs_buf_do_callbacks_fail() call into
	  xfs_buf_iodone_callback_error, so the callbacks can be executed
	  before the buffer is released, and only after it has been
	  retried once

 fs/xfs/xfs_buf_item.c | 27 ++++++++++++++++++++++++++-
 fs/xfs/xfs_trans.h    |  5 ++++-
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 6ac3816..8f128e3 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -1051,6 +1051,24 @@ xfs_buf_do_callbacks(
 	}
 }
 
+STATIC void
+xfs_buf_do_callbacks_fail(
+	struct xfs_buf		*bp)
+{
+	struct xfs_log_item	*lip, *next;
+	unsigned int		bflags = bp->b_flags;
+
+	lip = bp->b_fspriv;
+	while (lip != NULL) {
+		next = lip->li_bio_list;
+
+		if (lip->li_ops->iop_error)
+			lip->li_ops->iop_error(lip, bflags);
+
+		lip = next;
+	}
+}
+
 static bool
 xfs_buf_iodone_callback_error(
 	struct xfs_buf		*bp)
@@ -1101,6 +1119,7 @@ xfs_buf_iodone_callback_error(
 
 		xfs_buf_ioerror(bp, 0);
 		xfs_buf_submit(bp);
+
 		return true;
 	}
 
@@ -1120,8 +1139,14 @@ xfs_buf_iodone_callback_error(
 	if ((mp->m_flags & XFS_MOUNT_UNMOUNTING) && mp->m_fail_unmount)
 		goto permanent_error;
 
-	/* still a transient error, higher layers will retry */
+	/*
+	 * still a transient error, run IO completion failure callbacks and
+	 * let the higher layers retry the buffer.
+	 * */
 	xfs_buf_ioerror(bp, 0);
+
+	/* run failure callbacks before releasing buffer */
+	xfs_buf_do_callbacks_fail(bp);
 	xfs_buf_relse(bp);
 	return true;
 
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 7ae04de..7fcf48d 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -65,10 +65,12 @@ typedef struct xfs_log_item {
 
 #define	XFS_LI_IN_AIL	0x1
 #define XFS_LI_ABORTED	0x2
+#define XFS_LI_FAILED	0x3
 
 #define XFS_LI_FLAGS \
 	{ XFS_LI_IN_AIL,	"IN_AIL" }, \
-	{ XFS_LI_ABORTED,	"ABORTED" }
+	{ XFS_LI_ABORTED,	"ABORTED" }, \
+	{ XFS_LI_FAILED,	"FAILED" }
 
 struct xfs_item_ops {
 	void (*iop_size)(xfs_log_item_t *, int *, int *);
@@ -79,6 +81,7 @@ struct xfs_item_ops {
 	void (*iop_unlock)(xfs_log_item_t *);
 	xfs_lsn_t (*iop_committed)(xfs_log_item_t *, xfs_lsn_t);
 	void (*iop_committing)(xfs_log_item_t *, xfs_lsn_t);
+	void (*iop_error)(xfs_log_item_t *, unsigned int bflags);
 };
 
 void	xfs_log_item_init(struct xfs_mount *mp, struct xfs_log_item *item,
-- 
2.9.3

[PATCH 3/3 V2] xfs: Properly retry failed inode items in case of error during buffer writeback

[Carlos Maiolino]

When a buffer has been failed during writeback, the inode items into it
are kept flush locked, and are never resubmitted due the flush lock, so,
if any buffer fails to be written, the items in AIL are never written to
disk and never unlocked.

This causes unmount operation to hang due these items flush locked in AIL,
but this also causes the items in AIL to never be written back, even when
the IO device comes back to normal.

I've been testing this patch with a DM-thin device, creating a
filesystem larger than the real device.

When writing enough data to fill the DM-thin device, XFS receives ENOSPC
errors from the device, and keep spinning on xfsaild (when 'retry
forever' configuration is set).

At this point, the filesystem can not be unmounted because of the flush locked
items in AIL, but worse, the items in AIL are never retried at all
(once xfs_inode_item_push() will skip the items that are flush locked),
even if the underlying DM-thin device is expanded to the proper size.

This patch fixes both cases, retrying any item that has been failed
previously, using the infra-structure provided by the previous patch.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
V2:
	- Fix XFS_LI_FAILED flag removal
	- Use atomic operations to set and clear XFS_LI_FAILED flag
	- Remove check for XBF_WRITE_FAIL in xfs_inode_item_push
	- Add more comments to the code
	- Add a helper function to resubmit the failed buffers, so this
	  can be also used in dquot system without duplicating code

 fs/xfs/xfs_buf_item.c   | 42 ++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_buf_item.h   |  2 ++
 fs/xfs/xfs_inode_item.c | 36 +++++++++++++++++++++++++++++++++++-
 3 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 8f128e3..4b68b27 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -29,6 +29,7 @@
 #include "xfs_error.h"
 #include "xfs_trace.h"
 #include "xfs_log.h"
+#include "xfs_inode.h"
 
 
 kmem_zone_t	*xfs_buf_item_zone;
@@ -1226,3 +1227,44 @@ xfs_buf_iodone(
 	xfs_trans_ail_delete(ailp, lip, SHUTDOWN_CORRUPT_INCORE);
 	xfs_buf_item_free(BUF_ITEM(lip));
 }
+
+/* Requeue a failed buffer for writeback
+ *
+ * Return true if the buffer has been re-queued properly, false otherwise
+ *
+ */
+bool
+xfs_buf_resubmit_failed_buffers(
+	struct xfs_inode	*ip,
+	struct xfs_log_item	*lip,
+	struct xfs_buf		*bp,
+	struct list_head	*buffer_list)
+{
+	struct xfs_dinode	*dip;
+	struct xfs_log_item	*next;
+	int			error;
+	bool			ret;
+
+	error = xfs_imap_to_bp(ip->i_mount, NULL, &ip->i_imap,
+			       &dip, &bp, XBF_TRYLOCK, 0);
+
+		if (error)
+			return false;
+
+		/* Clear XFS_LI_FAILED flag from all items before resubmit */
+		while (lip != NULL) {
+			next = lip->li_bio_list;
+
+			clear_bit(XFS_LI_FAILED, &lip->li_flags);
+			lip = next;
+		}
+
+		/* Add this buffer back to the delayed write list */
+		if (!xfs_buf_delwri_queue(bp, buffer_list))
+			ret = false;
+		else
+			ret = true;
+
+		xfs_buf_relse(bp);
+		return ret;
+}
diff --git a/fs/xfs/xfs_buf_item.h b/fs/xfs/xfs_buf_item.h
index f7eba99..7340c64 100644
--- a/fs/xfs/xfs_buf_item.h
+++ b/fs/xfs/xfs_buf_item.h
@@ -70,6 +70,8 @@ void	xfs_buf_attach_iodone(struct xfs_buf *,
 			      xfs_log_item_t *);
 void	xfs_buf_iodone_callbacks(struct xfs_buf *);
 void	xfs_buf_iodone(struct xfs_buf *, struct xfs_log_item *);
+bool	xfs_buf_resubmit_failed_buffers(struct xfs_inode *, struct xfs_log_item *,
+					struct xfs_buf *, struct list_head *);
 
 extern kmem_zone_t	*xfs_buf_item_zone;
 
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index eeeadbb..97db299 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -27,6 +27,7 @@
 #include "xfs_error.h"
 #include "xfs_trace.h"
 #include "xfs_trans_priv.h"
+#include "xfs_buf_item.h"
 #include "xfs_log.h"
 
 
@@ -475,6 +476,24 @@ xfs_inode_item_unpin(
 		wake_up_bit(&ip->i_flags, __XFS_IPINNED_BIT);
 }
 
+STATIC void
+xfs_inode_item_error(
+	struct xfs_log_item	*lip,
+	unsigned int		bflags)
+{
+
+	/*
+	 * The buffer writeback containing this inode has been failed
+	 * mark it as failed and unlock the flush lock, so it can be retried
+	 * again.
+	 * It requires an atomic operation, once the parent object is not locked
+	 * in this context, and we need to avoid races with other log item state
+	 * changes.
+	 */
+	if (bflags & XBF_WRITE_FAIL)
+		set_bit(XFS_LI_FAILED, &lip->li_flags);
+}
+
 STATIC uint
 xfs_inode_item_push(
 	struct xfs_log_item	*lip,
@@ -517,8 +536,22 @@ xfs_inode_item_push(
 	 * the AIL.
 	 */
 	if (!xfs_iflock_nowait(ip)) {
+
+		/*
+		 * The buffer containing this item failed to be written back
+		 * previously. Resubmit the buffer for IO.
+		 */
+		if (lip->li_flags & XFS_LI_FAILED) {
+			if (!xfs_buf_resubmit_failed_buffers(ip, lip, bp,
+							     buffer_list))
+				rval = XFS_ITEM_FLUSHING;
+
+			goto out_unlock;
+		}
+
 		rval = XFS_ITEM_FLUSHING;
 		goto out_unlock;
+
 	}
 
 	ASSERT(iip->ili_fields != 0 || XFS_FORCED_SHUTDOWN(ip->i_mount));
@@ -622,7 +655,8 @@ static const struct xfs_item_ops xfs_inode_item_ops = {
 	.iop_unlock	= xfs_inode_item_unlock,
 	.iop_committed	= xfs_inode_item_committed,
 	.iop_push	= xfs_inode_item_push,
-	.iop_committing = xfs_inode_item_committing
+	.iop_committing = xfs_inode_item_committing,
+	.iop_error	= xfs_inode_item_error
 };
 
 
-- 
2.9.3

Re: [PATCH 1/3] xfs: use atomic operations to handle xfs_log_item flags

[Christoph Hellwig]

> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -587,7 +587,7 @@ xfs_buf_item_unlock(
>  	 * (cancelled) buffers at unpin time, but we'll never go through the
>  	 * pin/unpin cycle if we abort inside commit.
>  	 */
> -	aborted = (lip->li_flags & XFS_LI_ABORTED) ? true : false;
> +	aborted = (test_bit(XFS_LI_ABORTED, &lip->li_flags)) ? true : false;

	aborted = test_bit(XFS_LI_ABORTED, &lip->li_flags);

does the same job in a slightly simpler way.

> +	ASSERT(!(test_bit(XFS_LI_IN_AIL, &bip->bli_item.li_flags)));

no need for the inner braces.

> +		ASSERT(!(test_bit(XFS_LI_IN_AIL, &ip->i_itemp->ili_item.li_flags)));

Same here.  Also please don't break lines after 80 characters.

> +	       !(test_bit(XFS_LI_IN_AIL, &dqp->q_logitem.qli_item.li_flags)));

Also no need for the braces around test_bit here.

> +	if (!(test_bit(XFS_LI_IN_AIL, &lip->li_flags))) {

.. again

> +		ASSERT(!(test_bit(XFS_LI_IN_AIL, &bip->bli_item.li_flags)));

.. again

Also last but not least the arguments to the *_bit functions are
bit indices, so they should be renumber to 0 and 1 (and the tracing
helpers will need some updates as well).

Re: [PATCH 2/3 V2] xfs: Add infrastructure needed for error propagation during buffer IO failure

[Christoph Hellwig]

Need to read up on the history and background a bit more, but some
cosmetic comments below:

> +	lip = bp->b_fspriv;
> +	while (lip != NULL) {
> +		next = lip->li_bio_list;
> +
> +		if (lip->li_ops->iop_error)
> +			lip->li_ops->iop_error(lip, bflags);
> +
> +		lip = next;
> +	}

	for (lip = bp->b_fspriv; lip; lip = next) {
		next = lip->li_bio_list;
		if (lip->li_ops->iop_error)
			lip->li_ops->iop_error(lip, bflags);
	}

?

> @@ -1101,6 +1119,7 @@ xfs_buf_iodone_callback_error(
>  
>  		xfs_buf_ioerror(bp, 0);
>  		xfs_buf_submit(bp);
> +
>  		return true;
>  	}

whitespace noise.

Re: [PATCH 1/3] xfs: use atomic operations to handle xfs_log_item flags

[Carlos Maiolino]

Hi,

On Mon, May 22, 2017 at 12:11:11PM -0700, Christoph Hellwig wrote:
> > --- a/fs/xfs/xfs_buf_item.c
> > +++ b/fs/xfs/xfs_buf_item.c
> > @@ -587,7 +587,7 @@ xfs_buf_item_unlock(
> >  	 * (cancelled) buffers at unpin time, but we'll never go through the
> >  	 * pin/unpin cycle if we abort inside commit.
> >  	 */
> > -	aborted = (lip->li_flags & XFS_LI_ABORTED) ? true : false;
> > +	aborted = (test_bit(XFS_LI_ABORTED, &lip->li_flags)) ? true : false;
> 
> 	aborted = test_bit(XFS_LI_ABORTED, &lip->li_flags);
> 
> does the same job in a slightly simpler way.
> 
> > +	ASSERT(!(test_bit(XFS_LI_IN_AIL, &bip->bli_item.li_flags)));
> 
> no need for the inner braces.
> 
> > +		ASSERT(!(test_bit(XFS_LI_IN_AIL, &ip->i_itemp->ili_item.li_flags)));
> 
> Same here.  Also please don't break lines after 80 characters.
> 
> > +	       !(test_bit(XFS_LI_IN_AIL, &dqp->q_logitem.qli_item.li_flags)));
> 
> Also no need for the braces around test_bit here.
> 
> > +	if (!(test_bit(XFS_LI_IN_AIL, &lip->li_flags))) {
> 
> .. again
> 
> > +		ASSERT(!(test_bit(XFS_LI_IN_AIL, &bip->bli_item.li_flags)));
> 
> .. again
> 
> Also last but not least the arguments to the *_bit functions are
> bit indices, so they should be renumber to 0 and 1 (and the tracing
> helpers will need some updates as well).

Thanks for the review of this series, I'll queue these changes for V3.


-- 
Carlos

Re: [PATCH 1/3] xfs: use atomic operations to handle xfs_log_item flags

[Carlos Maiolino]

On Mon, May 22, 2017 at 12:11:11PM -0700, Christoph Hellwig wrote:
> > --- a/fs/xfs/xfs_buf_item.c
> > +++ b/fs/xfs/xfs_buf_item.c
> > @@ -587,7 +587,7 @@ xfs_buf_item_unlock(
> >  	 * (cancelled) buffers at unpin time, but we'll never go through the
> >  	 * pin/unpin cycle if we abort inside commit.
> >  	 */
> > -	aborted = (lip->li_flags & XFS_LI_ABORTED) ? true : false;
> > +	aborted = (test_bit(XFS_LI_ABORTED, &lip->li_flags)) ? true : false;
> 
> 	aborted = test_bit(XFS_LI_ABORTED, &lip->li_flags);
> 
> does the same job in a slightly simpler way.
> 
> > +	ASSERT(!(test_bit(XFS_LI_IN_AIL, &bip->bli_item.li_flags)));
> 
> no need for the inner braces.
> 
> > +		ASSERT(!(test_bit(XFS_LI_IN_AIL, &ip->i_itemp->ili_item.li_flags)));
> 
> Same here.  Also please don't break lines after 80 characters.

Btw, I believe you meant I didn't break the line here? Removing the inner braces
will leave it with 82 chars, I wonder if it's worth to break the line here in
this case? Just looks harder to read with the line broken for just 2 chars.


> 
> > +	       !(test_bit(XFS_LI_IN_AIL, &dqp->q_logitem.qli_item.li_flags)));
> 
> Also no need for the braces around test_bit here.
> 
> > +	if (!(test_bit(XFS_LI_IN_AIL, &lip->li_flags))) {
> 
> .. again
> 
> > +		ASSERT(!(test_bit(XFS_LI_IN_AIL, &bip->bli_item.li_flags)));
> 
> .. again
> 
> Also last but not least the arguments to the *_bit functions are
> bit indices, so they should be renumber to 0 and 1 (and the tracing
> helpers will need some updates as well).

-- 
Carlos

Re: [PATCH 2/3 V2] xfs: Add infrastructure needed for error propagation during buffer IO failure

[Carlos Maiolino]

On Mon, May 22, 2017 at 12:13:22PM -0700, Christoph Hellwig wrote:
> Need to read up on the history and background a bit more, but some
> cosmetic comments below:
> 
> > +	lip = bp->b_fspriv;
> > +	while (lip != NULL) {
> > +		next = lip->li_bio_list;
> > +
> > +		if (lip->li_ops->iop_error)
> > +			lip->li_ops->iop_error(lip, bflags);
> > +
> > +		lip = next;
> > +	}
> 
> 	for (lip = bp->b_fspriv; lip; lip = next) {
> 		next = lip->li_bio_list;
> 		if (lip->li_ops->iop_error)
> 			lip->li_ops->iop_error(lip, bflags);
> 	}
> 
> ?

Agree, looks much better.

> 
> > @@ -1101,6 +1119,7 @@ xfs_buf_iodone_callback_error(
> >  
> >  		xfs_buf_ioerror(bp, 0);
> >  		xfs_buf_submit(bp);
> > +
> >  		return true;
> >  	}
> 
> whitespace noise.


Fixed and queued for V3, thanks

-- 
Carlos

Re: [PATCH 1/3] xfs: use atomic operations to handle xfs_log_item flags

[Brian Foster]

On Mon, May 22, 2017 at 05:32:18PM +0200, Carlos Maiolino wrote:
> In order to fix a bug during buffer retries, a new flag type will be
> added to xfs_log_item, and such operations need to be atomic.
> 
> Change all operations in xfs_log_item flags to atomic operations
> 
> To use atomic operations, xfs_log_item->li_flags also needed to be
> converted to unsigned long type.
> 
> There is a small whitespace cleanup in the patch too
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---

As noted in the v1 discussion, I don't think this needs to be a
dependency of the bug fix patch. Provided we can alleviate Dave's
concerns over performance and not add any ->xa_lock acquisitions that
aren't triggered by I/O errors, can we fix the bug using ->xa_lock and
then port this on top? That way we have more of a backportable fix for
older kernels affected by this problem.

(Please see my comments on patches 2 and 3 wrt proposed changes to avoid
the custom callback and need for atomic flags..)

Brian

>  fs/xfs/xfs_bmap_item.c     |  4 ++--
>  fs/xfs/xfs_buf_item.c      |  4 ++--
>  fs/xfs/xfs_dquot.c         |  2 +-
>  fs/xfs/xfs_extfree_item.c  |  4 ++--
>  fs/xfs/xfs_icache.c        |  2 +-
>  fs/xfs/xfs_icreate_item.c  |  2 +-
>  fs/xfs/xfs_inode.c         |  4 ++--
>  fs/xfs/xfs_inode_item.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         |  4 ++--
>  fs/xfs/xfs_trans.c         |  4 ++--
>  fs/xfs/xfs_trans.h         |  2 +-
>  fs/xfs/xfs_trans_ail.c     | 12 ++++++------
>  fs/xfs/xfs_trans_buf.c     |  2 +-
>  fs/xfs/xfs_trans_priv.h    |  2 +-
>  17 files changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index d419d23..9ebdca9 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -141,7 +141,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_item_free(BUI_ITEM(lip));
>  }
>  
> @@ -304,7 +304,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 0306168..6ac3816 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -587,7 +587,7 @@ xfs_buf_item_unlock(
>  	 * (cancelled) buffers at unpin time, but we'll never go through the
>  	 * pin/unpin cycle if we abort inside commit.
>  	 */
> -	aborted = (lip->li_flags & XFS_LI_ABORTED) ? true : false;
> +	aborted = (test_bit(XFS_LI_ABORTED, &lip->li_flags)) ? true : false;
>  	/*
>  	 * Before possibly freeing the buf item, copy the per-transaction state
>  	 * so we can reference it safely later after clearing it from the
> @@ -975,7 +975,7 @@ xfs_buf_item_relse(
>  	xfs_buf_log_item_t	*bip = bp->b_fspriv;
>  
>  	trace_xfs_buf_item_relse(bp, _RET_IP_);
> -	ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
> +	ASSERT(!(test_bit(XFS_LI_IN_AIL, &bip->bli_item.li_flags)));
>  
>  	bp->b_fspriv = bip->bli_item.li_bio_list;
>  	if (bp->b_fspriv == NULL)
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 9d06cc3..e8f2cbc 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -1003,7 +1003,7 @@ 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) {
>  
>  		/* xfs_trans_ail_delete() drops the AIL lock. */
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 44f8c54..32a519d 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -150,7 +150,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_item_free(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 f61c84f8..23d750f 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -107,7 +107,7 @@ 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 865ad13..e24cf83 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, &icp->ic_item.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 ec9826c..208c8c7 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -504,7 +504,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++;
>  			}
>  		}
> @@ -601,7 +601,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(lock_mode, 1))) {
>  			xfs_iunlock(ip0, lock_mode);
>  			if ((++attempts % 5) == 0)
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 08cb7d1..eeeadbb 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -783,7 +783,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_qm.c b/fs/xfs/xfs_qm.c
> index 5fe6e70..da58263 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -169,7 +169,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 96fe209..5ecfd04 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -139,7 +139,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_item_free(CUI_ITEM(lip));
>  }
>  
> @@ -308,7 +308,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 f3b139c..ada5ec7 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -139,7 +139,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_item_free(RUI_ITEM(lip));
>  }
>  
> @@ -330,7 +330,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 7c5a165..d09e539 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -1031,7 +1031,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(
> @@ -1083,7 +1083,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 be86e4e..6c8f492 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -764,7 +764,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);
> @@ -835,7 +835,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 a07acbf..7ae04de 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_log_item		*li_bio_list;	/* buffer item list */
>  	void				(*li_cb)(struct xfs_buf *,
>  						 struct xfs_log_item *);
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 9056c0f..76e0de7 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -45,7 +45,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->xa_ail)
>  		ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0);
> @@ -653,7 +653,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_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;
> @@ -663,7 +663,7 @@ xfs_trans_ail_update_bulk(
>  			if (mlip == lip)
>  				mlip_changed = 1;
>  		} else {
> -			lip->li_flags |= XFS_LI_IN_AIL;
> +			set_bit(XFS_LI_IN_AIL, &lip->li_flags);
>  			trace_xfs_ail_insert(lip, 0, lsn);
>  		}
>  		lip->li_lsn = lsn;
> @@ -687,13 +687,13 @@ xfs_trans_ail_update_bulk(
>  bool
>  xfs_ail_delete_one(
>  	struct xfs_ail		*ailp,
> -	struct xfs_log_item 	*lip)
> +	struct xfs_log_item	*lip)
>  {
>  	struct xfs_log_item	*mlip = xfs_ail_min(ailp);
>  
>  	trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn);
>  	xfs_ail_delete(ailp, lip);
> -	lip->li_flags &= ~XFS_LI_IN_AIL;
> +	clear_bit(XFS_LI_IN_AIL, &lip->li_flags);
>  	lip->li_lsn = 0;
>  
>  	return mlip == lip;
> @@ -729,7 +729,7 @@ xfs_trans_ail_delete(
>  	struct xfs_mount	*mp = ailp->xa_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->xa_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 8ee29ca..15814b5 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -433,7 +433,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
>  		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 d91706c..82ea000 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->xa_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->xa_lock);
> -- 
> 2.9.3
> 
> --
> 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 2/3 V2] xfs: Add infrastructure needed for error propagation during buffer IO failure

[Brian Foster]

On Mon, May 22, 2017 at 05:32:19PM +0200, Carlos Maiolino wrote:
> With the current code, XFS never re-submit a failed buffer for IO,
> because the failed item in the buffer is kept in the flush locked state
> forever.
> 
> To be able to resubmit an log item for IO, we need a way to mark an item
> as failed, if, for any reason the buffer which the item belonged to
> failed during writeback.
> 
> Add a new log item callback to be used after an IO completion failure
> and make the needed clean ups.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> V2:
> 	- Update commit log to include a better description of why this
> 	  patch is needed and fix spelling mistakes
> 	- Move xfs_buf_do_callbacks_fail() call into
> 	  xfs_buf_iodone_callback_error, so the callbacks can be executed
> 	  before the buffer is released, and only after it has been
> 	  retried once
> 
>  fs/xfs/xfs_buf_item.c | 27 ++++++++++++++++++++++++++-
>  fs/xfs/xfs_trans.h    |  5 ++++-
>  2 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 6ac3816..8f128e3 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -1051,6 +1051,24 @@ xfs_buf_do_callbacks(
>  	}
>  }
>  
> +STATIC void
> +xfs_buf_do_callbacks_fail(
> +	struct xfs_buf		*bp)
> +{
> +	struct xfs_log_item	*lip, *next;
> +	unsigned int		bflags = bp->b_flags;
> +
> +	lip = bp->b_fspriv;
> +	while (lip != NULL) {
> +		next = lip->li_bio_list;
> +
> +		if (lip->li_ops->iop_error)
> +			lip->li_ops->iop_error(lip, bflags);
> +
> +		lip = next;
> +	}

AFAICT, this could do something like the following:

	spin_lock(&ailp->xa_lock);
	while (lip != NULL) {
		next = lip->li_bio_list;
		lip->li_flags |= XFS_LI_FAILED;
		lip = next;
	}
	spin_unlock(&ailp->xa_lock);

... to generically and unconditionally flag the log item as failed and
avoid the need for ->iop_error(). We also need to clear XFS_LI_FAILED at
the same place we clear XFS_LI_IN_AIL (i.e., AIL removal) to ensure a
subsequent successful I/O completion updates the log item appropriately.

Then the result of this patch is that all log items are flagged as
failed on I/O error until they are ultimately removed from the AIL. We
otherwise have so far not changed behavior in any way.

Brian

> +}
> +
>  static bool
>  xfs_buf_iodone_callback_error(
>  	struct xfs_buf		*bp)
> @@ -1101,6 +1119,7 @@ xfs_buf_iodone_callback_error(
>  
>  		xfs_buf_ioerror(bp, 0);
>  		xfs_buf_submit(bp);
> +
>  		return true;
>  	}
>  
> @@ -1120,8 +1139,14 @@ xfs_buf_iodone_callback_error(
>  	if ((mp->m_flags & XFS_MOUNT_UNMOUNTING) && mp->m_fail_unmount)
>  		goto permanent_error;
>  
> -	/* still a transient error, higher layers will retry */
> +	/*
> +	 * still a transient error, run IO completion failure callbacks and
> +	 * let the higher layers retry the buffer.
> +	 * */
>  	xfs_buf_ioerror(bp, 0);
> +
> +	/* run failure callbacks before releasing buffer */
> +	xfs_buf_do_callbacks_fail(bp);
>  	xfs_buf_relse(bp);
>  	return true;
>  
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 7ae04de..7fcf48d 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -65,10 +65,12 @@ typedef struct xfs_log_item {
>  
>  #define	XFS_LI_IN_AIL	0x1
>  #define XFS_LI_ABORTED	0x2
> +#define XFS_LI_FAILED	0x3
>  
>  #define XFS_LI_FLAGS \
>  	{ XFS_LI_IN_AIL,	"IN_AIL" }, \
> -	{ XFS_LI_ABORTED,	"ABORTED" }
> +	{ XFS_LI_ABORTED,	"ABORTED" }, \
> +	{ XFS_LI_FAILED,	"FAILED" }
>  
>  struct xfs_item_ops {
>  	void (*iop_size)(xfs_log_item_t *, int *, int *);
> @@ -79,6 +81,7 @@ struct xfs_item_ops {
>  	void (*iop_unlock)(xfs_log_item_t *);
>  	xfs_lsn_t (*iop_committed)(xfs_log_item_t *, xfs_lsn_t);
>  	void (*iop_committing)(xfs_log_item_t *, xfs_lsn_t);
> +	void (*iop_error)(xfs_log_item_t *, unsigned int bflags);
>  };
>  
>  void	xfs_log_item_init(struct xfs_mount *mp, struct xfs_log_item *item,
> -- 
> 2.9.3
> 
> --
> 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 3/3 V2] xfs: Properly retry failed inode items in case of error during buffer writeback

[Brian Foster]

On Mon, May 22, 2017 at 05:32:20PM +0200, Carlos Maiolino wrote:
> When a buffer has been failed during writeback, the inode items into it
> are kept flush locked, and are never resubmitted due the flush lock, so,
> if any buffer fails to be written, the items in AIL are never written to
> disk and never unlocked.
> 
> This causes unmount operation to hang due these items flush locked in AIL,
> but this also causes the items in AIL to never be written back, even when
> the IO device comes back to normal.
> 
> I've been testing this patch with a DM-thin device, creating a
> filesystem larger than the real device.
> 
> When writing enough data to fill the DM-thin device, XFS receives ENOSPC
> errors from the device, and keep spinning on xfsaild (when 'retry
> forever' configuration is set).
> 
> At this point, the filesystem can not be unmounted because of the flush locked
> items in AIL, but worse, the items in AIL are never retried at all
> (once xfs_inode_item_push() will skip the items that are flush locked),
> even if the underlying DM-thin device is expanded to the proper size.
> 
> This patch fixes both cases, retrying any item that has been failed
> previously, using the infra-structure provided by the previous patch.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> V2:
> 	- Fix XFS_LI_FAILED flag removal
> 	- Use atomic operations to set and clear XFS_LI_FAILED flag
> 	- Remove check for XBF_WRITE_FAIL in xfs_inode_item_push
> 	- Add more comments to the code
> 	- Add a helper function to resubmit the failed buffers, so this
> 	  can be also used in dquot system without duplicating code
> 
>  fs/xfs/xfs_buf_item.c   | 42 ++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_buf_item.h   |  2 ++
>  fs/xfs/xfs_inode_item.c | 36 +++++++++++++++++++++++++++++++++++-
>  3 files changed, 79 insertions(+), 1 deletion(-)
> 
...
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index eeeadbb..97db299 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -27,6 +27,7 @@
>  #include "xfs_error.h"
>  #include "xfs_trace.h"
>  #include "xfs_trans_priv.h"
> +#include "xfs_buf_item.h"
>  #include "xfs_log.h"
>  
>  
> @@ -475,6 +476,24 @@ xfs_inode_item_unpin(
>  		wake_up_bit(&ip->i_flags, __XFS_IPINNED_BIT);
>  }
>  
> +STATIC void
> +xfs_inode_item_error(
> +	struct xfs_log_item	*lip,
> +	unsigned int		bflags)
> +{
> +
> +	/*
> +	 * The buffer writeback containing this inode has been failed
> +	 * mark it as failed and unlock the flush lock, so it can be retried
> +	 * again.
> +	 * It requires an atomic operation, once the parent object is not locked
> +	 * in this context, and we need to avoid races with other log item state
> +	 * changes.
> +	 */
> +	if (bflags & XBF_WRITE_FAIL)
> +		set_bit(XFS_LI_FAILED, &lip->li_flags);
> +}

With the change to patch 2 to set LI_FAILED on all log items, this can
go away completely. We know that LI_FAILED will be set on any log item
attached to a buffer that fails.

> +
>  STATIC uint
>  xfs_inode_item_push(
>  	struct xfs_log_item	*lip,
> @@ -517,8 +536,22 @@ xfs_inode_item_push(
>  	 * the AIL.
>  	 */
>  	if (!xfs_iflock_nowait(ip)) {
> +
> +		/*
> +		 * The buffer containing this item failed to be written back
> +		 * previously. Resubmit the buffer for IO.
> +		 */
> +		if (lip->li_flags & XFS_LI_FAILED) {
> +			if (!xfs_buf_resubmit_failed_buffers(ip, lip, bp,
> +							     buffer_list))
> +				rval = XFS_ITEM_FLUSHING;
> +
> +			goto out_unlock;
> +		}
> +

I think we need to do the XFS_LI_FAILED check first thing in
xfs_inode_item_push(). As part of the v1 discussion, Dave pointed out
that there is the possibility that somebody else is holding the inode
lock and blocking on the flush lock by the time xfs_inode_item_push() is
called. That means we would never get past the xfs_ilock_nowait() call
earlier in the function and thus never resubmit the buffer.

That aside, we're now modifying inode log item behavior based on
LI_FAILED. Since we now skip an iflush when LI_FAILED is set, we need to
handle the case in xfs_iflush_done() where the flush lock is unlocked
but the log item is not removed from the AIL. I think this should be
quite rare, but IIUC the scenario goes something like this:

	- Inode 1 transaction is committed, AIL push flushes the inode,
	  buffer I/O fails and the log item is set LI_FAILED. The inode
	  log item is now sitting in the AIL waiting for a retry.
	- Inode 1 is relogged and committed in another transaction. Its
	  place in the AIL is moved. The changes to the xfs_inode in
	  this tx have not been flushed to the buffer.
	- Inode 2 transaction is committed and inserted to AIL. Inode 2
	  is backed by the same buffer as inode 1.
	- AIL pushes inode 2, flushes to the buffer and submits. It does
	  not clear LI_FAILED of inode 1 because inode 2 has never
	  failed.
	- Buffer I/O succeeds, xfs_iflush_done() runs and removes inode
	  2 from the AIL and flush unlocks. Inode 1 is flush unlocked
	  but remains on the AIL because the flush from the second
	  transaction above has not yet occurred.
	- xfs_iflush_done() clears LI_FAILED so the subsequent AIL push
	  of inode 1 actually flushes the latest in-core inode to the
	  buffer.

So IOW, I think we need to include something like the hunk appended
below (untested) to this patch.

Dave,

Do you see anything wrong with this overall approach? I think this
avoids the hard dependency on atomic flags because all li_flags updates
remain under ->xa_lock. We've covered the I/O submission context concern
because we incorporate the previously discussed idea of clearing the
flag on successful I/O completion. Finally, normal running performance
should not be affected because the ->xa_lock is not taken anywhere new
unless I/O errors have occurred. Thoughts?

Brian

--- 8< ---

diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 29ada12..5e1ecb1 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -722,7 +722,8 @@ xfs_iflush_done(
 		 * the AIL lock.
 		 */
 		iip = INODE_ITEM(blip);
-		if (iip->ili_logged && blip->li_lsn == iip->ili_flush_lsn)
+		if ((iip->ili_logged && blip->li_lsn == iip->ili_flush_lsn) ||
+		    blip->li_flags & XFS_LI_FAILED)
 			need_ail++;
 
 		blip = next;
@@ -730,7 +731,8 @@ xfs_iflush_done(
 
 	/* 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)
+	if ((iip->ili_logged && lip->li_lsn == iip->ili_flush_lsn) ||
+	    lip->li_flags & XFS_LI_FAILED)
 		need_ail++;
 
 	/*
@@ -751,6 +753,8 @@ xfs_iflush_done(
 			if (INODE_ITEM(blip)->ili_logged &&
 			    blip->li_lsn == INODE_ITEM(blip)->ili_flush_lsn)
 				mlip_changed |= xfs_ail_delete_one(ailp, blip);
+			else if (blip->li_flags & XFS_LI_FAILED)
+				blip->li_flags &= ~XFS_LI_FAILED;
 		}
 
 		if (mlip_changed) {

Re: [PATCH 2/3 V2] xfs: Add infrastructure needed for error propagation during buffer IO failure

[Brian Foster]

On Wed, May 24, 2017 at 01:07:09PM -0400, Brian Foster wrote:
> On Mon, May 22, 2017 at 05:32:19PM +0200, Carlos Maiolino wrote:
> > With the current code, XFS never re-submit a failed buffer for IO,
> > because the failed item in the buffer is kept in the flush locked state
> > forever.
> > 
> > To be able to resubmit an log item for IO, we need a way to mark an item
> > as failed, if, for any reason the buffer which the item belonged to
> > failed during writeback.
> > 
> > Add a new log item callback to be used after an IO completion failure
> > and make the needed clean ups.
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> > V2:
> > 	- Update commit log to include a better description of why this
> > 	  patch is needed and fix spelling mistakes
> > 	- Move xfs_buf_do_callbacks_fail() call into
> > 	  xfs_buf_iodone_callback_error, so the callbacks can be executed
> > 	  before the buffer is released, and only after it has been
> > 	  retried once
> > 
> >  fs/xfs/xfs_buf_item.c | 27 ++++++++++++++++++++++++++-
> >  fs/xfs/xfs_trans.h    |  5 ++++-
> >  2 files changed, 30 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> > index 6ac3816..8f128e3 100644
> > --- a/fs/xfs/xfs_buf_item.c
> > +++ b/fs/xfs/xfs_buf_item.c
> > @@ -1051,6 +1051,24 @@ xfs_buf_do_callbacks(
> >  	}
> >  }
> >  
> > +STATIC void
> > +xfs_buf_do_callbacks_fail(
> > +	struct xfs_buf		*bp)
> > +{
> > +	struct xfs_log_item	*lip, *next;
> > +	unsigned int		bflags = bp->b_flags;
> > +
> > +	lip = bp->b_fspriv;
> > +	while (lip != NULL) {
> > +		next = lip->li_bio_list;
> > +
> > +		if (lip->li_ops->iop_error)
> > +			lip->li_ops->iop_error(lip, bflags);
> > +
> > +		lip = next;
> > +	}
> 
> AFAICT, this could do something like the following:
> 
> 	spin_lock(&ailp->xa_lock);
> 	while (lip != NULL) {
> 		next = lip->li_bio_list;
> 		lip->li_flags |= XFS_LI_FAILED;
> 		lip = next;
> 	}
> 	spin_unlock(&ailp->xa_lock);
> 
> ... to generically and unconditionally flag the log item as failed and
> avoid the need for ->iop_error(). We also need to clear XFS_LI_FAILED at
> the same place we clear XFS_LI_IN_AIL (i.e., AIL removal) to ensure a
> subsequent successful I/O completion updates the log item appropriately.
> 
> Then the result of this patch is that all log items are flagged as
> failed on I/O error until they are ultimately removed from the AIL. We
> otherwise have so far not changed behavior in any way.
> 

After poking around with this the past day or so, I think there is
another problem that we have to deal with (two, actually) in this area.

The first is not a problem introduced by these patches, but like the
whole flush lock thing, is a flaw in the whole "rely on the AIL to
retry" behavior that is already in place. The issue is that once we
flush a non-buffer log item to the backing buffer and the buffer I/O
happens to fail, we don't hold a reference on the buffer. This means
that we could flush an inode, the backing buffer I/O could fail (we flag
the log item as such), and the buffer could be reclaimed before the next
push of the inode log item. The LI_FAILED inode log item has already
been flushed, so we basically just read the buffer off disk and write it
back, having lost the inode changes that were flushed in the first
place. This is a data corruption vector, so I think needs to be fixed
before we unwind the flush lock livelock.

This isn't a problem with buffer log items that fail because the
xfs_buf_log_item holds a reference to the buffer. I think the solution
for inode/dquot log items is therefore fairly straightforward: do
something similar where the log item holds a reference to the buf and
releases it at the appropriate point. This could be done generically to
xfs_log_item, to the type-specific log item, and at push/flush time or
only for LI_FAILED items that are going back to the AIL[1]. IMO, the
latter seems like the least invasive approach to start with.

The second problem here is somewhat of a side effect of the first. It's
not sufficient to simply pin the buffer in-core (without further code
changes, at least) because if the filesystem shuts down with failed
items in the AIL, the buffer read path is short circuited to return
error. This means that the xfs_imap_to_bp() call in the LI_FAILED
handling path is simply not safe. If the filesystem has shut down due to
unrelated reasons, it returns -EIO and we end up in a similar livelock
as with the flush lock problem.

I think this can also be addressed by the solution noted above by also
holding a pointer to the underlying xfs_buf in the LI_FAILED log item
(which is also something that xfs_buf_log_item has today). That
basically means that any LI_FAILED log item should have an ->*li_buf
pointer that can be directly referenced to requeue the buffer to the
delwri queue. I think that should clean everything up properly in the
shutdown case as the I/O submission sets an error on the buf, runs the
ioend processing and the buf error handling invokes the callbacks now
that the fs is shutdown. Thoughts..?

Brian

[1] Note that if we end up changing behavior on error and on the
type-specific log item, this could reintroduce the need for
->iop_error(). Sorry for being back and forth on that, I basically just
think we should keep it if the implementation needs to access the
xfs_inode_log_item from the xfs_log_item or kill it otherwise.

> Brian
> 
> > +}
> > +
> >  static bool
> >  xfs_buf_iodone_callback_error(
> >  	struct xfs_buf		*bp)
> > @@ -1101,6 +1119,7 @@ xfs_buf_iodone_callback_error(
> >  
> >  		xfs_buf_ioerror(bp, 0);
> >  		xfs_buf_submit(bp);
> > +
> >  		return true;
> >  	}
> >  
> > @@ -1120,8 +1139,14 @@ xfs_buf_iodone_callback_error(
> >  	if ((mp->m_flags & XFS_MOUNT_UNMOUNTING) && mp->m_fail_unmount)
> >  		goto permanent_error;
> >  
> > -	/* still a transient error, higher layers will retry */
> > +	/*
> > +	 * still a transient error, run IO completion failure callbacks and
> > +	 * let the higher layers retry the buffer.
> > +	 * */
> >  	xfs_buf_ioerror(bp, 0);
> > +
> > +	/* run failure callbacks before releasing buffer */
> > +	xfs_buf_do_callbacks_fail(bp);
> >  	xfs_buf_relse(bp);
> >  	return true;
> >  
> > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> > index 7ae04de..7fcf48d 100644
> > --- a/fs/xfs/xfs_trans.h
> > +++ b/fs/xfs/xfs_trans.h
> > @@ -65,10 +65,12 @@ typedef struct xfs_log_item {
> >  
> >  #define	XFS_LI_IN_AIL	0x1
> >  #define XFS_LI_ABORTED	0x2
> > +#define XFS_LI_FAILED	0x3
> >  
> >  #define XFS_LI_FLAGS \
> >  	{ XFS_LI_IN_AIL,	"IN_AIL" }, \
> > -	{ XFS_LI_ABORTED,	"ABORTED" }
> > +	{ XFS_LI_ABORTED,	"ABORTED" }, \
> > +	{ XFS_LI_FAILED,	"FAILED" }
> >  
> >  struct xfs_item_ops {
> >  	void (*iop_size)(xfs_log_item_t *, int *, int *);
> > @@ -79,6 +81,7 @@ struct xfs_item_ops {
> >  	void (*iop_unlock)(xfs_log_item_t *);
> >  	xfs_lsn_t (*iop_committed)(xfs_log_item_t *, xfs_lsn_t);
> >  	void (*iop_committing)(xfs_log_item_t *, xfs_lsn_t);
> > +	void (*iop_error)(xfs_log_item_t *, unsigned int bflags);
> >  };
> >  
> >  void	xfs_log_item_init(struct xfs_mount *mp, struct xfs_log_item *item,
> > -- 
> > 2.9.3
> > 
> > --
> > 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
> --
> 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 1/3] xfs: use atomic operations to handle xfs_log_item flags

[Carlos Maiolino]

On Wed, May 24, 2017 at 01:06:41PM -0400, Brian Foster wrote:
> On Mon, May 22, 2017 at 05:32:18PM +0200, Carlos Maiolino wrote:
> > In order to fix a bug during buffer retries, a new flag type will be
> > added to xfs_log_item, and such operations need to be atomic.
> > 
> > Change all operations in xfs_log_item flags to atomic operations
> > 
> > To use atomic operations, xfs_log_item->li_flags also needed to be
> > converted to unsigned long type.
> > 
> > There is a small whitespace cleanup in the patch too
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> 
> As noted in the v1 discussion, I don't think this needs to be a
> dependency of the bug fix patch. Provided we can alleviate Dave's
> concerns over performance and not add any ->xa_lock acquisitions that
> aren't triggered by I/O errors, can we fix the bug using ->xa_lock and
> then port this on top? That way we have more of a backportable fix for
> older kernels affected by this problem.
> 

Hmm, I'm doing some tests with ->xa_lock, and I am actually deadlocking the
system when I try to acquire it in xfs_inode_item_push(), I wonder if ->xa_lock
is really correct to be used here, or maybe I'm doing something wrong? Still
checking what's going on though.

> (Please see my comments on patches 2 and 3 wrt proposed changes to avoid
> the custom callback and need for atomic flags..)
> 
> Brian
> 
> >  fs/xfs/xfs_bmap_item.c     |  4 ++--
> >  fs/xfs/xfs_buf_item.c      |  4 ++--
> >  fs/xfs/xfs_dquot.c         |  2 +-
> >  fs/xfs/xfs_extfree_item.c  |  4 ++--
> >  fs/xfs/xfs_icache.c        |  2 +-
> >  fs/xfs/xfs_icreate_item.c  |  2 +-
> >  fs/xfs/xfs_inode.c         |  4 ++--
> >  fs/xfs/xfs_inode_item.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         |  4 ++--
> >  fs/xfs/xfs_trans.c         |  4 ++--
> >  fs/xfs/xfs_trans.h         |  2 +-
> >  fs/xfs/xfs_trans_ail.c     | 12 ++++++------
> >  fs/xfs/xfs_trans_buf.c     |  2 +-
> >  fs/xfs/xfs_trans_priv.h    |  2 +-
> >  17 files changed, 30 insertions(+), 30 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> > index d419d23..9ebdca9 100644
> > --- a/fs/xfs/xfs_bmap_item.c
> > +++ b/fs/xfs/xfs_bmap_item.c
> > @@ -141,7 +141,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_item_free(BUI_ITEM(lip));
> >  }
> >  
> > @@ -304,7 +304,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 0306168..6ac3816 100644
> > --- a/fs/xfs/xfs_buf_item.c
> > +++ b/fs/xfs/xfs_buf_item.c
> > @@ -587,7 +587,7 @@ xfs_buf_item_unlock(
> >  	 * (cancelled) buffers at unpin time, but we'll never go through the
> >  	 * pin/unpin cycle if we abort inside commit.
> >  	 */
> > -	aborted = (lip->li_flags & XFS_LI_ABORTED) ? true : false;
> > +	aborted = (test_bit(XFS_LI_ABORTED, &lip->li_flags)) ? true : false;
> >  	/*
> >  	 * Before possibly freeing the buf item, copy the per-transaction state
> >  	 * so we can reference it safely later after clearing it from the
> > @@ -975,7 +975,7 @@ xfs_buf_item_relse(
> >  	xfs_buf_log_item_t	*bip = bp->b_fspriv;
> >  
> >  	trace_xfs_buf_item_relse(bp, _RET_IP_);
> > -	ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
> > +	ASSERT(!(test_bit(XFS_LI_IN_AIL, &bip->bli_item.li_flags)));
> >  
> >  	bp->b_fspriv = bip->bli_item.li_bio_list;
> >  	if (bp->b_fspriv == NULL)
> > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > index 9d06cc3..e8f2cbc 100644
> > --- a/fs/xfs/xfs_dquot.c
> > +++ b/fs/xfs/xfs_dquot.c
> > @@ -1003,7 +1003,7 @@ 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) {
> >  
> >  		/* xfs_trans_ail_delete() drops the AIL lock. */
> > diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> > index 44f8c54..32a519d 100644
> > --- a/fs/xfs/xfs_extfree_item.c
> > +++ b/fs/xfs/xfs_extfree_item.c
> > @@ -150,7 +150,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_item_free(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 f61c84f8..23d750f 100644
> > --- a/fs/xfs/xfs_icache.c
> > +++ b/fs/xfs/xfs_icache.c
> > @@ -107,7 +107,7 @@ 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 865ad13..e24cf83 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, &icp->ic_item.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 ec9826c..208c8c7 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -504,7 +504,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++;
> >  			}
> >  		}
> > @@ -601,7 +601,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(lock_mode, 1))) {
> >  			xfs_iunlock(ip0, lock_mode);
> >  			if ((++attempts % 5) == 0)
> > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> > index 08cb7d1..eeeadbb 100644
> > --- a/fs/xfs/xfs_inode_item.c
> > +++ b/fs/xfs/xfs_inode_item.c
> > @@ -783,7 +783,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_qm.c b/fs/xfs/xfs_qm.c
> > index 5fe6e70..da58263 100644
> > --- a/fs/xfs/xfs_qm.c
> > +++ b/fs/xfs/xfs_qm.c
> > @@ -169,7 +169,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 96fe209..5ecfd04 100644
> > --- a/fs/xfs/xfs_refcount_item.c
> > +++ b/fs/xfs/xfs_refcount_item.c
> > @@ -139,7 +139,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_item_free(CUI_ITEM(lip));
> >  }
> >  
> > @@ -308,7 +308,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 f3b139c..ada5ec7 100644
> > --- a/fs/xfs/xfs_rmap_item.c
> > +++ b/fs/xfs/xfs_rmap_item.c
> > @@ -139,7 +139,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_item_free(RUI_ITEM(lip));
> >  }
> >  
> > @@ -330,7 +330,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 7c5a165..d09e539 100644
> > --- a/fs/xfs/xfs_trace.h
> > +++ b/fs/xfs/xfs_trace.h
> > @@ -1031,7 +1031,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(
> > @@ -1083,7 +1083,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 be86e4e..6c8f492 100644
> > --- a/fs/xfs/xfs_trans.c
> > +++ b/fs/xfs/xfs_trans.c
> > @@ -764,7 +764,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);
> > @@ -835,7 +835,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 a07acbf..7ae04de 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_log_item		*li_bio_list;	/* buffer item list */
> >  	void				(*li_cb)(struct xfs_buf *,
> >  						 struct xfs_log_item *);
> > diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> > index 9056c0f..76e0de7 100644
> > --- a/fs/xfs/xfs_trans_ail.c
> > +++ b/fs/xfs/xfs_trans_ail.c
> > @@ -45,7 +45,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->xa_ail)
> >  		ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0);
> > @@ -653,7 +653,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_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;
> > @@ -663,7 +663,7 @@ xfs_trans_ail_update_bulk(
> >  			if (mlip == lip)
> >  				mlip_changed = 1;
> >  		} else {
> > -			lip->li_flags |= XFS_LI_IN_AIL;
> > +			set_bit(XFS_LI_IN_AIL, &lip->li_flags);
> >  			trace_xfs_ail_insert(lip, 0, lsn);
> >  		}
> >  		lip->li_lsn = lsn;
> > @@ -687,13 +687,13 @@ xfs_trans_ail_update_bulk(
> >  bool
> >  xfs_ail_delete_one(
> >  	struct xfs_ail		*ailp,
> > -	struct xfs_log_item 	*lip)
> > +	struct xfs_log_item	*lip)
> >  {
> >  	struct xfs_log_item	*mlip = xfs_ail_min(ailp);
> >  
> >  	trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn);
> >  	xfs_ail_delete(ailp, lip);
> > -	lip->li_flags &= ~XFS_LI_IN_AIL;
> > +	clear_bit(XFS_LI_IN_AIL, &lip->li_flags);
> >  	lip->li_lsn = 0;
> >  
> >  	return mlip == lip;
> > @@ -729,7 +729,7 @@ xfs_trans_ail_delete(
> >  	struct xfs_mount	*mp = ailp->xa_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->xa_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 8ee29ca..15814b5 100644
> > --- a/fs/xfs/xfs_trans_buf.c
> > +++ b/fs/xfs/xfs_trans_buf.c
> > @@ -433,7 +433,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> >  		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 d91706c..82ea000 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->xa_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->xa_lock);
> > -- 
> > 2.9.3
> > 
> > --
> > 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

-- 
Carlos

Re: [PATCH 1/3] xfs: use atomic operations to handle xfs_log_item flags

[Carlos Maiolino]

On Mon, Jun 05, 2017 at 02:54:33PM +0200, Carlos Maiolino wrote:
> On Wed, May 24, 2017 at 01:06:41PM -0400, Brian Foster wrote:
> > On Mon, May 22, 2017 at 05:32:18PM +0200, Carlos Maiolino wrote:
> > > In order to fix a bug during buffer retries, a new flag type will be
> > > added to xfs_log_item, and such operations need to be atomic.
> > > 
> > > Change all operations in xfs_log_item flags to atomic operations
> > > 
> > > To use atomic operations, xfs_log_item->li_flags also needed to be
> > > converted to unsigned long type.
> > > 
> > > There is a small whitespace cleanup in the patch too
> > > 
> > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > > ---
> > 
> > As noted in the v1 discussion, I don't think this needs to be a
> > dependency of the bug fix patch. Provided we can alleviate Dave's
> > concerns over performance and not add any ->xa_lock acquisitions that
> > aren't triggered by I/O errors, can we fix the bug using ->xa_lock and
> > then port this on top? That way we have more of a backportable fix for
> > older kernels affected by this problem.
> > 
> 
> Hmm, I'm doing some tests with ->xa_lock, and I am actually deadlocking the
> system when I try to acquire it in xfs_inode_item_push(), I wonder if ->xa_lock
> is really correct to be used here, or maybe I'm doing something wrong? Still
> checking what's going on though.
> 

My bad actually, I didn't realize xa_lock was already acquired when
re-submitting a failed buffer, at least for resubmission, it's already protected

> > (Please see my comments on patches 2 and 3 wrt proposed changes to avoid
> > the custom callback and need for atomic flags..)
> > 
> > Brian
> > 
> > >  fs/xfs/xfs_bmap_item.c     |  4 ++--
> > >  fs/xfs/xfs_buf_item.c      |  4 ++--
> > >  fs/xfs/xfs_dquot.c         |  2 +-
> > >  fs/xfs/xfs_extfree_item.c  |  4 ++--
> > >  fs/xfs/xfs_icache.c        |  2 +-
> > >  fs/xfs/xfs_icreate_item.c  |  2 +-
> > >  fs/xfs/xfs_inode.c         |  4 ++--
> > >  fs/xfs/xfs_inode_item.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         |  4 ++--
> > >  fs/xfs/xfs_trans.c         |  4 ++--
> > >  fs/xfs/xfs_trans.h         |  2 +-
> > >  fs/xfs/xfs_trans_ail.c     | 12 ++++++------
> > >  fs/xfs/xfs_trans_buf.c     |  2 +-
> > >  fs/xfs/xfs_trans_priv.h    |  2 +-
> > >  17 files changed, 30 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> > > index d419d23..9ebdca9 100644
> > > --- a/fs/xfs/xfs_bmap_item.c
> > > +++ b/fs/xfs/xfs_bmap_item.c
> > > @@ -141,7 +141,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_item_free(BUI_ITEM(lip));
> > >  }
> > >  
> > > @@ -304,7 +304,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 0306168..6ac3816 100644
> > > --- a/fs/xfs/xfs_buf_item.c
> > > +++ b/fs/xfs/xfs_buf_item.c
> > > @@ -587,7 +587,7 @@ xfs_buf_item_unlock(
> > >  	 * (cancelled) buffers at unpin time, but we'll never go through the
> > >  	 * pin/unpin cycle if we abort inside commit.
> > >  	 */
> > > -	aborted = (lip->li_flags & XFS_LI_ABORTED) ? true : false;
> > > +	aborted = (test_bit(XFS_LI_ABORTED, &lip->li_flags)) ? true : false;
> > >  	/*
> > >  	 * Before possibly freeing the buf item, copy the per-transaction state
> > >  	 * so we can reference it safely later after clearing it from the
> > > @@ -975,7 +975,7 @@ xfs_buf_item_relse(
> > >  	xfs_buf_log_item_t	*bip = bp->b_fspriv;
> > >  
> > >  	trace_xfs_buf_item_relse(bp, _RET_IP_);
> > > -	ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
> > > +	ASSERT(!(test_bit(XFS_LI_IN_AIL, &bip->bli_item.li_flags)));
> > >  
> > >  	bp->b_fspriv = bip->bli_item.li_bio_list;
> > >  	if (bp->b_fspriv == NULL)
> > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > > index 9d06cc3..e8f2cbc 100644
> > > --- a/fs/xfs/xfs_dquot.c
> > > +++ b/fs/xfs/xfs_dquot.c
> > > @@ -1003,7 +1003,7 @@ 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) {
> > >  
> > >  		/* xfs_trans_ail_delete() drops the AIL lock. */
> > > diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> > > index 44f8c54..32a519d 100644
> > > --- a/fs/xfs/xfs_extfree_item.c
> > > +++ b/fs/xfs/xfs_extfree_item.c
> > > @@ -150,7 +150,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_item_free(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 f61c84f8..23d750f 100644
> > > --- a/fs/xfs/xfs_icache.c
> > > +++ b/fs/xfs/xfs_icache.c
> > > @@ -107,7 +107,7 @@ 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 865ad13..e24cf83 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, &icp->ic_item.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 ec9826c..208c8c7 100644
> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > > @@ -504,7 +504,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++;
> > >  			}
> > >  		}
> > > @@ -601,7 +601,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(lock_mode, 1))) {
> > >  			xfs_iunlock(ip0, lock_mode);
> > >  			if ((++attempts % 5) == 0)
> > > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> > > index 08cb7d1..eeeadbb 100644
> > > --- a/fs/xfs/xfs_inode_item.c
> > > +++ b/fs/xfs/xfs_inode_item.c
> > > @@ -783,7 +783,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_qm.c b/fs/xfs/xfs_qm.c
> > > index 5fe6e70..da58263 100644
> > > --- a/fs/xfs/xfs_qm.c
> > > +++ b/fs/xfs/xfs_qm.c
> > > @@ -169,7 +169,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 96fe209..5ecfd04 100644
> > > --- a/fs/xfs/xfs_refcount_item.c
> > > +++ b/fs/xfs/xfs_refcount_item.c
> > > @@ -139,7 +139,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_item_free(CUI_ITEM(lip));
> > >  }
> > >  
> > > @@ -308,7 +308,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 f3b139c..ada5ec7 100644
> > > --- a/fs/xfs/xfs_rmap_item.c
> > > +++ b/fs/xfs/xfs_rmap_item.c
> > > @@ -139,7 +139,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_item_free(RUI_ITEM(lip));
> > >  }
> > >  
> > > @@ -330,7 +330,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 7c5a165..d09e539 100644
> > > --- a/fs/xfs/xfs_trace.h
> > > +++ b/fs/xfs/xfs_trace.h
> > > @@ -1031,7 +1031,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(
> > > @@ -1083,7 +1083,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 be86e4e..6c8f492 100644
> > > --- a/fs/xfs/xfs_trans.c
> > > +++ b/fs/xfs/xfs_trans.c
> > > @@ -764,7 +764,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);
> > > @@ -835,7 +835,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 a07acbf..7ae04de 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_log_item		*li_bio_list;	/* buffer item list */
> > >  	void				(*li_cb)(struct xfs_buf *,
> > >  						 struct xfs_log_item *);
> > > diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> > > index 9056c0f..76e0de7 100644
> > > --- a/fs/xfs/xfs_trans_ail.c
> > > +++ b/fs/xfs/xfs_trans_ail.c
> > > @@ -45,7 +45,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->xa_ail)
> > >  		ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0);
> > > @@ -653,7 +653,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_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;
> > > @@ -663,7 +663,7 @@ xfs_trans_ail_update_bulk(
> > >  			if (mlip == lip)
> > >  				mlip_changed = 1;
> > >  		} else {
> > > -			lip->li_flags |= XFS_LI_IN_AIL;
> > > +			set_bit(XFS_LI_IN_AIL, &lip->li_flags);
> > >  			trace_xfs_ail_insert(lip, 0, lsn);
> > >  		}
> > >  		lip->li_lsn = lsn;
> > > @@ -687,13 +687,13 @@ xfs_trans_ail_update_bulk(
> > >  bool
> > >  xfs_ail_delete_one(
> > >  	struct xfs_ail		*ailp,
> > > -	struct xfs_log_item 	*lip)
> > > +	struct xfs_log_item	*lip)
> > >  {
> > >  	struct xfs_log_item	*mlip = xfs_ail_min(ailp);
> > >  
> > >  	trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn);
> > >  	xfs_ail_delete(ailp, lip);
> > > -	lip->li_flags &= ~XFS_LI_IN_AIL;
> > > +	clear_bit(XFS_LI_IN_AIL, &lip->li_flags);
> > >  	lip->li_lsn = 0;
> > >  
> > >  	return mlip == lip;
> > > @@ -729,7 +729,7 @@ xfs_trans_ail_delete(
> > >  	struct xfs_mount	*mp = ailp->xa_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->xa_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 8ee29ca..15814b5 100644
> > > --- a/fs/xfs/xfs_trans_buf.c
> > > +++ b/fs/xfs/xfs_trans_buf.c
> > > @@ -433,7 +433,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > >  		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 d91706c..82ea000 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->xa_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->xa_lock);
> > > -- 
> > > 2.9.3
> > > 
> > > --
> > > 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
> 
> -- 
> Carlos
> --
> 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

-- 
Carlos