* [PATCH v4 01/17] xfs: refactor failed buffer resubmission into xfsaild
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
@ 2020-05-04 14:11 ` Brian Foster
2020-05-04 14:11 ` [PATCH v4 02/17] xfs: factor out buffer I/O failure code Brian Foster
` (16 subsequent siblings)
17 siblings, 0 replies; 37+ messages in thread
From: Brian Foster @ 2020-05-04 14:11 UTC (permalink / raw)
To: linux-xfs
Flush locked log items whose underlying buffers fail metadata
writeback are tagged with a special flag to indicate that the flush
lock is already held. This is currently implemented in the type
specific ->iop_push() callback, but the processing required for such
items is not type specific because we're only doing basic state
management on the underlying buffer.
Factor the failed log item handling out of the inode and dquot
->iop_push() callbacks and open code the buffer resubmit helper into
a single helper called from xfsaild_push_item(). This provides a
generic mechanism for handling failed metadata buffer writeback with
a bit less code.
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/xfs_buf_item.c | 39 ---------------------------------------
fs/xfs/xfs_buf_item.h | 2 --
fs/xfs/xfs_dquot_item.c | 15 ---------------
fs/xfs/xfs_inode_item.c | 15 ---------------
fs/xfs/xfs_trans_ail.c | 41 +++++++++++++++++++++++++++++++++++++++++
5 files changed, 41 insertions(+), 71 deletions(-)
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 1545657c3ca0..8796adde2d12 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -1248,42 +1248,3 @@ 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.
- *
- * We clear the log item failed state here as well, but we have to be careful
- * about reference counts because the only active reference counts on the buffer
- * may be the failed log items. Hence if we clear the log item failed state
- * before queuing the buffer for IO we can release all active references to
- * the buffer and free it, leading to use after free problems in
- * xfs_buf_delwri_queue. It makes no difference to the buffer or log items which
- * order we process them in - the buffer is locked, and we own the buffer list
- * so nothing on them is going to change while we are performing this action.
- *
- * Hence we can safely queue the buffer for IO before we clear the failed log
- * item state, therefore always having an active reference to the buffer and
- * avoiding the transient zero-reference state that leads to use-after-free.
- *
- * Return true if the buffer was added to the buffer list, false if it was
- * already on the buffer list.
- */
-bool
-xfs_buf_resubmit_failed_buffers(
- struct xfs_buf *bp,
- struct list_head *buffer_list)
-{
- struct xfs_log_item *lip;
- bool ret;
-
- ret = xfs_buf_delwri_queue(bp, buffer_list);
-
- /*
- * XFS_LI_FAILED set/clear is protected by ail_lock, caller of this
- * function already have it acquired
- */
- list_for_each_entry(lip, &bp->b_li_list, li_bio_list)
- xfs_clear_li_failed(lip);
-
- return ret;
-}
diff --git a/fs/xfs/xfs_buf_item.h b/fs/xfs/xfs_buf_item.h
index 30114b510332..c9c57e2da932 100644
--- a/fs/xfs/xfs_buf_item.h
+++ b/fs/xfs/xfs_buf_item.h
@@ -59,8 +59,6 @@ void xfs_buf_attach_iodone(struct xfs_buf *,
struct xfs_log_item *);
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_buf *,
- struct list_head *);
bool xfs_buf_log_check_iovec(struct xfs_log_iovec *iovec);
extern kmem_zone_t *xfs_buf_item_zone;
diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
index baad1748d0d1..5a7808299a32 100644
--- a/fs/xfs/xfs_dquot_item.c
+++ b/fs/xfs/xfs_dquot_item.c
@@ -145,21 +145,6 @@ xfs_qm_dquot_logitem_push(
if (atomic_read(&dqp->q_pincount) > 0)
return XFS_ITEM_PINNED;
- /*
- * The buffer containing this item failed to be written back
- * previously. Resubmit the buffer for IO
- */
- if (test_bit(XFS_LI_FAILED, &lip->li_flags)) {
- if (!xfs_buf_trylock(bp))
- return XFS_ITEM_LOCKED;
-
- if (!xfs_buf_resubmit_failed_buffers(bp, buffer_list))
- rval = XFS_ITEM_FLUSHING;
-
- xfs_buf_unlock(bp);
- return rval;
- }
-
if (!xfs_dqlock_nowait(dqp))
return XFS_ITEM_LOCKED;
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index f779cca2346f..1d4d256a2e96 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -497,21 +497,6 @@ xfs_inode_item_push(
if (xfs_ipincount(ip) > 0)
return XFS_ITEM_PINNED;
- /*
- * The buffer containing this item failed to be written back
- * previously. Resubmit the buffer for IO.
- */
- if (test_bit(XFS_LI_FAILED, &lip->li_flags)) {
- if (!xfs_buf_trylock(bp))
- return XFS_ITEM_LOCKED;
-
- if (!xfs_buf_resubmit_failed_buffers(bp, buffer_list))
- rval = XFS_ITEM_FLUSHING;
-
- xfs_buf_unlock(bp);
- return rval;
- }
-
if (!xfs_ilock_nowait(ip, XFS_ILOCK_SHARED))
return XFS_ITEM_LOCKED;
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 564253550b75..2574d01e4a83 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -345,6 +345,45 @@ xfs_ail_delete(
xfs_trans_ail_cursor_clear(ailp, lip);
}
+/*
+ * Requeue a failed buffer for writeback.
+ *
+ * We clear the log item failed state here as well, but we have to be careful
+ * about reference counts because the only active reference counts on the buffer
+ * may be the failed log items. Hence if we clear the log item failed state
+ * before queuing the buffer for IO we can release all active references to
+ * the buffer and free it, leading to use after free problems in
+ * xfs_buf_delwri_queue. It makes no difference to the buffer or log items which
+ * order we process them in - the buffer is locked, and we own the buffer list
+ * so nothing on them is going to change while we are performing this action.
+ *
+ * Hence we can safely queue the buffer for IO before we clear the failed log
+ * item state, therefore always having an active reference to the buffer and
+ * avoiding the transient zero-reference state that leads to use-after-free.
+ */
+static inline int
+xfsaild_resubmit_item(
+ struct xfs_log_item *lip,
+ struct list_head *buffer_list)
+{
+ struct xfs_buf *bp = lip->li_buf;
+
+ if (!xfs_buf_trylock(bp))
+ return XFS_ITEM_LOCKED;
+
+ if (!xfs_buf_delwri_queue(bp, buffer_list)) {
+ xfs_buf_unlock(bp);
+ return XFS_ITEM_FLUSHING;
+ }
+
+ /* protected by ail_lock */
+ list_for_each_entry(lip, &bp->b_li_list, li_bio_list)
+ xfs_clear_li_failed(lip);
+
+ xfs_buf_unlock(bp);
+ return XFS_ITEM_SUCCESS;
+}
+
static inline uint
xfsaild_push_item(
struct xfs_ail *ailp,
@@ -365,6 +404,8 @@ xfsaild_push_item(
*/
if (!lip->li_ops->iop_push)
return XFS_ITEM_PINNED;
+ if (test_bit(XFS_LI_FAILED, &lip->li_flags))
+ return xfsaild_resubmit_item(lip, &ailp->ail_buf_list);
return lip->li_ops->iop_push(lip, &ailp->ail_buf_list);
}
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v4 02/17] xfs: factor out buffer I/O failure code
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
2020-05-04 14:11 ` [PATCH v4 01/17] xfs: refactor failed buffer resubmission into xfsaild Brian Foster
@ 2020-05-04 14:11 ` Brian Foster
2020-05-04 14:11 ` [PATCH v4 03/17] xfs: simplify inode flush error handling Brian Foster
` (15 subsequent siblings)
17 siblings, 0 replies; 37+ messages in thread
From: Brian Foster @ 2020-05-04 14:11 UTC (permalink / raw)
To: linux-xfs
We use the same buffer I/O failure code in a few different places.
It's not much code, but it's not necessarily self-explanatory.
Factor it into a helper and document it in one place.
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_buf.c | 21 +++++++++++++++++----
fs/xfs/xfs_buf.h | 1 +
fs/xfs/xfs_buf_item.c | 21 +++------------------
fs/xfs/xfs_inode.c | 6 +-----
4 files changed, 22 insertions(+), 27 deletions(-)
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 9ec3eaf1c618..d5d6a68bb1e6 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1248,6 +1248,22 @@ xfs_buf_ioerror_alert(
-bp->b_error);
}
+/*
+ * To simulate an I/O failure, the buffer must be locked and held with at least
+ * three references. The LRU reference is dropped by the stale call. The buf
+ * item reference is dropped via ioend processing. The third reference is owned
+ * by the caller and is dropped on I/O completion if the buffer is XBF_ASYNC.
+ */
+void
+xfs_buf_ioend_fail(
+ struct xfs_buf *bp)
+{
+ bp->b_flags &= ~XBF_DONE;
+ xfs_buf_stale(bp);
+ xfs_buf_ioerror(bp, -EIO);
+ xfs_buf_ioend(bp);
+}
+
int
xfs_bwrite(
struct xfs_buf *bp)
@@ -1480,10 +1496,7 @@ __xfs_buf_submit(
/* on shutdown we stale and complete the buffer immediately */
if (XFS_FORCED_SHUTDOWN(bp->b_mount)) {
- xfs_buf_ioerror(bp, -EIO);
- bp->b_flags &= ~XBF_DONE;
- xfs_buf_stale(bp);
- xfs_buf_ioend(bp);
+ xfs_buf_ioend_fail(bp);
return -EIO;
}
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 9a04c53c2488..06ea3eef866e 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -263,6 +263,7 @@ extern void __xfs_buf_ioerror(struct xfs_buf *bp, int error,
xfs_failaddr_t failaddr);
#define xfs_buf_ioerror(bp, err) __xfs_buf_ioerror((bp), (err), __this_address)
extern void xfs_buf_ioerror_alert(struct xfs_buf *bp, xfs_failaddr_t fa);
+void xfs_buf_ioend_fail(struct xfs_buf *);
extern int __xfs_buf_submit(struct xfs_buf *bp, bool);
static inline int xfs_buf_submit(struct xfs_buf *bp)
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 8796adde2d12..b452a399a441 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -471,28 +471,13 @@ xfs_buf_item_unpin(
xfs_buf_relse(bp);
} else if (freed && remove) {
/*
- * There are currently two references to the buffer - the active
- * LRU reference and the buf log item. What we are about to do
- * here - simulate a failed IO completion - requires 3
- * references.
- *
- * The LRU reference is removed by the xfs_buf_stale() call. The
- * buf item reference is removed by the xfs_buf_iodone()
- * callback that is run by xfs_buf_do_callbacks() during ioend
- * processing (via the bp->b_iodone callback), and then finally
- * the ioend processing will drop the IO reference if the buffer
- * is marked XBF_ASYNC.
- *
- * Hence we need to take an additional reference here so that IO
- * completion processing doesn't free the buffer prematurely.
+ * The buffer must be locked and held by the caller to simulate
+ * an async I/O failure.
*/
xfs_buf_lock(bp);
xfs_buf_hold(bp);
bp->b_flags |= XBF_ASYNC;
- xfs_buf_ioerror(bp, -EIO);
- bp->b_flags &= ~XBF_DONE;
- xfs_buf_stale(bp);
- xfs_buf_ioend(bp);
+ xfs_buf_ioend_fail(bp);
}
}
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index d1772786af29..909ca7c0bac4 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3630,11 +3630,7 @@ xfs_iflush_cluster(
*/
ASSERT(bp->b_iodone);
bp->b_flags |= XBF_ASYNC;
- bp->b_flags &= ~XBF_DONE;
- xfs_buf_stale(bp);
- xfs_buf_ioerror(bp, -EIO);
- xfs_buf_ioend(bp);
-
+ xfs_buf_ioend_fail(bp);
xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
/* abort the corrupt inode, as it was not attached to the buffer */
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v4 03/17] xfs: simplify inode flush error handling
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
2020-05-04 14:11 ` [PATCH v4 01/17] xfs: refactor failed buffer resubmission into xfsaild Brian Foster
2020-05-04 14:11 ` [PATCH v4 02/17] xfs: factor out buffer I/O failure code Brian Foster
@ 2020-05-04 14:11 ` Brian Foster
2020-05-04 21:31 ` Darrick J. Wong
2020-05-05 21:09 ` Allison Collins
2020-05-04 14:11 ` [PATCH v4 04/17] xfs: remove unnecessary shutdown check from xfs_iflush() Brian Foster
` (14 subsequent siblings)
17 siblings, 2 replies; 37+ messages in thread
From: Brian Foster @ 2020-05-04 14:11 UTC (permalink / raw)
To: linux-xfs
The inode flush code has several layers of error handling between
the inode and cluster flushing code. If the inode flush fails before
acquiring the backing buffer, the inode flush is aborted. If the
cluster flush fails, the current inode flush is aborted and the
cluster buffer is failed to handle the initial inode and any others
that might have been attached before the error.
Since xfs_iflush() is the only caller of xfs_iflush_cluster(), the
error handling between the two can be condensed in the top-level
function. If we update xfs_iflush_int() to always fall through to
the log item update and attach the item completion handler to the
buffer, any errors that occur after the first call to
xfs_iflush_int() can be handled with a buffer I/O failure.
Lift the error handling from xfs_iflush_cluster() into xfs_iflush()
and consolidate with the existing error handling. This also replaces
the need to release the buffer because failing the buffer with
XBF_ASYNC drops the current reference.
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_inode.c | 117 +++++++++++++++++----------------------------
1 file changed, 45 insertions(+), 72 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 909ca7c0bac4..84f2ee9957dc 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3496,6 +3496,7 @@ xfs_iflush_cluster(
struct xfs_inode **cilist;
struct xfs_inode *cip;
struct xfs_ino_geometry *igeo = M_IGEO(mp);
+ int error = 0;
int nr_found;
int clcount = 0;
int i;
@@ -3588,11 +3589,10 @@ xfs_iflush_cluster(
* re-check that it's dirty before flushing.
*/
if (!xfs_inode_clean(cip)) {
- int error;
error = xfs_iflush_int(cip, bp);
if (error) {
xfs_iunlock(cip, XFS_ILOCK_SHARED);
- goto cluster_corrupt_out;
+ goto out_free;
}
clcount++;
} else {
@@ -3611,33 +3611,7 @@ xfs_iflush_cluster(
kmem_free(cilist);
out_put:
xfs_perag_put(pag);
- return 0;
-
-
-cluster_corrupt_out:
- /*
- * Corruption detected in the clustering loop. Invalidate the
- * inode buffer and shut down the filesystem.
- */
- rcu_read_unlock();
-
- /*
- * We'll always have an inode attached to the buffer for completion
- * process by the time we are called from xfs_iflush(). Hence we have
- * always need to do IO completion processing to abort the inodes
- * attached to the buffer. handle them just like the shutdown case in
- * xfs_buf_submit().
- */
- ASSERT(bp->b_iodone);
- bp->b_flags |= XBF_ASYNC;
- xfs_buf_ioend_fail(bp);
- xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
-
- /* abort the corrupt inode, as it was not attached to the buffer */
- xfs_iflush_abort(cip, false);
- kmem_free(cilist);
- xfs_perag_put(pag);
- return -EFSCORRUPTED;
+ return error;
}
/*
@@ -3693,17 +3667,16 @@ xfs_iflush(
*/
if (XFS_FORCED_SHUTDOWN(mp)) {
error = -EIO;
- goto abort_out;
+ goto abort;
}
/*
* Get the buffer containing the on-disk inode. We are doing a try-lock
- * operation here, so we may get an EAGAIN error. In that case, we
- * simply want to return with the inode still dirty.
+ * operation here, so we may get an EAGAIN error. In that case, return
+ * leaving the inode dirty.
*
* If we get any other error, we effectively have a corruption situation
- * and we cannot flush the inode, so we treat it the same as failing
- * xfs_iflush_int().
+ * and we cannot flush the inode. Abort the flush and shut down.
*/
error = xfs_imap_to_bp(mp, NULL, &ip->i_imap, &dip, &bp, XBF_TRYLOCK,
0);
@@ -3712,14 +3685,7 @@ xfs_iflush(
return error;
}
if (error)
- goto corrupt_out;
-
- /*
- * First flush out the inode that xfs_iflush was called with.
- */
- error = xfs_iflush_int(ip, bp);
- if (error)
- goto corrupt_out;
+ goto abort;
/*
* If the buffer is pinned then push on the log now so we won't
@@ -3729,28 +3695,29 @@ xfs_iflush(
xfs_log_force(mp, 0);
/*
- * inode clustering: try to gather other inodes into this write
+ * Flush the provided inode then attempt to gather others from the
+ * cluster into the write.
*
- * Note: Any error during clustering will result in the filesystem
- * being shut down and completion callbacks run on the cluster buffer.
- * As we have already flushed and attached this inode to the buffer,
- * it has already been aborted and released by xfs_iflush_cluster() and
- * so we have no further error handling to do here.
+ * Note: Once we attempt to flush an inode, we must run buffer
+ * completion callbacks on any failure. If this fails, simulate an I/O
+ * failure on the buffer and shut down.
*/
- error = xfs_iflush_cluster(ip, bp);
- if (error)
- return error;
+ error = xfs_iflush_int(ip, bp);
+ if (!error)
+ error = xfs_iflush_cluster(ip, bp);
+ if (error) {
+ bp->b_flags |= XBF_ASYNC;
+ xfs_buf_ioend_fail(bp);
+ goto shutdown;
+ }
*bpp = bp;
return 0;
-corrupt_out:
- if (bp)
- xfs_buf_relse(bp);
- xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
-abort_out:
- /* abort the corrupt inode, as it was not attached to the buffer */
+abort:
xfs_iflush_abort(ip, false);
+shutdown:
+ xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
return error;
}
@@ -3792,6 +3759,7 @@ xfs_iflush_int(
struct xfs_inode_log_item *iip = ip->i_itemp;
struct xfs_dinode *dip;
struct xfs_mount *mp = ip->i_mount;
+ int error;
ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
ASSERT(xfs_isiflocked(ip));
@@ -3799,15 +3767,21 @@ xfs_iflush_int(
ip->i_d.di_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK));
ASSERT(iip != NULL && iip->ili_fields != 0);
- /* set *dip = inode's place in the buffer */
dip = xfs_buf_offset(bp, ip->i_imap.im_boffset);
+ /*
+ * We don't flush the inode if any of the following checks fail, but we
+ * do still update the log item and attach to the backing buffer as if
+ * the flush happened. This is a formality to facilitate predictable
+ * error handling as the caller will shutdown and fail the buffer.
+ */
+ error = -EFSCORRUPTED;
if (XFS_TEST_ERROR(dip->di_magic != cpu_to_be16(XFS_DINODE_MAGIC),
mp, XFS_ERRTAG_IFLUSH_1)) {
xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
"%s: Bad inode %Lu magic number 0x%x, ptr "PTR_FMT,
__func__, ip->i_ino, be16_to_cpu(dip->di_magic), dip);
- goto corrupt_out;
+ goto flush_out;
}
if (S_ISREG(VFS_I(ip)->i_mode)) {
if (XFS_TEST_ERROR(
@@ -3817,7 +3791,7 @@ xfs_iflush_int(
xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
"%s: Bad regular inode %Lu, ptr "PTR_FMT,
__func__, ip->i_ino, ip);
- goto corrupt_out;
+ goto flush_out;
}
} else if (S_ISDIR(VFS_I(ip)->i_mode)) {
if (XFS_TEST_ERROR(
@@ -3828,7 +3802,7 @@ xfs_iflush_int(
xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
"%s: Bad directory inode %Lu, ptr "PTR_FMT,
__func__, ip->i_ino, ip);
- goto corrupt_out;
+ goto flush_out;
}
}
if (XFS_TEST_ERROR(ip->i_d.di_nextents + ip->i_d.di_anextents >
@@ -3839,14 +3813,14 @@ xfs_iflush_int(
__func__, ip->i_ino,
ip->i_d.di_nextents + ip->i_d.di_anextents,
ip->i_d.di_nblocks, ip);
- goto corrupt_out;
+ goto flush_out;
}
if (XFS_TEST_ERROR(ip->i_d.di_forkoff > mp->m_sb.sb_inodesize,
mp, XFS_ERRTAG_IFLUSH_6)) {
xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
"%s: bad inode %Lu, forkoff 0x%x, ptr "PTR_FMT,
__func__, ip->i_ino, ip->i_d.di_forkoff, ip);
- goto corrupt_out;
+ goto flush_out;
}
/*
@@ -3863,7 +3837,7 @@ xfs_iflush_int(
/* Check the inline fork data before we write out. */
if (!xfs_inode_verify_forks(ip))
- goto corrupt_out;
+ goto flush_out;
/*
* Copy the dirty parts of the inode into the on-disk inode. We always
@@ -3906,6 +3880,8 @@ xfs_iflush_int(
* need the AIL lock, because it is a 64 bit value that cannot be read
* atomically.
*/
+ error = 0;
+flush_out:
iip->ili_last_fields = iip->ili_fields;
iip->ili_fields = 0;
iip->ili_fsync_fields = 0;
@@ -3915,10 +3891,10 @@ xfs_iflush_int(
&iip->ili_item.li_lsn);
/*
- * Attach the function xfs_iflush_done to the inode's
- * buffer. This will remove the inode from the AIL
- * and unlock the inode's flush lock when the inode is
- * completely written to disk.
+ * Attach the inode item callback to the buffer whether the flush
+ * succeeded or not. If not, the caller will shut down and fail I/O
+ * completion on the buffer to remove the inode from the AIL and release
+ * the flush lock.
*/
xfs_buf_attach_iodone(bp, xfs_iflush_done, &iip->ili_item);
@@ -3927,10 +3903,7 @@ xfs_iflush_int(
ASSERT(!list_empty(&bp->b_li_list));
ASSERT(bp->b_iodone != NULL);
- return 0;
-
-corrupt_out:
- return -EFSCORRUPTED;
+ return error;
}
/* Release an inode. */
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 03/17] xfs: simplify inode flush error handling
2020-05-04 14:11 ` [PATCH v4 03/17] xfs: simplify inode flush error handling Brian Foster
@ 2020-05-04 21:31 ` Darrick J. Wong
2020-05-05 21:09 ` Allison Collins
1 sibling, 0 replies; 37+ messages in thread
From: Darrick J. Wong @ 2020-05-04 21:31 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Mon, May 04, 2020 at 10:11:40AM -0400, Brian Foster wrote:
> The inode flush code has several layers of error handling between
> the inode and cluster flushing code. If the inode flush fails before
> acquiring the backing buffer, the inode flush is aborted. If the
> cluster flush fails, the current inode flush is aborted and the
> cluster buffer is failed to handle the initial inode and any others
> that might have been attached before the error.
>
> Since xfs_iflush() is the only caller of xfs_iflush_cluster(), the
> error handling between the two can be condensed in the top-level
> function. If we update xfs_iflush_int() to always fall through to
> the log item update and attach the item completion handler to the
> buffer, any errors that occur after the first call to
> xfs_iflush_int() can be handled with a buffer I/O failure.
>
> Lift the error handling from xfs_iflush_cluster() into xfs_iflush()
> and consolidate with the existing error handling. This also replaces
> the need to release the buffer because failing the buffer with
> XBF_ASYNC drops the current reference.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Seems fine to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_inode.c | 117 +++++++++++++++++----------------------------
> 1 file changed, 45 insertions(+), 72 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 909ca7c0bac4..84f2ee9957dc 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3496,6 +3496,7 @@ xfs_iflush_cluster(
> struct xfs_inode **cilist;
> struct xfs_inode *cip;
> struct xfs_ino_geometry *igeo = M_IGEO(mp);
> + int error = 0;
> int nr_found;
> int clcount = 0;
> int i;
> @@ -3588,11 +3589,10 @@ xfs_iflush_cluster(
> * re-check that it's dirty before flushing.
> */
> if (!xfs_inode_clean(cip)) {
> - int error;
> error = xfs_iflush_int(cip, bp);
> if (error) {
> xfs_iunlock(cip, XFS_ILOCK_SHARED);
> - goto cluster_corrupt_out;
> + goto out_free;
> }
> clcount++;
> } else {
> @@ -3611,33 +3611,7 @@ xfs_iflush_cluster(
> kmem_free(cilist);
> out_put:
> xfs_perag_put(pag);
> - return 0;
> -
> -
> -cluster_corrupt_out:
> - /*
> - * Corruption detected in the clustering loop. Invalidate the
> - * inode buffer and shut down the filesystem.
> - */
> - rcu_read_unlock();
> -
> - /*
> - * We'll always have an inode attached to the buffer for completion
> - * process by the time we are called from xfs_iflush(). Hence we have
> - * always need to do IO completion processing to abort the inodes
> - * attached to the buffer. handle them just like the shutdown case in
> - * xfs_buf_submit().
> - */
> - ASSERT(bp->b_iodone);
> - bp->b_flags |= XBF_ASYNC;
> - xfs_buf_ioend_fail(bp);
> - xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> -
> - /* abort the corrupt inode, as it was not attached to the buffer */
> - xfs_iflush_abort(cip, false);
> - kmem_free(cilist);
> - xfs_perag_put(pag);
> - return -EFSCORRUPTED;
> + return error;
> }
>
> /*
> @@ -3693,17 +3667,16 @@ xfs_iflush(
> */
> if (XFS_FORCED_SHUTDOWN(mp)) {
> error = -EIO;
> - goto abort_out;
> + goto abort;
> }
>
> /*
> * Get the buffer containing the on-disk inode. We are doing a try-lock
> - * operation here, so we may get an EAGAIN error. In that case, we
> - * simply want to return with the inode still dirty.
> + * operation here, so we may get an EAGAIN error. In that case, return
> + * leaving the inode dirty.
> *
> * If we get any other error, we effectively have a corruption situation
> - * and we cannot flush the inode, so we treat it the same as failing
> - * xfs_iflush_int().
> + * and we cannot flush the inode. Abort the flush and shut down.
> */
> error = xfs_imap_to_bp(mp, NULL, &ip->i_imap, &dip, &bp, XBF_TRYLOCK,
> 0);
> @@ -3712,14 +3685,7 @@ xfs_iflush(
> return error;
> }
> if (error)
> - goto corrupt_out;
> -
> - /*
> - * First flush out the inode that xfs_iflush was called with.
> - */
> - error = xfs_iflush_int(ip, bp);
> - if (error)
> - goto corrupt_out;
> + goto abort;
>
> /*
> * If the buffer is pinned then push on the log now so we won't
> @@ -3729,28 +3695,29 @@ xfs_iflush(
> xfs_log_force(mp, 0);
>
> /*
> - * inode clustering: try to gather other inodes into this write
> + * Flush the provided inode then attempt to gather others from the
> + * cluster into the write.
> *
> - * Note: Any error during clustering will result in the filesystem
> - * being shut down and completion callbacks run on the cluster buffer.
> - * As we have already flushed and attached this inode to the buffer,
> - * it has already been aborted and released by xfs_iflush_cluster() and
> - * so we have no further error handling to do here.
> + * Note: Once we attempt to flush an inode, we must run buffer
> + * completion callbacks on any failure. If this fails, simulate an I/O
> + * failure on the buffer and shut down.
> */
> - error = xfs_iflush_cluster(ip, bp);
> - if (error)
> - return error;
> + error = xfs_iflush_int(ip, bp);
> + if (!error)
> + error = xfs_iflush_cluster(ip, bp);
> + if (error) {
> + bp->b_flags |= XBF_ASYNC;
> + xfs_buf_ioend_fail(bp);
> + goto shutdown;
> + }
>
> *bpp = bp;
> return 0;
>
> -corrupt_out:
> - if (bp)
> - xfs_buf_relse(bp);
> - xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> -abort_out:
> - /* abort the corrupt inode, as it was not attached to the buffer */
> +abort:
> xfs_iflush_abort(ip, false);
> +shutdown:
> + xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> return error;
> }
>
> @@ -3792,6 +3759,7 @@ xfs_iflush_int(
> struct xfs_inode_log_item *iip = ip->i_itemp;
> struct xfs_dinode *dip;
> struct xfs_mount *mp = ip->i_mount;
> + int error;
>
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> ASSERT(xfs_isiflocked(ip));
> @@ -3799,15 +3767,21 @@ xfs_iflush_int(
> ip->i_d.di_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK));
> ASSERT(iip != NULL && iip->ili_fields != 0);
>
> - /* set *dip = inode's place in the buffer */
> dip = xfs_buf_offset(bp, ip->i_imap.im_boffset);
>
> + /*
> + * We don't flush the inode if any of the following checks fail, but we
> + * do still update the log item and attach to the backing buffer as if
> + * the flush happened. This is a formality to facilitate predictable
> + * error handling as the caller will shutdown and fail the buffer.
> + */
> + error = -EFSCORRUPTED;
> if (XFS_TEST_ERROR(dip->di_magic != cpu_to_be16(XFS_DINODE_MAGIC),
> mp, XFS_ERRTAG_IFLUSH_1)) {
> xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
> "%s: Bad inode %Lu magic number 0x%x, ptr "PTR_FMT,
> __func__, ip->i_ino, be16_to_cpu(dip->di_magic), dip);
> - goto corrupt_out;
> + goto flush_out;
> }
> if (S_ISREG(VFS_I(ip)->i_mode)) {
> if (XFS_TEST_ERROR(
> @@ -3817,7 +3791,7 @@ xfs_iflush_int(
> xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
> "%s: Bad regular inode %Lu, ptr "PTR_FMT,
> __func__, ip->i_ino, ip);
> - goto corrupt_out;
> + goto flush_out;
> }
> } else if (S_ISDIR(VFS_I(ip)->i_mode)) {
> if (XFS_TEST_ERROR(
> @@ -3828,7 +3802,7 @@ xfs_iflush_int(
> xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
> "%s: Bad directory inode %Lu, ptr "PTR_FMT,
> __func__, ip->i_ino, ip);
> - goto corrupt_out;
> + goto flush_out;
> }
> }
> if (XFS_TEST_ERROR(ip->i_d.di_nextents + ip->i_d.di_anextents >
> @@ -3839,14 +3813,14 @@ xfs_iflush_int(
> __func__, ip->i_ino,
> ip->i_d.di_nextents + ip->i_d.di_anextents,
> ip->i_d.di_nblocks, ip);
> - goto corrupt_out;
> + goto flush_out;
> }
> if (XFS_TEST_ERROR(ip->i_d.di_forkoff > mp->m_sb.sb_inodesize,
> mp, XFS_ERRTAG_IFLUSH_6)) {
> xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
> "%s: bad inode %Lu, forkoff 0x%x, ptr "PTR_FMT,
> __func__, ip->i_ino, ip->i_d.di_forkoff, ip);
> - goto corrupt_out;
> + goto flush_out;
> }
>
> /*
> @@ -3863,7 +3837,7 @@ xfs_iflush_int(
>
> /* Check the inline fork data before we write out. */
> if (!xfs_inode_verify_forks(ip))
> - goto corrupt_out;
> + goto flush_out;
>
> /*
> * Copy the dirty parts of the inode into the on-disk inode. We always
> @@ -3906,6 +3880,8 @@ xfs_iflush_int(
> * need the AIL lock, because it is a 64 bit value that cannot be read
> * atomically.
> */
> + error = 0;
> +flush_out:
> iip->ili_last_fields = iip->ili_fields;
> iip->ili_fields = 0;
> iip->ili_fsync_fields = 0;
> @@ -3915,10 +3891,10 @@ xfs_iflush_int(
> &iip->ili_item.li_lsn);
>
> /*
> - * Attach the function xfs_iflush_done to the inode's
> - * buffer. This will remove the inode from the AIL
> - * and unlock the inode's flush lock when the inode is
> - * completely written to disk.
> + * Attach the inode item callback to the buffer whether the flush
> + * succeeded or not. If not, the caller will shut down and fail I/O
> + * completion on the buffer to remove the inode from the AIL and release
> + * the flush lock.
> */
> xfs_buf_attach_iodone(bp, xfs_iflush_done, &iip->ili_item);
>
> @@ -3927,10 +3903,7 @@ xfs_iflush_int(
>
> ASSERT(!list_empty(&bp->b_li_list));
> ASSERT(bp->b_iodone != NULL);
> - return 0;
> -
> -corrupt_out:
> - return -EFSCORRUPTED;
> + return error;
> }
>
> /* Release an inode. */
> --
> 2.21.1
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v4 03/17] xfs: simplify inode flush error handling
2020-05-04 14:11 ` [PATCH v4 03/17] xfs: simplify inode flush error handling Brian Foster
2020-05-04 21:31 ` Darrick J. Wong
@ 2020-05-05 21:09 ` Allison Collins
1 sibling, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-05-05 21:09 UTC (permalink / raw)
To: Brian Foster, linux-xfs
On 5/4/20 7:11 AM, Brian Foster wrote:
> The inode flush code has several layers of error handling between
> the inode and cluster flushing code. If the inode flush fails before
> acquiring the backing buffer, the inode flush is aborted. If the
> cluster flush fails, the current inode flush is aborted and the
> cluster buffer is failed to handle the initial inode and any others
> that might have been attached before the error.
>
> Since xfs_iflush() is the only caller of xfs_iflush_cluster(), the
> error handling between the two can be condensed in the top-level
> function. If we update xfs_iflush_int() to always fall through to
> the log item update and attach the item completion handler to the
> buffer, any errors that occur after the first call to
> xfs_iflush_int() can be handled with a buffer I/O failure.
>
> Lift the error handling from xfs_iflush_cluster() into xfs_iflush()
> and consolidate with the existing error handling. This also replaces
> the need to release the buffer because failing the buffer with
> XBF_ASYNC drops the current reference.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Ok, I think it looks good
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
> ---
> fs/xfs/xfs_inode.c | 117 +++++++++++++++++----------------------------
> 1 file changed, 45 insertions(+), 72 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 909ca7c0bac4..84f2ee9957dc 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3496,6 +3496,7 @@ xfs_iflush_cluster(
> struct xfs_inode **cilist;
> struct xfs_inode *cip;
> struct xfs_ino_geometry *igeo = M_IGEO(mp);
> + int error = 0;
> int nr_found;
> int clcount = 0;
> int i;
> @@ -3588,11 +3589,10 @@ xfs_iflush_cluster(
> * re-check that it's dirty before flushing.
> */
> if (!xfs_inode_clean(cip)) {
> - int error;
> error = xfs_iflush_int(cip, bp);
> if (error) {
> xfs_iunlock(cip, XFS_ILOCK_SHARED);
> - goto cluster_corrupt_out;
> + goto out_free;
> }
> clcount++;
> } else {
> @@ -3611,33 +3611,7 @@ xfs_iflush_cluster(
> kmem_free(cilist);
> out_put:
> xfs_perag_put(pag);
> - return 0;
> -
> -
> -cluster_corrupt_out:
> - /*
> - * Corruption detected in the clustering loop. Invalidate the
> - * inode buffer and shut down the filesystem.
> - */
> - rcu_read_unlock();
> -
> - /*
> - * We'll always have an inode attached to the buffer for completion
> - * process by the time we are called from xfs_iflush(). Hence we have
> - * always need to do IO completion processing to abort the inodes
> - * attached to the buffer. handle them just like the shutdown case in
> - * xfs_buf_submit().
> - */
> - ASSERT(bp->b_iodone);
> - bp->b_flags |= XBF_ASYNC;
> - xfs_buf_ioend_fail(bp);
> - xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> -
> - /* abort the corrupt inode, as it was not attached to the buffer */
> - xfs_iflush_abort(cip, false);
> - kmem_free(cilist);
> - xfs_perag_put(pag);
> - return -EFSCORRUPTED;
> + return error;
> }
>
> /*
> @@ -3693,17 +3667,16 @@ xfs_iflush(
> */
> if (XFS_FORCED_SHUTDOWN(mp)) {
> error = -EIO;
> - goto abort_out;
> + goto abort;
> }
>
> /*
> * Get the buffer containing the on-disk inode. We are doing a try-lock
> - * operation here, so we may get an EAGAIN error. In that case, we
> - * simply want to return with the inode still dirty.
> + * operation here, so we may get an EAGAIN error. In that case, return
> + * leaving the inode dirty.
> *
> * If we get any other error, we effectively have a corruption situation
> - * and we cannot flush the inode, so we treat it the same as failing
> - * xfs_iflush_int().
> + * and we cannot flush the inode. Abort the flush and shut down.
> */
> error = xfs_imap_to_bp(mp, NULL, &ip->i_imap, &dip, &bp, XBF_TRYLOCK,
> 0);
> @@ -3712,14 +3685,7 @@ xfs_iflush(
> return error;
> }
> if (error)
> - goto corrupt_out;
> -
> - /*
> - * First flush out the inode that xfs_iflush was called with.
> - */
> - error = xfs_iflush_int(ip, bp);
> - if (error)
> - goto corrupt_out;
> + goto abort;
>
> /*
> * If the buffer is pinned then push on the log now so we won't
> @@ -3729,28 +3695,29 @@ xfs_iflush(
> xfs_log_force(mp, 0);
>
> /*
> - * inode clustering: try to gather other inodes into this write
> + * Flush the provided inode then attempt to gather others from the
> + * cluster into the write.
> *
> - * Note: Any error during clustering will result in the filesystem
> - * being shut down and completion callbacks run on the cluster buffer.
> - * As we have already flushed and attached this inode to the buffer,
> - * it has already been aborted and released by xfs_iflush_cluster() and
> - * so we have no further error handling to do here.
> + * Note: Once we attempt to flush an inode, we must run buffer
> + * completion callbacks on any failure. If this fails, simulate an I/O
> + * failure on the buffer and shut down.
> */
> - error = xfs_iflush_cluster(ip, bp);
> - if (error)
> - return error;
> + error = xfs_iflush_int(ip, bp);
> + if (!error)
> + error = xfs_iflush_cluster(ip, bp);
> + if (error) {
> + bp->b_flags |= XBF_ASYNC;
> + xfs_buf_ioend_fail(bp);
> + goto shutdown;
> + }
>
> *bpp = bp;
> return 0;
>
> -corrupt_out:
> - if (bp)
> - xfs_buf_relse(bp);
> - xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> -abort_out:
> - /* abort the corrupt inode, as it was not attached to the buffer */
> +abort:
> xfs_iflush_abort(ip, false);
> +shutdown:
> + xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> return error;
> }
>
> @@ -3792,6 +3759,7 @@ xfs_iflush_int(
> struct xfs_inode_log_item *iip = ip->i_itemp;
> struct xfs_dinode *dip;
> struct xfs_mount *mp = ip->i_mount;
> + int error;
>
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> ASSERT(xfs_isiflocked(ip));
> @@ -3799,15 +3767,21 @@ xfs_iflush_int(
> ip->i_d.di_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK));
> ASSERT(iip != NULL && iip->ili_fields != 0);
>
> - /* set *dip = inode's place in the buffer */
> dip = xfs_buf_offset(bp, ip->i_imap.im_boffset);
>
> + /*
> + * We don't flush the inode if any of the following checks fail, but we
> + * do still update the log item and attach to the backing buffer as if
> + * the flush happened. This is a formality to facilitate predictable
> + * error handling as the caller will shutdown and fail the buffer.
> + */
> + error = -EFSCORRUPTED;
> if (XFS_TEST_ERROR(dip->di_magic != cpu_to_be16(XFS_DINODE_MAGIC),
> mp, XFS_ERRTAG_IFLUSH_1)) {
> xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
> "%s: Bad inode %Lu magic number 0x%x, ptr "PTR_FMT,
> __func__, ip->i_ino, be16_to_cpu(dip->di_magic), dip);
> - goto corrupt_out;
> + goto flush_out;
> }
> if (S_ISREG(VFS_I(ip)->i_mode)) {
> if (XFS_TEST_ERROR(
> @@ -3817,7 +3791,7 @@ xfs_iflush_int(
> xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
> "%s: Bad regular inode %Lu, ptr "PTR_FMT,
> __func__, ip->i_ino, ip);
> - goto corrupt_out;
> + goto flush_out;
> }
> } else if (S_ISDIR(VFS_I(ip)->i_mode)) {
> if (XFS_TEST_ERROR(
> @@ -3828,7 +3802,7 @@ xfs_iflush_int(
> xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
> "%s: Bad directory inode %Lu, ptr "PTR_FMT,
> __func__, ip->i_ino, ip);
> - goto corrupt_out;
> + goto flush_out;
> }
> }
> if (XFS_TEST_ERROR(ip->i_d.di_nextents + ip->i_d.di_anextents >
> @@ -3839,14 +3813,14 @@ xfs_iflush_int(
> __func__, ip->i_ino,
> ip->i_d.di_nextents + ip->i_d.di_anextents,
> ip->i_d.di_nblocks, ip);
> - goto corrupt_out;
> + goto flush_out;
> }
> if (XFS_TEST_ERROR(ip->i_d.di_forkoff > mp->m_sb.sb_inodesize,
> mp, XFS_ERRTAG_IFLUSH_6)) {
> xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
> "%s: bad inode %Lu, forkoff 0x%x, ptr "PTR_FMT,
> __func__, ip->i_ino, ip->i_d.di_forkoff, ip);
> - goto corrupt_out;
> + goto flush_out;
> }
>
> /*
> @@ -3863,7 +3837,7 @@ xfs_iflush_int(
>
> /* Check the inline fork data before we write out. */
> if (!xfs_inode_verify_forks(ip))
> - goto corrupt_out;
> + goto flush_out;
>
> /*
> * Copy the dirty parts of the inode into the on-disk inode. We always
> @@ -3906,6 +3880,8 @@ xfs_iflush_int(
> * need the AIL lock, because it is a 64 bit value that cannot be read
> * atomically.
> */
> + error = 0;
> +flush_out:
> iip->ili_last_fields = iip->ili_fields;
> iip->ili_fields = 0;
> iip->ili_fsync_fields = 0;
> @@ -3915,10 +3891,10 @@ xfs_iflush_int(
> &iip->ili_item.li_lsn);
>
> /*
> - * Attach the function xfs_iflush_done to the inode's
> - * buffer. This will remove the inode from the AIL
> - * and unlock the inode's flush lock when the inode is
> - * completely written to disk.
> + * Attach the inode item callback to the buffer whether the flush
> + * succeeded or not. If not, the caller will shut down and fail I/O
> + * completion on the buffer to remove the inode from the AIL and release
> + * the flush lock.
> */
> xfs_buf_attach_iodone(bp, xfs_iflush_done, &iip->ili_item);
>
> @@ -3927,10 +3903,7 @@ xfs_iflush_int(
>
> ASSERT(!list_empty(&bp->b_li_list));
> ASSERT(bp->b_iodone != NULL);
> - return 0;
> -
> -corrupt_out:
> - return -EFSCORRUPTED;
> + return error;
> }
>
> /* Release an inode. */
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 04/17] xfs: remove unnecessary shutdown check from xfs_iflush()
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
` (2 preceding siblings ...)
2020-05-04 14:11 ` [PATCH v4 03/17] xfs: simplify inode flush error handling Brian Foster
@ 2020-05-04 14:11 ` Brian Foster
2020-05-04 14:11 ` [PATCH v4 05/17] xfs: reset buffer write failure state on successful completion Brian Foster
` (13 subsequent siblings)
17 siblings, 0 replies; 37+ messages in thread
From: Brian Foster @ 2020-05-04 14:11 UTC (permalink / raw)
To: linux-xfs
The shutdown check in xfs_iflush() duplicates checks down in the
buffer code. If the fs is shut down, xfs_trans_read_buf_map() always
returns an error and falls into the same error path. Remove the
unnecessary check along with the warning in xfs_imap_to_bp()
that generates excessive noise in the log if the fs is shut down.
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_inode_buf.c | 7 +------
fs/xfs/xfs_inode.c | 13 -------------
2 files changed, 1 insertion(+), 19 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 39c5a6e24915..b102e611bf54 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -172,12 +172,7 @@ xfs_imap_to_bp(
(int)imap->im_len, buf_flags, &bp,
&xfs_inode_buf_ops);
if (error) {
- if (error == -EAGAIN) {
- ASSERT(buf_flags & XBF_TRYLOCK);
- return error;
- }
- xfs_warn(mp, "%s: xfs_trans_read_buf() returned error %d.",
- __func__, error);
+ ASSERT(error != -EAGAIN || (buf_flags & XBF_TRYLOCK));
return error;
}
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 84f2ee9957dc..6fb3e26afa8b 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3657,19 +3657,6 @@ xfs_iflush(
return 0;
}
- /*
- * This may have been unpinned because the filesystem is shutting
- * down forcibly. If that's the case we must not write this inode
- * to disk, because the log record didn't make it to disk.
- *
- * We also have to remove the log item from the AIL in this case,
- * as we wait for an empty AIL as part of the unmount process.
- */
- if (XFS_FORCED_SHUTDOWN(mp)) {
- error = -EIO;
- goto abort;
- }
-
/*
* Get the buffer containing the on-disk inode. We are doing a try-lock
* operation here, so we may get an EAGAIN error. In that case, return
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v4 05/17] xfs: reset buffer write failure state on successful completion
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
` (3 preceding siblings ...)
2020-05-04 14:11 ` [PATCH v4 04/17] xfs: remove unnecessary shutdown check from xfs_iflush() Brian Foster
@ 2020-05-04 14:11 ` Brian Foster
2020-05-05 21:09 ` Allison Collins
2020-05-05 21:09 ` Allison Collins
2020-05-04 14:11 ` [PATCH v4 06/17] xfs: refactor ratelimited buffer error messages into helper Brian Foster
` (12 subsequent siblings)
17 siblings, 2 replies; 37+ messages in thread
From: Brian Foster @ 2020-05-04 14:11 UTC (permalink / raw)
To: linux-xfs
The buffer write failure flag is intended to control the internal
write retry that XFS has historically implemented to help mitigate
the severity of transient I/O errors. The flag is set when a buffer
is resubmitted from the I/O completion path due to a previous
failure. It is checked on subsequent I/O completions to skip the
internal retry and fall through to the higher level configurable
error handling mechanism. The flag is cleared in the synchronous and
delwri submission paths and also checked in various places to log
write failure messages.
There are a couple minor problems with the current usage of this
flag. One is that we issue an internal retry after every submission
from xfsaild due to how delwri submission clears the flag. This
results in double the expected or configured number of write
attempts when under sustained failures. Another more subtle issue is
that the flag is never cleared on successful I/O completion. This
can cause xfs_wait_buftarg() to suggest that dirty buffers are being
thrown away due to the existence of the flag, when the reality is
that the flag might still be set because the write succeeded on the
retry.
Clear the write failure flag on successful I/O completion to address
both of these problems. This means that the internal retry attempt
occurs once since the last time a buffer write failed and that
various other contexts only see the flag set when the immediately
previous write attempt has failed.
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_buf.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index d5d6a68bb1e6..fd76a84cefdd 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1197,8 +1197,10 @@ xfs_buf_ioend(
bp->b_ops->verify_read(bp);
}
- if (!bp->b_error)
+ if (!bp->b_error) {
+ bp->b_flags &= ~XBF_WRITE_FAIL;
bp->b_flags |= XBF_DONE;
+ }
if (bp->b_iodone)
(*(bp->b_iodone))(bp);
@@ -1274,7 +1276,7 @@ xfs_bwrite(
bp->b_flags |= XBF_WRITE;
bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q |
- XBF_WRITE_FAIL | XBF_DONE);
+ XBF_DONE);
error = xfs_buf_submit(bp);
if (error)
@@ -1996,7 +1998,7 @@ xfs_buf_delwri_submit_buffers(
* synchronously. Otherwise, drop the buffer from the delwri
* queue and submit async.
*/
- bp->b_flags &= ~(_XBF_DELWRI_Q | XBF_WRITE_FAIL);
+ bp->b_flags &= ~_XBF_DELWRI_Q;
bp->b_flags |= XBF_WRITE;
if (wait_list) {
bp->b_flags &= ~XBF_ASYNC;
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 05/17] xfs: reset buffer write failure state on successful completion
2020-05-04 14:11 ` [PATCH v4 05/17] xfs: reset buffer write failure state on successful completion Brian Foster
@ 2020-05-05 21:09 ` Allison Collins
2020-05-05 21:09 ` Allison Collins
1 sibling, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-05-05 21:09 UTC (permalink / raw)
To: Brian Foster, linux-xfs
On 5/4/20 7:11 AM, Brian Foster wrote:
> The buffer write failure flag is intended to control the internal
> write retry that XFS has historically implemented to help mitigate
> the severity of transient I/O errors. The flag is set when a buffer
> is resubmitted from the I/O completion path due to a previous
> failure. It is checked on subsequent I/O completions to skip the
> internal retry and fall through to the higher level configurable
> error handling mechanism. The flag is cleared in the synchronous and
> delwri submission paths and also checked in various places to log
> write failure messages.
>
> There are a couple minor problems with the current usage of this
> flag. One is that we issue an internal retry after every submission
> from xfsaild due to how delwri submission clears the flag. This
> results in double the expected or configured number of write
> attempts when under sustained failures. Another more subtle issue is
> that the flag is never cleared on successful I/O completion. This
> can cause xfs_wait_buftarg() to suggest that dirty buffers are being
> thrown away due to the existence of the flag, when the reality is
> that the flag might still be set because the write succeeded on the
> retry.
>
> Clear the write failure flag on successful I/O completion to address
> both of these problems. This means that the internal retry attempt
> occurs once since the last time a buffer write failed and that
> various other contexts only see the flag set when the immediately
> previous write attempt has failed.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks ok to me:
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
> ---
> fs/xfs/xfs_buf.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index d5d6a68bb1e6..fd76a84cefdd 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1197,8 +1197,10 @@ xfs_buf_ioend(
> bp->b_ops->verify_read(bp);
> }
>
> - if (!bp->b_error)
> + if (!bp->b_error) {
> + bp->b_flags &= ~XBF_WRITE_FAIL;
> bp->b_flags |= XBF_DONE;
> + }
>
> if (bp->b_iodone)
> (*(bp->b_iodone))(bp);
> @@ -1274,7 +1276,7 @@ xfs_bwrite(
>
> bp->b_flags |= XBF_WRITE;
> bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q |
> - XBF_WRITE_FAIL | XBF_DONE);
> + XBF_DONE);
>
> error = xfs_buf_submit(bp);
> if (error)
> @@ -1996,7 +1998,7 @@ xfs_buf_delwri_submit_buffers(
> * synchronously. Otherwise, drop the buffer from the delwri
> * queue and submit async.
> */
> - bp->b_flags &= ~(_XBF_DELWRI_Q | XBF_WRITE_FAIL);
> + bp->b_flags &= ~_XBF_DELWRI_Q;
> bp->b_flags |= XBF_WRITE;
> if (wait_list) {
> bp->b_flags &= ~XBF_ASYNC;
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v4 05/17] xfs: reset buffer write failure state on successful completion
2020-05-04 14:11 ` [PATCH v4 05/17] xfs: reset buffer write failure state on successful completion Brian Foster
2020-05-05 21:09 ` Allison Collins
@ 2020-05-05 21:09 ` Allison Collins
1 sibling, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-05-05 21:09 UTC (permalink / raw)
To: Brian Foster, linux-xfs
On 5/4/20 7:11 AM, Brian Foster wrote:
> The buffer write failure flag is intended to control the internal
> write retry that XFS has historically implemented to help mitigate
> the severity of transient I/O errors. The flag is set when a buffer
> is resubmitted from the I/O completion path due to a previous
> failure. It is checked on subsequent I/O completions to skip the
> internal retry and fall through to the higher level configurable
> error handling mechanism. The flag is cleared in the synchronous and
> delwri submission paths and also checked in various places to log
> write failure messages.
>
> There are a couple minor problems with the current usage of this
> flag. One is that we issue an internal retry after every submission
> from xfsaild due to how delwri submission clears the flag. This
> results in double the expected or configured number of write
> attempts when under sustained failures. Another more subtle issue is
> that the flag is never cleared on successful I/O completion. This
> can cause xfs_wait_buftarg() to suggest that dirty buffers are being
> thrown away due to the existence of the flag, when the reality is
> that the flag might still be set because the write succeeded on the
> retry.
>
> Clear the write failure flag on successful I/O completion to address
> both of these problems. This means that the internal retry attempt
> occurs once since the last time a buffer write failed and that
> various other contexts only see the flag set when the immediately
> previous write attempt has failed.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks ok to me:
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
> ---
> fs/xfs/xfs_buf.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index d5d6a68bb1e6..fd76a84cefdd 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1197,8 +1197,10 @@ xfs_buf_ioend(
> bp->b_ops->verify_read(bp);
> }
>
> - if (!bp->b_error)
> + if (!bp->b_error) {
> + bp->b_flags &= ~XBF_WRITE_FAIL;
> bp->b_flags |= XBF_DONE;
> + }
>
> if (bp->b_iodone)
> (*(bp->b_iodone))(bp);
> @@ -1274,7 +1276,7 @@ xfs_bwrite(
>
> bp->b_flags |= XBF_WRITE;
> bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q |
> - XBF_WRITE_FAIL | XBF_DONE);
> + XBF_DONE);
>
> error = xfs_buf_submit(bp);
> if (error)
> @@ -1996,7 +1998,7 @@ xfs_buf_delwri_submit_buffers(
> * synchronously. Otherwise, drop the buffer from the delwri
> * queue and submit async.
> */
> - bp->b_flags &= ~(_XBF_DELWRI_Q | XBF_WRITE_FAIL);
> + bp->b_flags &= ~_XBF_DELWRI_Q;
> bp->b_flags |= XBF_WRITE;
> if (wait_list) {
> bp->b_flags &= ~XBF_ASYNC;
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 06/17] xfs: refactor ratelimited buffer error messages into helper
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
` (4 preceding siblings ...)
2020-05-04 14:11 ` [PATCH v4 05/17] xfs: reset buffer write failure state on successful completion Brian Foster
@ 2020-05-04 14:11 ` Brian Foster
2020-05-05 21:09 ` Allison Collins
2020-05-04 14:11 ` [PATCH v4 07/17] xfs: ratelimit unmount time per-buffer I/O error alert Brian Foster
` (11 subsequent siblings)
17 siblings, 1 reply; 37+ messages in thread
From: Brian Foster @ 2020-05-04 14:11 UTC (permalink / raw)
To: linux-xfs
XFS has some inconsistent log message rate limiting with respect to
buffer alerts. The metadata I/O error notification uses the generic
ratelimited alert, the buffer push code uses a custom rate limit and
the similar quiesce time failure checks are not rate limited at all
(when they should be).
The custom rate limit defined in the buf item code is specifically
crafted for buffer alerts. It is more aggressive than generic rate
limiting code because it must accommodate a high frequency of I/O
error events in a relative short timeframe.
Factor out the custom rate limit state from the buf item code into a
per-buftarg rate limit so various alerts are limited based on the
target. Define a buffer alert helper function and use it for the
buffer alerts that are already ratelimited.
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_buf.c | 15 +++++++++++----
fs/xfs/xfs_buf.h | 1 +
fs/xfs/xfs_buf_item.c | 17 ++++-------------
fs/xfs/xfs_message.c | 22 ++++++++++++++++++++++
fs/xfs/xfs_message.h | 3 +++
5 files changed, 41 insertions(+), 17 deletions(-)
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index fd76a84cefdd..594d5e1df6f8 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1244,10 +1244,10 @@ xfs_buf_ioerror_alert(
struct xfs_buf *bp,
xfs_failaddr_t func)
{
- xfs_alert_ratelimited(bp->b_mount,
-"metadata I/O error in \"%pS\" at daddr 0x%llx len %d error %d",
- func, (uint64_t)XFS_BUF_ADDR(bp), bp->b_length,
- -bp->b_error);
+ xfs_buf_alert_ratelimited(bp, "XFS: metadata IO error",
+ "metadata I/O error in \"%pS\" at daddr 0x%llx len %d error %d",
+ func, (uint64_t)XFS_BUF_ADDR(bp),
+ bp->b_length, -bp->b_error);
}
/*
@@ -1828,6 +1828,13 @@ xfs_alloc_buftarg(
btp->bt_bdev = bdev;
btp->bt_daxdev = dax_dev;
+ /*
+ * Buffer IO error rate limiting. Limit it to no more than 10 messages
+ * per 30 seconds so as to not spam logs too much on repeated errors.
+ */
+ ratelimit_state_init(&btp->bt_ioerror_rl, 30 * HZ,
+ DEFAULT_RATELIMIT_BURST);
+
if (xfs_setsize_buftarg_early(btp, bdev))
goto error_free;
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 06ea3eef866e..050c53b739e2 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -91,6 +91,7 @@ typedef struct xfs_buftarg {
struct list_lru bt_lru;
struct percpu_counter bt_io_count;
+ struct ratelimit_state bt_ioerror_rl;
} xfs_buftarg_t;
struct xfs_buf;
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index b452a399a441..1f7acffc99ba 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -481,14 +481,6 @@ xfs_buf_item_unpin(
}
}
-/*
- * Buffer IO error rate limiting. Limit it to no more than 10 messages per 30
- * seconds so as to not spam logs too much on repeated detection of the same
- * buffer being bad..
- */
-
-static DEFINE_RATELIMIT_STATE(xfs_buf_write_fail_rl_state, 30 * HZ, 10);
-
STATIC uint
xfs_buf_item_push(
struct xfs_log_item *lip,
@@ -518,11 +510,10 @@ xfs_buf_item_push(
trace_xfs_buf_item_push(bip);
/* has a previous flush failed due to IO errors? */
- if ((bp->b_flags & XBF_WRITE_FAIL) &&
- ___ratelimit(&xfs_buf_write_fail_rl_state, "XFS: Failing async write")) {
- xfs_warn(bp->b_mount,
-"Failing async write on buffer block 0x%llx. Retrying async write.",
- (long long)bp->b_bn);
+ if (bp->b_flags & XBF_WRITE_FAIL) {
+ xfs_buf_alert_ratelimited(bp, "XFS: Failing async write",
+ "Failing async write on buffer block 0x%llx. Retrying async write.",
+ (long long)bp->b_bn);
}
if (!xfs_buf_delwri_queue(bp, buffer_list))
diff --git a/fs/xfs/xfs_message.c b/fs/xfs/xfs_message.c
index e0f9d3b6abe9..bc66d95c8d4c 100644
--- a/fs/xfs/xfs_message.c
+++ b/fs/xfs/xfs_message.c
@@ -117,3 +117,25 @@ xfs_hex_dump(const void *p, int length)
{
print_hex_dump(KERN_ALERT, "", DUMP_PREFIX_OFFSET, 16, 1, p, length, 1);
}
+
+void
+xfs_buf_alert_ratelimited(
+ struct xfs_buf *bp,
+ const char *rlmsg,
+ const char *fmt,
+ ...)
+{
+ struct xfs_mount *mp = bp->b_mount;
+ struct va_format vaf;
+ va_list args;
+
+ /* use the more aggressive per-target rate limit for buffers */
+ if (!___ratelimit(&bp->b_target->bt_ioerror_rl, rlmsg))
+ return;
+
+ va_start(args, fmt);
+ vaf.fmt = fmt;
+ vaf.va = &args;
+ __xfs_printk(KERN_ALERT, mp, &vaf);
+ va_end(args);
+}
diff --git a/fs/xfs/xfs_message.h b/fs/xfs/xfs_message.h
index 0b05e10995a0..6be2ebe3a7b9 100644
--- a/fs/xfs/xfs_message.h
+++ b/fs/xfs/xfs_message.h
@@ -62,4 +62,7 @@ void asswarn(struct xfs_mount *mp, char *expr, char *f, int l);
extern void xfs_hex_dump(const void *p, int length);
+void xfs_buf_alert_ratelimited(struct xfs_buf *bp, const char *rlmsg,
+ const char *fmt, ...);
+
#endif /* __XFS_MESSAGE_H */
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 06/17] xfs: refactor ratelimited buffer error messages into helper
2020-05-04 14:11 ` [PATCH v4 06/17] xfs: refactor ratelimited buffer error messages into helper Brian Foster
@ 2020-05-05 21:09 ` Allison Collins
0 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-05-05 21:09 UTC (permalink / raw)
To: Brian Foster, linux-xfs
On 5/4/20 7:11 AM, Brian Foster wrote:
> XFS has some inconsistent log message rate limiting with respect to
> buffer alerts. The metadata I/O error notification uses the generic
> ratelimited alert, the buffer push code uses a custom rate limit and
> the similar quiesce time failure checks are not rate limited at all
> (when they should be).
>
> The custom rate limit defined in the buf item code is specifically
> crafted for buffer alerts. It is more aggressive than generic rate
> limiting code because it must accommodate a high frequency of I/O
> error events in a relative short timeframe.
>
> Factor out the custom rate limit state from the buf item code into a
> per-buftarg rate limit so various alerts are limited based on the
> target. Define a buffer alert helper function and use it for the
> buffer alerts that are already ratelimited.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Alrighty, make sense
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
> ---
> fs/xfs/xfs_buf.c | 15 +++++++++++----
> fs/xfs/xfs_buf.h | 1 +
> fs/xfs/xfs_buf_item.c | 17 ++++-------------
> fs/xfs/xfs_message.c | 22 ++++++++++++++++++++++
> fs/xfs/xfs_message.h | 3 +++
> 5 files changed, 41 insertions(+), 17 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index fd76a84cefdd..594d5e1df6f8 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1244,10 +1244,10 @@ xfs_buf_ioerror_alert(
> struct xfs_buf *bp,
> xfs_failaddr_t func)
> {
> - xfs_alert_ratelimited(bp->b_mount,
> -"metadata I/O error in \"%pS\" at daddr 0x%llx len %d error %d",
> - func, (uint64_t)XFS_BUF_ADDR(bp), bp->b_length,
> - -bp->b_error);
> + xfs_buf_alert_ratelimited(bp, "XFS: metadata IO error",
> + "metadata I/O error in \"%pS\" at daddr 0x%llx len %d error %d",
> + func, (uint64_t)XFS_BUF_ADDR(bp),
> + bp->b_length, -bp->b_error);
> }
>
> /*
> @@ -1828,6 +1828,13 @@ xfs_alloc_buftarg(
> btp->bt_bdev = bdev;
> btp->bt_daxdev = dax_dev;
>
> + /*
> + * Buffer IO error rate limiting. Limit it to no more than 10 messages
> + * per 30 seconds so as to not spam logs too much on repeated errors.
> + */
> + ratelimit_state_init(&btp->bt_ioerror_rl, 30 * HZ,
> + DEFAULT_RATELIMIT_BURST);
> +
> if (xfs_setsize_buftarg_early(btp, bdev))
> goto error_free;
>
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 06ea3eef866e..050c53b739e2 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -91,6 +91,7 @@ typedef struct xfs_buftarg {
> struct list_lru bt_lru;
>
> struct percpu_counter bt_io_count;
> + struct ratelimit_state bt_ioerror_rl;
> } xfs_buftarg_t;
>
> struct xfs_buf;
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index b452a399a441..1f7acffc99ba 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -481,14 +481,6 @@ xfs_buf_item_unpin(
> }
> }
>
> -/*
> - * Buffer IO error rate limiting. Limit it to no more than 10 messages per 30
> - * seconds so as to not spam logs too much on repeated detection of the same
> - * buffer being bad..
> - */
> -
> -static DEFINE_RATELIMIT_STATE(xfs_buf_write_fail_rl_state, 30 * HZ, 10);
> -
> STATIC uint
> xfs_buf_item_push(
> struct xfs_log_item *lip,
> @@ -518,11 +510,10 @@ xfs_buf_item_push(
> trace_xfs_buf_item_push(bip);
>
> /* has a previous flush failed due to IO errors? */
> - if ((bp->b_flags & XBF_WRITE_FAIL) &&
> - ___ratelimit(&xfs_buf_write_fail_rl_state, "XFS: Failing async write")) {
> - xfs_warn(bp->b_mount,
> -"Failing async write on buffer block 0x%llx. Retrying async write.",
> - (long long)bp->b_bn);
> + if (bp->b_flags & XBF_WRITE_FAIL) {
> + xfs_buf_alert_ratelimited(bp, "XFS: Failing async write",
> + "Failing async write on buffer block 0x%llx. Retrying async write.",
> + (long long)bp->b_bn);
> }
>
> if (!xfs_buf_delwri_queue(bp, buffer_list))
> diff --git a/fs/xfs/xfs_message.c b/fs/xfs/xfs_message.c
> index e0f9d3b6abe9..bc66d95c8d4c 100644
> --- a/fs/xfs/xfs_message.c
> +++ b/fs/xfs/xfs_message.c
> @@ -117,3 +117,25 @@ xfs_hex_dump(const void *p, int length)
> {
> print_hex_dump(KERN_ALERT, "", DUMP_PREFIX_OFFSET, 16, 1, p, length, 1);
> }
> +
> +void
> +xfs_buf_alert_ratelimited(
> + struct xfs_buf *bp,
> + const char *rlmsg,
> + const char *fmt,
> + ...)
> +{
> + struct xfs_mount *mp = bp->b_mount;
> + struct va_format vaf;
> + va_list args;
> +
> + /* use the more aggressive per-target rate limit for buffers */
> + if (!___ratelimit(&bp->b_target->bt_ioerror_rl, rlmsg))
> + return;
> +
> + va_start(args, fmt);
> + vaf.fmt = fmt;
> + vaf.va = &args;
> + __xfs_printk(KERN_ALERT, mp, &vaf);
> + va_end(args);
> +}
> diff --git a/fs/xfs/xfs_message.h b/fs/xfs/xfs_message.h
> index 0b05e10995a0..6be2ebe3a7b9 100644
> --- a/fs/xfs/xfs_message.h
> +++ b/fs/xfs/xfs_message.h
> @@ -62,4 +62,7 @@ void asswarn(struct xfs_mount *mp, char *expr, char *f, int l);
>
> extern void xfs_hex_dump(const void *p, int length);
>
> +void xfs_buf_alert_ratelimited(struct xfs_buf *bp, const char *rlmsg,
> + const char *fmt, ...);
> +
> #endif /* __XFS_MESSAGE_H */
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 07/17] xfs: ratelimit unmount time per-buffer I/O error alert
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
` (5 preceding siblings ...)
2020-05-04 14:11 ` [PATCH v4 06/17] xfs: refactor ratelimited buffer error messages into helper Brian Foster
@ 2020-05-04 14:11 ` Brian Foster
2020-05-05 21:10 ` Allison Collins
2020-05-06 11:05 ` [PATCH v4.1 " Brian Foster
2020-05-04 14:11 ` [PATCH v4 08/17] xfs: fix duplicate verification from xfs_qm_dqflush() Brian Foster
` (10 subsequent siblings)
17 siblings, 2 replies; 37+ messages in thread
From: Brian Foster @ 2020-05-04 14:11 UTC (permalink / raw)
To: linux-xfs
At unmount time, XFS emits an alert for every in-core buffer that
might have undergone a write error. In practice this behavior is
probably reasonable given that the filesystem is likely short lived
once I/O errors begin to occur consistently. Under certain test or
otherwise expected error conditions, this can spam the logs and slow
down the unmount.
Now that we have a ratelimit mechanism specifically for buffer
alerts, reuse it for the per-buffer alerts in xfs_wait_buftarg().
Also lift the final repair message out of the loop so it always
prints and assert that the metadata error handling code has shut
down the fs.
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_buf.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 594d5e1df6f8..8f0f605de579 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1657,7 +1657,8 @@ xfs_wait_buftarg(
struct xfs_buftarg *btp)
{
LIST_HEAD(dispose);
- int loop = 0;
+ int loop = 0;
+ bool write_fail = false;
/*
* First wait on the buftarg I/O count for all in-flight buffers to be
@@ -1685,17 +1686,23 @@ xfs_wait_buftarg(
bp = list_first_entry(&dispose, struct xfs_buf, b_lru);
list_del_init(&bp->b_lru);
if (bp->b_flags & XBF_WRITE_FAIL) {
- xfs_alert(btp->bt_mount,
+ write_fail = true;
+ xfs_buf_alert_ratelimited(bp,
+ "XFS: Corruption Alert",
"Corruption Alert: Buffer at daddr 0x%llx had permanent write failures!",
(long long)bp->b_bn);
- xfs_alert(btp->bt_mount,
-"Please run xfs_repair to determine the extent of the problem.");
}
xfs_buf_rele(bp);
}
if (loop++ != 0)
delay(100);
}
+
+ if (write_fail) {
+ ASSERT(XFS_FORCED_SHUTDOWN(btp->bt_mount));
+ xfs_alert(btp->bt_mount,
+ "Please run xfs_repair to determine the extent of the problem.");
+ }
}
static enum lru_status
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 07/17] xfs: ratelimit unmount time per-buffer I/O error alert
2020-05-04 14:11 ` [PATCH v4 07/17] xfs: ratelimit unmount time per-buffer I/O error alert Brian Foster
@ 2020-05-05 21:10 ` Allison Collins
2020-05-06 11:05 ` [PATCH v4.1 " Brian Foster
1 sibling, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-05-05 21:10 UTC (permalink / raw)
To: Brian Foster, linux-xfs
On 5/4/20 7:11 AM, Brian Foster wrote:
> At unmount time, XFS emits an alert for every in-core buffer that
> might have undergone a write error. In practice this behavior is
> probably reasonable given that the filesystem is likely short lived
> once I/O errors begin to occur consistently. Under certain test or
> otherwise expected error conditions, this can spam the logs and slow
> down the unmount.
>
> Now that we have a ratelimit mechanism specifically for buffer
> alerts, reuse it for the per-buffer alerts in xfs_wait_buftarg().
> Also lift the final repair message out of the loop so it always
> prints and assert that the metadata error handling code has shut
> down the fs.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks fine to me:
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
> ---
> fs/xfs/xfs_buf.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 594d5e1df6f8..8f0f605de579 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1657,7 +1657,8 @@ xfs_wait_buftarg(
> struct xfs_buftarg *btp)
> {
> LIST_HEAD(dispose);
> - int loop = 0;
> + int loop = 0;
> + bool write_fail = false;
>
> /*
> * First wait on the buftarg I/O count for all in-flight buffers to be
> @@ -1685,17 +1686,23 @@ xfs_wait_buftarg(
> bp = list_first_entry(&dispose, struct xfs_buf, b_lru);
> list_del_init(&bp->b_lru);
> if (bp->b_flags & XBF_WRITE_FAIL) {
> - xfs_alert(btp->bt_mount,
> + write_fail = true;
> + xfs_buf_alert_ratelimited(bp,
> + "XFS: Corruption Alert",
> "Corruption Alert: Buffer at daddr 0x%llx had permanent write failures!",
> (long long)bp->b_bn);
> - xfs_alert(btp->bt_mount,
> -"Please run xfs_repair to determine the extent of the problem.");
> }
> xfs_buf_rele(bp);
> }
> if (loop++ != 0)
> delay(100);
> }
> +
> + if (write_fail) {
> + ASSERT(XFS_FORCED_SHUTDOWN(btp->bt_mount));
> + xfs_alert(btp->bt_mount,
> + "Please run xfs_repair to determine the extent of the problem.");
> + }
> }
>
> static enum lru_status
>
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH v4.1 07/17] xfs: ratelimit unmount time per-buffer I/O error alert
2020-05-04 14:11 ` [PATCH v4 07/17] xfs: ratelimit unmount time per-buffer I/O error alert Brian Foster
2020-05-05 21:10 ` Allison Collins
@ 2020-05-06 11:05 ` Brian Foster
2020-05-07 20:48 ` Dave Chinner
1 sibling, 1 reply; 37+ messages in thread
From: Brian Foster @ 2020-05-06 11:05 UTC (permalink / raw)
To: linux-xfs
At unmount time, XFS emits an alert for every in-core buffer that
might have undergone a write error. In practice this behavior is
probably reasonable given that the filesystem is likely short lived
once I/O errors begin to occur consistently. Under certain test or
otherwise expected error conditions, this can spam the logs and slow
down the unmount.
Now that we have a ratelimit mechanism specifically for buffer
alerts, reuse it for the per-buffer alerts in xfs_wait_buftarg().
Also lift the final repair message out of the loop so it always
prints and assert that the metadata error handling code has shut
down the fs.
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
---
v4.1:
- Add comment to document dependency on external permanent error
processing.
fs/xfs/xfs_buf.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 594d5e1df6f8..3918270f4eab 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1657,7 +1657,8 @@ xfs_wait_buftarg(
struct xfs_buftarg *btp)
{
LIST_HEAD(dispose);
- int loop = 0;
+ int loop = 0;
+ bool write_fail = false;
/*
* First wait on the buftarg I/O count for all in-flight buffers to be
@@ -1685,17 +1686,29 @@ xfs_wait_buftarg(
bp = list_first_entry(&dispose, struct xfs_buf, b_lru);
list_del_init(&bp->b_lru);
if (bp->b_flags & XBF_WRITE_FAIL) {
- xfs_alert(btp->bt_mount,
+ write_fail = true;
+ xfs_buf_alert_ratelimited(bp,
+ "XFS: Corruption Alert",
"Corruption Alert: Buffer at daddr 0x%llx had permanent write failures!",
(long long)bp->b_bn);
- xfs_alert(btp->bt_mount,
-"Please run xfs_repair to determine the extent of the problem.");
}
xfs_buf_rele(bp);
}
if (loop++ != 0)
delay(100);
}
+
+ /*
+ * If one or more failed buffers were freed, that means dirty metadata
+ * was thrown away. This should only ever happen after I/O completion
+ * handling has elevated I/O error(s) to permanent failures and shuts
+ * down the fs.
+ */
+ if (write_fail) {
+ ASSERT(XFS_FORCED_SHUTDOWN(btp->bt_mount));
+ xfs_alert(btp->bt_mount,
+ "Please run xfs_repair to determine the extent of the problem.");
+ }
}
static enum lru_status
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4.1 07/17] xfs: ratelimit unmount time per-buffer I/O error alert
2020-05-06 11:05 ` [PATCH v4.1 " Brian Foster
@ 2020-05-07 20:48 ` Dave Chinner
0 siblings, 0 replies; 37+ messages in thread
From: Dave Chinner @ 2020-05-07 20:48 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Wed, May 06, 2020 at 07:05:48AM -0400, Brian Foster wrote:
> At unmount time, XFS emits an alert for every in-core buffer that
> might have undergone a write error. In practice this behavior is
> probably reasonable given that the filesystem is likely short lived
> once I/O errors begin to occur consistently. Under certain test or
> otherwise expected error conditions, this can spam the logs and slow
> down the unmount.
>
> Now that we have a ratelimit mechanism specifically for buffer
> alerts, reuse it for the per-buffer alerts in xfs_wait_buftarg().
> Also lift the final repair message out of the loop so it always
> prints and assert that the metadata error handling code has shut
> down the fs.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Allison Collins <allison.henderson@oracle.com>
> ---
>
> v4.1:
> - Add comment to document dependency on external permanent error
> processing.
Looks good now. Thanks!
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 08/17] xfs: fix duplicate verification from xfs_qm_dqflush()
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
` (6 preceding siblings ...)
2020-05-04 14:11 ` [PATCH v4 07/17] xfs: ratelimit unmount time per-buffer I/O error alert Brian Foster
@ 2020-05-04 14:11 ` Brian Foster
2020-05-05 21:22 ` Allison Collins
2020-05-04 14:11 ` [PATCH v4 09/17] xfs: abort consistently on dquot flush failure Brian Foster
` (9 subsequent siblings)
17 siblings, 1 reply; 37+ messages in thread
From: Brian Foster @ 2020-05-04 14:11 UTC (permalink / raw)
To: linux-xfs
The pre-flush dquot verification in xfs_qm_dqflush() duplicates the
read verifier by checking the dquot in the on-disk buffer. Instead,
verify the in-core variant before it is flushed to the buffer.
Fixes: 7224fa482a6d ("xfs: add full xfs_dqblk verifier")
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/xfs_dquot.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index af2c8e5ceea0..265feb62290d 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -1116,13 +1116,12 @@ xfs_qm_dqflush(
dqb = bp->b_addr + dqp->q_bufoffset;
ddqp = &dqb->dd_diskdq;
- /*
- * A simple sanity check in case we got a corrupted dquot.
- */
- fa = xfs_dqblk_verify(mp, dqb, be32_to_cpu(ddqp->d_id), 0);
+ /* sanity check the in-core structure before we flush */
+ fa = xfs_dquot_verify(mp, &dqp->q_core, be32_to_cpu(dqp->q_core.d_id),
+ 0);
if (fa) {
xfs_alert(mp, "corrupt dquot ID 0x%x in memory at %pS",
- be32_to_cpu(ddqp->d_id), fa);
+ be32_to_cpu(dqp->q_core.d_id), fa);
xfs_buf_relse(bp);
xfs_dqfunlock(dqp);
xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 08/17] xfs: fix duplicate verification from xfs_qm_dqflush()
2020-05-04 14:11 ` [PATCH v4 08/17] xfs: fix duplicate verification from xfs_qm_dqflush() Brian Foster
@ 2020-05-05 21:22 ` Allison Collins
0 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-05-05 21:22 UTC (permalink / raw)
To: Brian Foster, linux-xfs
On 5/4/20 7:11 AM, Brian Foster wrote:
> The pre-flush dquot verification in xfs_qm_dqflush() duplicates the
> read verifier by checking the dquot in the on-disk buffer. Instead,
> verify the in-core variant before it is flushed to the buffer.
>
> Fixes: 7224fa482a6d ("xfs: add full xfs_dqblk verifier")
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Ok, looks good:
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
> ---
> fs/xfs/xfs_dquot.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index af2c8e5ceea0..265feb62290d 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -1116,13 +1116,12 @@ xfs_qm_dqflush(
> dqb = bp->b_addr + dqp->q_bufoffset;
> ddqp = &dqb->dd_diskdq;
>
> - /*
> - * A simple sanity check in case we got a corrupted dquot.
> - */
> - fa = xfs_dqblk_verify(mp, dqb, be32_to_cpu(ddqp->d_id), 0);
> + /* sanity check the in-core structure before we flush */
> + fa = xfs_dquot_verify(mp, &dqp->q_core, be32_to_cpu(dqp->q_core.d_id),
> + 0);
> if (fa) {
> xfs_alert(mp, "corrupt dquot ID 0x%x in memory at %pS",
> - be32_to_cpu(ddqp->d_id), fa);
> + be32_to_cpu(dqp->q_core.d_id), fa);
> xfs_buf_relse(bp);
> xfs_dqfunlock(dqp);
> xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 09/17] xfs: abort consistently on dquot flush failure
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
` (7 preceding siblings ...)
2020-05-04 14:11 ` [PATCH v4 08/17] xfs: fix duplicate verification from xfs_qm_dqflush() Brian Foster
@ 2020-05-04 14:11 ` Brian Foster
2020-05-04 14:11 ` [PATCH v4 10/17] xfs: acquire ->ail_lock from xfs_trans_ail_delete() Brian Foster
` (8 subsequent siblings)
17 siblings, 0 replies; 37+ messages in thread
From: Brian Foster @ 2020-05-04 14:11 UTC (permalink / raw)
To: linux-xfs
The dquot flush handler effectively aborts the dquot flush if the
filesystem is already shut down, but doesn't actually shut down if
the flush fails. Update xfs_qm_dqflush() to consistently abort the
dquot flush and shutdown the fs if the flush fails with an
unexpected error.
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/xfs_dquot.c | 32 ++++++++++----------------------
1 file changed, 10 insertions(+), 22 deletions(-)
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 265feb62290d..ffe607733c50 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -1068,6 +1068,7 @@ xfs_qm_dqflush(
struct xfs_buf **bpp)
{
struct xfs_mount *mp = dqp->q_mount;
+ struct xfs_log_item *lip = &dqp->q_logitem.qli_item;
struct xfs_buf *bp;
struct xfs_dqblk *dqb;
struct xfs_disk_dquot *ddqp;
@@ -1083,32 +1084,16 @@ xfs_qm_dqflush(
xfs_qm_dqunpin_wait(dqp);
- /*
- * This may have been unpinned because the filesystem is shutting
- * down forcibly. If that's the case we must not write this dquot
- * to disk, because the log record didn't make it to disk.
- *
- * We also have to remove the log item from the AIL in this case,
- * as we wait for an emptry AIL as part of the unmount process.
- */
- if (XFS_FORCED_SHUTDOWN(mp)) {
- struct xfs_log_item *lip = &dqp->q_logitem.qli_item;
- dqp->dq_flags &= ~XFS_DQ_DIRTY;
-
- xfs_trans_ail_remove(lip, SHUTDOWN_CORRUPT_INCORE);
-
- error = -EIO;
- goto out_unlock;
- }
-
/*
* Get the buffer containing the on-disk dquot
*/
error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp, dqp->q_blkno,
mp->m_quotainfo->qi_dqchunklen, XBF_TRYLOCK,
&bp, &xfs_dquot_buf_ops);
- if (error)
+ if (error == -EAGAIN)
goto out_unlock;
+ if (error)
+ goto out_abort;
/*
* Calculate the location of the dquot inside the buffer.
@@ -1123,9 +1108,8 @@ xfs_qm_dqflush(
xfs_alert(mp, "corrupt dquot ID 0x%x in memory at %pS",
be32_to_cpu(dqp->q_core.d_id), fa);
xfs_buf_relse(bp);
- xfs_dqfunlock(dqp);
- xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
- return -EFSCORRUPTED;
+ error = -EFSCORRUPTED;
+ goto out_abort;
}
/* This is the only portion of data that needs to persist */
@@ -1174,6 +1158,10 @@ xfs_qm_dqflush(
*bpp = bp;
return 0;
+out_abort:
+ dqp->dq_flags &= ~XFS_DQ_DIRTY;
+ xfs_trans_ail_remove(lip, SHUTDOWN_CORRUPT_INCORE);
+ xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
out_unlock:
xfs_dqfunlock(dqp);
return error;
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v4 10/17] xfs: acquire ->ail_lock from xfs_trans_ail_delete()
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
` (8 preceding siblings ...)
2020-05-04 14:11 ` [PATCH v4 09/17] xfs: abort consistently on dquot flush failure Brian Foster
@ 2020-05-04 14:11 ` Brian Foster
2020-05-05 22:22 ` Allison Collins
2020-05-04 14:11 ` [PATCH v4 11/17] xfs: use delete helper for items expected to be in AIL Brian Foster
` (7 subsequent siblings)
17 siblings, 1 reply; 37+ messages in thread
From: Brian Foster @ 2020-05-04 14:11 UTC (permalink / raw)
To: linux-xfs
Several callers acquire the lock just prior to the call. Callers
that require ->ail_lock for other purposes already check IN_AIL
state and thus don't require the additional shutdown check in the
helper. Push the lock down into xfs_trans_ail_delete(), open code
the instances that still acquire it, and remove the unnecessary ailp
parameter.
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_buf_item.c | 27 +++++++++++----------------
fs/xfs/xfs_dquot.c | 6 ++++--
fs/xfs/xfs_trans_ail.c | 3 ++-
fs/xfs/xfs_trans_priv.h | 14 ++++++++------
4 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 1f7acffc99ba..06e306b49283 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -410,7 +410,6 @@ xfs_buf_item_unpin(
{
struct xfs_buf_log_item *bip = BUF_ITEM(lip);
xfs_buf_t *bp = bip->bli_buf;
- struct xfs_ail *ailp = lip->li_ailp;
int stale = bip->bli_flags & XFS_BLI_STALE;
int freed;
@@ -452,10 +451,10 @@ xfs_buf_item_unpin(
}
/*
- * If we get called here because of an IO error, we may
- * or may not have the item on the AIL. xfs_trans_ail_delete()
- * will take care of that situation.
- * xfs_trans_ail_delete() drops the AIL lock.
+ * If we get called here because of an IO error, we may or may
+ * not have the item on the AIL. xfs_trans_ail_delete() will
+ * take care of that situation. xfs_trans_ail_delete() drops
+ * the AIL lock.
*/
if (bip->bli_flags & XFS_BLI_STALE_INODE) {
xfs_buf_do_callbacks(bp);
@@ -463,8 +462,7 @@ xfs_buf_item_unpin(
list_del_init(&bp->b_li_list);
bp->b_iodone = NULL;
} else {
- spin_lock(&ailp->ail_lock);
- xfs_trans_ail_delete(ailp, lip, SHUTDOWN_LOG_IO_ERROR);
+ xfs_trans_ail_delete(lip, SHUTDOWN_LOG_IO_ERROR);
xfs_buf_item_relse(bp);
ASSERT(bp->b_log_item == NULL);
}
@@ -1205,22 +1203,19 @@ xfs_buf_iodone(
struct xfs_buf *bp,
struct xfs_log_item *lip)
{
- struct xfs_ail *ailp = lip->li_ailp;
-
ASSERT(BUF_ITEM(lip)->bli_buf == bp);
xfs_buf_rele(bp);
/*
- * If we are forcibly shutting down, this may well be
- * off the AIL already. That's because we simulate the
- * log-committed callbacks to unpin these buffers. Or we may never
- * have put this item on AIL because of the transaction was
- * aborted forcibly. xfs_trans_ail_delete() takes care of these.
+ * If we are forcibly shutting down, this may well be off the AIL
+ * already. That's because we simulate the log-committed callbacks to
+ * unpin these buffers. Or we may never have put this item on AIL
+ * because of the transaction was aborted forcibly.
+ * xfs_trans_ail_delete() takes care of these.
*
* Either way, AIL is useless if we're forcing a shutdown.
*/
- spin_lock(&ailp->ail_lock);
- xfs_trans_ail_delete(ailp, lip, SHUTDOWN_CORRUPT_INCORE);
+ xfs_trans_ail_delete(lip, SHUTDOWN_CORRUPT_INCORE);
xfs_buf_item_free(BUF_ITEM(lip));
}
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index ffe607733c50..5fb65f43b980 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -1021,6 +1021,7 @@ xfs_qm_dqflush_done(
struct xfs_dq_logitem *qip = (struct xfs_dq_logitem *)lip;
struct xfs_dquot *dqp = qip->qli_dquot;
struct xfs_ail *ailp = lip->li_ailp;
+ xfs_lsn_t tail_lsn;
/*
* We only want to pull the item from the AIL if its
@@ -1034,10 +1035,11 @@ xfs_qm_dqflush_done(
((lip->li_lsn == qip->qli_flush_lsn) ||
test_bit(XFS_LI_FAILED, &lip->li_flags))) {
- /* xfs_trans_ail_delete() drops the AIL lock. */
spin_lock(&ailp->ail_lock);
if (lip->li_lsn == qip->qli_flush_lsn) {
- xfs_trans_ail_delete(ailp, lip, SHUTDOWN_CORRUPT_INCORE);
+ /* xfs_ail_update_finish() drops the AIL lock */
+ tail_lsn = xfs_ail_delete_one(ailp, lip);
+ xfs_ail_update_finish(ailp, tail_lsn);
} else {
/*
* Clear the failed state since we are about to drop the
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 2574d01e4a83..cfba691664c7 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -864,13 +864,14 @@ xfs_ail_delete_one(
*/
void
xfs_trans_ail_delete(
- struct xfs_ail *ailp,
struct xfs_log_item *lip,
int shutdown_type)
{
+ struct xfs_ail *ailp = lip->li_ailp;
struct xfs_mount *mp = ailp->ail_mount;
xfs_lsn_t tail_lsn;
+ spin_lock(&ailp->ail_lock);
if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
spin_unlock(&ailp->ail_lock);
if (!XFS_FORCED_SHUTDOWN(mp)) {
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index 35655eac01a6..e4362fb8d483 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -94,8 +94,7 @@ xfs_trans_ail_update(
xfs_lsn_t xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip);
void xfs_ail_update_finish(struct xfs_ail *ailp, xfs_lsn_t old_lsn)
__releases(ailp->ail_lock);
-void xfs_trans_ail_delete(struct xfs_ail *ailp, struct xfs_log_item *lip,
- int shutdown_type);
+void xfs_trans_ail_delete(struct xfs_log_item *lip, int shutdown_type);
static inline void
xfs_trans_ail_remove(
@@ -103,13 +102,16 @@ xfs_trans_ail_remove(
int shutdown_type)
{
struct xfs_ail *ailp = lip->li_ailp;
+ xfs_lsn_t tail_lsn;
spin_lock(&ailp->ail_lock);
- /* xfs_trans_ail_delete() drops the AIL lock */
- if (test_bit(XFS_LI_IN_AIL, &lip->li_flags))
- xfs_trans_ail_delete(ailp, lip, shutdown_type);
- else
+ /* xfs_ail_update_finish() drops the AIL lock */
+ if (test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
+ tail_lsn = xfs_ail_delete_one(ailp, lip);
+ xfs_ail_update_finish(ailp, tail_lsn);
+ } else {
spin_unlock(&ailp->ail_lock);
+ }
}
void xfs_ail_push(struct xfs_ail *, xfs_lsn_t);
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 10/17] xfs: acquire ->ail_lock from xfs_trans_ail_delete()
2020-05-04 14:11 ` [PATCH v4 10/17] xfs: acquire ->ail_lock from xfs_trans_ail_delete() Brian Foster
@ 2020-05-05 22:22 ` Allison Collins
0 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-05-05 22:22 UTC (permalink / raw)
To: Brian Foster, linux-xfs
On 5/4/20 7:11 AM, Brian Foster wrote:
> Several callers acquire the lock just prior to the call. Callers
> that require ->ail_lock for other purposes already check IN_AIL
> state and thus don't require the additional shutdown check in the
> helper. Push the lock down into xfs_trans_ail_delete(), open code
> the instances that still acquire it, and remove the unnecessary ailp
> parameter.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Ok, followed it through and I think it makes sense:
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
> ---
> fs/xfs/xfs_buf_item.c | 27 +++++++++++----------------
> fs/xfs/xfs_dquot.c | 6 ++++--
> fs/xfs/xfs_trans_ail.c | 3 ++-
> fs/xfs/xfs_trans_priv.h | 14 ++++++++------
> 4 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 1f7acffc99ba..06e306b49283 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -410,7 +410,6 @@ xfs_buf_item_unpin(
> {
> struct xfs_buf_log_item *bip = BUF_ITEM(lip);
> xfs_buf_t *bp = bip->bli_buf;
> - struct xfs_ail *ailp = lip->li_ailp;
> int stale = bip->bli_flags & XFS_BLI_STALE;
> int freed;
>
> @@ -452,10 +451,10 @@ xfs_buf_item_unpin(
> }
>
> /*
> - * If we get called here because of an IO error, we may
> - * or may not have the item on the AIL. xfs_trans_ail_delete()
> - * will take care of that situation.
> - * xfs_trans_ail_delete() drops the AIL lock.
> + * If we get called here because of an IO error, we may or may
> + * not have the item on the AIL. xfs_trans_ail_delete() will
> + * take care of that situation. xfs_trans_ail_delete() drops
> + * the AIL lock.
> */
> if (bip->bli_flags & XFS_BLI_STALE_INODE) {
> xfs_buf_do_callbacks(bp);
> @@ -463,8 +462,7 @@ xfs_buf_item_unpin(
> list_del_init(&bp->b_li_list);
> bp->b_iodone = NULL;
> } else {
> - spin_lock(&ailp->ail_lock);
> - xfs_trans_ail_delete(ailp, lip, SHUTDOWN_LOG_IO_ERROR);
> + xfs_trans_ail_delete(lip, SHUTDOWN_LOG_IO_ERROR);
> xfs_buf_item_relse(bp);
> ASSERT(bp->b_log_item == NULL);
> }
> @@ -1205,22 +1203,19 @@ xfs_buf_iodone(
> struct xfs_buf *bp,
> struct xfs_log_item *lip)
> {
> - struct xfs_ail *ailp = lip->li_ailp;
> -
> ASSERT(BUF_ITEM(lip)->bli_buf == bp);
>
> xfs_buf_rele(bp);
>
> /*
> - * If we are forcibly shutting down, this may well be
> - * off the AIL already. That's because we simulate the
> - * log-committed callbacks to unpin these buffers. Or we may never
> - * have put this item on AIL because of the transaction was
> - * aborted forcibly. xfs_trans_ail_delete() takes care of these.
> + * If we are forcibly shutting down, this may well be off the AIL
> + * already. That's because we simulate the log-committed callbacks to
> + * unpin these buffers. Or we may never have put this item on AIL
> + * because of the transaction was aborted forcibly.
> + * xfs_trans_ail_delete() takes care of these.
> *
> * Either way, AIL is useless if we're forcing a shutdown.
> */
> - spin_lock(&ailp->ail_lock);
> - xfs_trans_ail_delete(ailp, lip, SHUTDOWN_CORRUPT_INCORE);
> + xfs_trans_ail_delete(lip, SHUTDOWN_CORRUPT_INCORE);
> xfs_buf_item_free(BUF_ITEM(lip));
> }
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index ffe607733c50..5fb65f43b980 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -1021,6 +1021,7 @@ xfs_qm_dqflush_done(
> struct xfs_dq_logitem *qip = (struct xfs_dq_logitem *)lip;
> struct xfs_dquot *dqp = qip->qli_dquot;
> struct xfs_ail *ailp = lip->li_ailp;
> + xfs_lsn_t tail_lsn;
>
> /*
> * We only want to pull the item from the AIL if its
> @@ -1034,10 +1035,11 @@ xfs_qm_dqflush_done(
> ((lip->li_lsn == qip->qli_flush_lsn) ||
> test_bit(XFS_LI_FAILED, &lip->li_flags))) {
>
> - /* xfs_trans_ail_delete() drops the AIL lock. */
> spin_lock(&ailp->ail_lock);
> if (lip->li_lsn == qip->qli_flush_lsn) {
> - xfs_trans_ail_delete(ailp, lip, SHUTDOWN_CORRUPT_INCORE);
> + /* xfs_ail_update_finish() drops the AIL lock */
> + tail_lsn = xfs_ail_delete_one(ailp, lip);
> + xfs_ail_update_finish(ailp, tail_lsn);
> } else {
> /*
> * Clear the failed state since we are about to drop the
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 2574d01e4a83..cfba691664c7 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -864,13 +864,14 @@ xfs_ail_delete_one(
> */
> void
> xfs_trans_ail_delete(
> - struct xfs_ail *ailp,
> struct xfs_log_item *lip,
> int shutdown_type)
> {
> + struct xfs_ail *ailp = lip->li_ailp;
> struct xfs_mount *mp = ailp->ail_mount;
> xfs_lsn_t tail_lsn;
>
> + spin_lock(&ailp->ail_lock);
> if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
> spin_unlock(&ailp->ail_lock);
> if (!XFS_FORCED_SHUTDOWN(mp)) {
> diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
> index 35655eac01a6..e4362fb8d483 100644
> --- a/fs/xfs/xfs_trans_priv.h
> +++ b/fs/xfs/xfs_trans_priv.h
> @@ -94,8 +94,7 @@ xfs_trans_ail_update(
> xfs_lsn_t xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip);
> void xfs_ail_update_finish(struct xfs_ail *ailp, xfs_lsn_t old_lsn)
> __releases(ailp->ail_lock);
> -void xfs_trans_ail_delete(struct xfs_ail *ailp, struct xfs_log_item *lip,
> - int shutdown_type);
> +void xfs_trans_ail_delete(struct xfs_log_item *lip, int shutdown_type);
>
> static inline void
> xfs_trans_ail_remove(
> @@ -103,13 +102,16 @@ xfs_trans_ail_remove(
> int shutdown_type)
> {
> struct xfs_ail *ailp = lip->li_ailp;
> + xfs_lsn_t tail_lsn;
>
> spin_lock(&ailp->ail_lock);
> - /* xfs_trans_ail_delete() drops the AIL lock */
> - if (test_bit(XFS_LI_IN_AIL, &lip->li_flags))
> - xfs_trans_ail_delete(ailp, lip, shutdown_type);
> - else
> + /* xfs_ail_update_finish() drops the AIL lock */
> + if (test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
> + tail_lsn = xfs_ail_delete_one(ailp, lip);
> + xfs_ail_update_finish(ailp, tail_lsn);
> + } else {
> spin_unlock(&ailp->ail_lock);
> + }
> }
>
> void xfs_ail_push(struct xfs_ail *, xfs_lsn_t);
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 11/17] xfs: use delete helper for items expected to be in AIL
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
` (9 preceding siblings ...)
2020-05-04 14:11 ` [PATCH v4 10/17] xfs: acquire ->ail_lock from xfs_trans_ail_delete() Brian Foster
@ 2020-05-04 14:11 ` Brian Foster
2020-05-05 23:17 ` Allison Collins
2020-05-04 14:11 ` [PATCH v4 12/17] xfs: drop unused shutdown parameter from xfs_trans_ail_remove() Brian Foster
` (6 subsequent siblings)
17 siblings, 1 reply; 37+ messages in thread
From: Brian Foster @ 2020-05-04 14:11 UTC (permalink / raw)
To: linux-xfs
Various intent log items call xfs_trans_ail_remove() with a log I/O
error shutdown type, but this helper historically checks whether an
item is in the AIL before calling xfs_trans_ail_delete(). This means
the shutdown check is essentially a no-op for users of
xfs_trans_ail_remove().
It is possible that some items might not be AIL resident when the
AIL remove attempt occurs, but this should be isolated to cases
where the filesystem has already shutdown. For example, this
includes abort of the transaction committing the intent and I/O
error of the iclog buffer committing the intent to the log.
Therefore, update these callsites to use xfs_trans_ail_delete() to
provide AIL state validation for the common path of items being
released and removed when associated done items commit to the
physical log.
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_bmap_item.c | 2 +-
fs/xfs/xfs_extfree_item.c | 2 +-
fs/xfs/xfs_refcount_item.c | 2 +-
fs/xfs/xfs_rmap_item.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index ee6f4229cebc..909221a4a8ab 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -51,7 +51,7 @@ xfs_bui_release(
{
ASSERT(atomic_read(&buip->bui_refcount) > 0);
if (atomic_dec_and_test(&buip->bui_refcount)) {
- xfs_trans_ail_remove(&buip->bui_item, SHUTDOWN_LOG_IO_ERROR);
+ xfs_trans_ail_delete(&buip->bui_item, SHUTDOWN_LOG_IO_ERROR);
xfs_bui_item_free(buip);
}
}
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 6ea847f6e298..cd98eba24884 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -55,7 +55,7 @@ xfs_efi_release(
{
ASSERT(atomic_read(&efip->efi_refcount) > 0);
if (atomic_dec_and_test(&efip->efi_refcount)) {
- xfs_trans_ail_remove(&efip->efi_item, SHUTDOWN_LOG_IO_ERROR);
+ xfs_trans_ail_delete(&efip->efi_item, SHUTDOWN_LOG_IO_ERROR);
xfs_efi_item_free(efip);
}
}
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 8eeed73928cd..712939a015a9 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -50,7 +50,7 @@ xfs_cui_release(
{
ASSERT(atomic_read(&cuip->cui_refcount) > 0);
if (atomic_dec_and_test(&cuip->cui_refcount)) {
- xfs_trans_ail_remove(&cuip->cui_item, SHUTDOWN_LOG_IO_ERROR);
+ xfs_trans_ail_delete(&cuip->cui_item, SHUTDOWN_LOG_IO_ERROR);
xfs_cui_item_free(cuip);
}
}
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 4911b68f95dd..ff949b32c051 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -50,7 +50,7 @@ xfs_rui_release(
{
ASSERT(atomic_read(&ruip->rui_refcount) > 0);
if (atomic_dec_and_test(&ruip->rui_refcount)) {
- xfs_trans_ail_remove(&ruip->rui_item, SHUTDOWN_LOG_IO_ERROR);
+ xfs_trans_ail_delete(&ruip->rui_item, SHUTDOWN_LOG_IO_ERROR);
xfs_rui_item_free(ruip);
}
}
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 11/17] xfs: use delete helper for items expected to be in AIL
2020-05-04 14:11 ` [PATCH v4 11/17] xfs: use delete helper for items expected to be in AIL Brian Foster
@ 2020-05-05 23:17 ` Allison Collins
0 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-05-05 23:17 UTC (permalink / raw)
To: Brian Foster, linux-xfs
On 5/4/20 7:11 AM, Brian Foster wrote:
> Various intent log items call xfs_trans_ail_remove() with a log I/O
> error shutdown type, but this helper historically checks whether an
> item is in the AIL before calling xfs_trans_ail_delete(). This means
> the shutdown check is essentially a no-op for users of
> xfs_trans_ail_remove().
>
> It is possible that some items might not be AIL resident when the
> AIL remove attempt occurs, but this should be isolated to cases
> where the filesystem has already shutdown. For example, this
> includes abort of the transaction committing the intent and I/O
> error of the iclog buffer committing the intent to the log.
> Therefore, update these callsites to use xfs_trans_ail_delete() to
> provide AIL state validation for the common path of items being
> released and removed when associated done items commit to the
> physical log.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks OK:
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
> ---
> fs/xfs/xfs_bmap_item.c | 2 +-
> fs/xfs/xfs_extfree_item.c | 2 +-
> fs/xfs/xfs_refcount_item.c | 2 +-
> fs/xfs/xfs_rmap_item.c | 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index ee6f4229cebc..909221a4a8ab 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -51,7 +51,7 @@ xfs_bui_release(
> {
> ASSERT(atomic_read(&buip->bui_refcount) > 0);
> if (atomic_dec_and_test(&buip->bui_refcount)) {
> - xfs_trans_ail_remove(&buip->bui_item, SHUTDOWN_LOG_IO_ERROR);
> + xfs_trans_ail_delete(&buip->bui_item, SHUTDOWN_LOG_IO_ERROR);
> xfs_bui_item_free(buip);
> }
> }
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 6ea847f6e298..cd98eba24884 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -55,7 +55,7 @@ xfs_efi_release(
> {
> ASSERT(atomic_read(&efip->efi_refcount) > 0);
> if (atomic_dec_and_test(&efip->efi_refcount)) {
> - xfs_trans_ail_remove(&efip->efi_item, SHUTDOWN_LOG_IO_ERROR);
> + xfs_trans_ail_delete(&efip->efi_item, SHUTDOWN_LOG_IO_ERROR);
> xfs_efi_item_free(efip);
> }
> }
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index 8eeed73928cd..712939a015a9 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -50,7 +50,7 @@ xfs_cui_release(
> {
> ASSERT(atomic_read(&cuip->cui_refcount) > 0);
> if (atomic_dec_and_test(&cuip->cui_refcount)) {
> - xfs_trans_ail_remove(&cuip->cui_item, SHUTDOWN_LOG_IO_ERROR);
> + xfs_trans_ail_delete(&cuip->cui_item, SHUTDOWN_LOG_IO_ERROR);
> xfs_cui_item_free(cuip);
> }
> }
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index 4911b68f95dd..ff949b32c051 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -50,7 +50,7 @@ xfs_rui_release(
> {
> ASSERT(atomic_read(&ruip->rui_refcount) > 0);
> if (atomic_dec_and_test(&ruip->rui_refcount)) {
> - xfs_trans_ail_remove(&ruip->rui_item, SHUTDOWN_LOG_IO_ERROR);
> + xfs_trans_ail_delete(&ruip->rui_item, SHUTDOWN_LOG_IO_ERROR);
> xfs_rui_item_free(ruip);
> }
> }
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 12/17] xfs: drop unused shutdown parameter from xfs_trans_ail_remove()
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
` (10 preceding siblings ...)
2020-05-04 14:11 ` [PATCH v4 11/17] xfs: use delete helper for items expected to be in AIL Brian Foster
@ 2020-05-04 14:11 ` Brian Foster
2020-05-05 23:20 ` Allison Collins
2020-05-04 14:11 ` [PATCH v4 13/17] xfs: combine xfs_trans_ail_[remove|delete]() Brian Foster
` (5 subsequent siblings)
17 siblings, 1 reply; 37+ messages in thread
From: Brian Foster @ 2020-05-04 14:11 UTC (permalink / raw)
To: linux-xfs
The shutdown parameter of xfs_trans_ail_remove() is no longer used.
The remaining callers use it for items that legitimately might not
be in the AIL or from contexts where AIL state has already been
checked. Remove the unnecessary parameter and fix up the callers.
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_buf_item.c | 2 +-
fs/xfs/xfs_dquot.c | 2 +-
fs/xfs/xfs_dquot_item.c | 2 +-
fs/xfs/xfs_inode_item.c | 6 +-----
fs/xfs/xfs_trans_priv.h | 3 +--
5 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 06e306b49283..47c547aca1f1 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -558,7 +558,7 @@ xfs_buf_item_put(
* state.
*/
if (aborted)
- xfs_trans_ail_remove(lip, SHUTDOWN_LOG_IO_ERROR);
+ xfs_trans_ail_remove(lip);
xfs_buf_item_relse(bip->bli_buf);
return true;
}
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 5fb65f43b980..497a9dbef1c9 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -1162,7 +1162,7 @@ xfs_qm_dqflush(
out_abort:
dqp->dq_flags &= ~XFS_DQ_DIRTY;
- xfs_trans_ail_remove(lip, SHUTDOWN_CORRUPT_INCORE);
+ xfs_trans_ail_remove(lip);
xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
out_unlock:
xfs_dqfunlock(dqp);
diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
index 5a7808299a32..8bd46810d5db 100644
--- a/fs/xfs/xfs_dquot_item.c
+++ b/fs/xfs/xfs_dquot_item.c
@@ -343,7 +343,7 @@ xfs_qm_qoff_logitem_relse(
ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags) ||
test_bit(XFS_LI_ABORTED, &lip->li_flags) ||
XFS_FORCED_SHUTDOWN(lip->li_mountp));
- xfs_trans_ail_remove(lip, SHUTDOWN_LOG_IO_ERROR);
+ xfs_trans_ail_remove(lip);
kmem_free(lip->li_lv_shadow);
kmem_free(qoff);
}
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 1d4d256a2e96..0e449d0a3d5c 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -768,11 +768,7 @@ xfs_iflush_abort(
xfs_inode_log_item_t *iip = ip->i_itemp;
if (iip) {
- 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);
- }
+ xfs_trans_ail_remove(&iip->ili_item);
iip->ili_logged = 0;
/*
* Clear the ili_last_fields bits now that we know that the
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index e4362fb8d483..ab0a82e90825 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -98,8 +98,7 @@ void xfs_trans_ail_delete(struct xfs_log_item *lip, int shutdown_type);
static inline void
xfs_trans_ail_remove(
- struct xfs_log_item *lip,
- int shutdown_type)
+ struct xfs_log_item *lip)
{
struct xfs_ail *ailp = lip->li_ailp;
xfs_lsn_t tail_lsn;
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 12/17] xfs: drop unused shutdown parameter from xfs_trans_ail_remove()
2020-05-04 14:11 ` [PATCH v4 12/17] xfs: drop unused shutdown parameter from xfs_trans_ail_remove() Brian Foster
@ 2020-05-05 23:20 ` Allison Collins
0 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-05-05 23:20 UTC (permalink / raw)
To: Brian Foster, linux-xfs
On 5/4/20 7:11 AM, Brian Foster wrote:
> The shutdown parameter of xfs_trans_ail_remove() is no longer used.
> The remaining callers use it for items that legitimately might not
> be in the AIL or from contexts where AIL state has already been
> checked. Remove the unnecessary parameter and fix up the callers.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks ok to me:
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
> ---
> fs/xfs/xfs_buf_item.c | 2 +-
> fs/xfs/xfs_dquot.c | 2 +-
> fs/xfs/xfs_dquot_item.c | 2 +-
> fs/xfs/xfs_inode_item.c | 6 +-----
> fs/xfs/xfs_trans_priv.h | 3 +--
> 5 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 06e306b49283..47c547aca1f1 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -558,7 +558,7 @@ xfs_buf_item_put(
> * state.
> */
> if (aborted)
> - xfs_trans_ail_remove(lip, SHUTDOWN_LOG_IO_ERROR);
> + xfs_trans_ail_remove(lip);
> xfs_buf_item_relse(bip->bli_buf);
> return true;
> }
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 5fb65f43b980..497a9dbef1c9 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -1162,7 +1162,7 @@ xfs_qm_dqflush(
>
> out_abort:
> dqp->dq_flags &= ~XFS_DQ_DIRTY;
> - xfs_trans_ail_remove(lip, SHUTDOWN_CORRUPT_INCORE);
> + xfs_trans_ail_remove(lip);
> xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> out_unlock:
> xfs_dqfunlock(dqp);
> diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
> index 5a7808299a32..8bd46810d5db 100644
> --- a/fs/xfs/xfs_dquot_item.c
> +++ b/fs/xfs/xfs_dquot_item.c
> @@ -343,7 +343,7 @@ xfs_qm_qoff_logitem_relse(
> ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags) ||
> test_bit(XFS_LI_ABORTED, &lip->li_flags) ||
> XFS_FORCED_SHUTDOWN(lip->li_mountp));
> - xfs_trans_ail_remove(lip, SHUTDOWN_LOG_IO_ERROR);
> + xfs_trans_ail_remove(lip);
> kmem_free(lip->li_lv_shadow);
> kmem_free(qoff);
> }
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 1d4d256a2e96..0e449d0a3d5c 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -768,11 +768,7 @@ xfs_iflush_abort(
> xfs_inode_log_item_t *iip = ip->i_itemp;
>
> if (iip) {
> - 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);
> - }
> + xfs_trans_ail_remove(&iip->ili_item);
> iip->ili_logged = 0;
> /*
> * Clear the ili_last_fields bits now that we know that the
> diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
> index e4362fb8d483..ab0a82e90825 100644
> --- a/fs/xfs/xfs_trans_priv.h
> +++ b/fs/xfs/xfs_trans_priv.h
> @@ -98,8 +98,7 @@ void xfs_trans_ail_delete(struct xfs_log_item *lip, int shutdown_type);
>
> static inline void
> xfs_trans_ail_remove(
> - struct xfs_log_item *lip,
> - int shutdown_type)
> + struct xfs_log_item *lip)
> {
> struct xfs_ail *ailp = lip->li_ailp;
> xfs_lsn_t tail_lsn;
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 13/17] xfs: combine xfs_trans_ail_[remove|delete]()
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
` (11 preceding siblings ...)
2020-05-04 14:11 ` [PATCH v4 12/17] xfs: drop unused shutdown parameter from xfs_trans_ail_remove() Brian Foster
@ 2020-05-04 14:11 ` Brian Foster
2020-05-05 23:35 ` Allison Collins
2020-05-04 14:11 ` [PATCH v4 14/17] xfs: remove unused iflush stale parameter Brian Foster
` (4 subsequent siblings)
17 siblings, 1 reply; 37+ messages in thread
From: Brian Foster @ 2020-05-04 14:11 UTC (permalink / raw)
To: linux-xfs
Now that the functions and callers of
xfs_trans_ail_[remove|delete]() have been fixed up appropriately,
the only difference between the two is the shutdown behavior. There
are only a few callers of the _remove() variant, so make the
shutdown conditional on the parameter and combine the two functions.
Suggested-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_buf_item.c | 2 +-
fs/xfs/xfs_dquot.c | 2 +-
fs/xfs/xfs_dquot_item.c | 2 +-
fs/xfs/xfs_inode_item.c | 2 +-
fs/xfs/xfs_trans_ail.c | 24 ++----------------------
fs/xfs/xfs_trans_priv.h | 17 -----------------
6 files changed, 6 insertions(+), 43 deletions(-)
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 47c547aca1f1..9e75e8d6042e 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -558,7 +558,7 @@ xfs_buf_item_put(
* state.
*/
if (aborted)
- xfs_trans_ail_remove(lip);
+ xfs_trans_ail_delete(lip, 0);
xfs_buf_item_relse(bip->bli_buf);
return true;
}
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 497a9dbef1c9..52e0f7245afc 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -1162,7 +1162,7 @@ xfs_qm_dqflush(
out_abort:
dqp->dq_flags &= ~XFS_DQ_DIRTY;
- xfs_trans_ail_remove(lip);
+ xfs_trans_ail_delete(lip, 0);
xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
out_unlock:
xfs_dqfunlock(dqp);
diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
index 8bd46810d5db..349c92d26570 100644
--- a/fs/xfs/xfs_dquot_item.c
+++ b/fs/xfs/xfs_dquot_item.c
@@ -343,7 +343,7 @@ xfs_qm_qoff_logitem_relse(
ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags) ||
test_bit(XFS_LI_ABORTED, &lip->li_flags) ||
XFS_FORCED_SHUTDOWN(lip->li_mountp));
- xfs_trans_ail_remove(lip);
+ xfs_trans_ail_delete(lip, 0);
kmem_free(lip->li_lv_shadow);
kmem_free(qoff);
}
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 0e449d0a3d5c..1a02058178d1 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -768,7 +768,7 @@ xfs_iflush_abort(
xfs_inode_log_item_t *iip = ip->i_itemp;
if (iip) {
- xfs_trans_ail_remove(&iip->ili_item);
+ xfs_trans_ail_delete(&iip->ili_item, 0);
iip->ili_logged = 0;
/*
* Clear the ili_last_fields bits now that we know that the
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index cfba691664c7..bf09d4b4df58 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -841,27 +841,6 @@ xfs_ail_delete_one(
return 0;
}
-/**
- * Remove a log items from the AIL
- *
- * @xfs_trans_ail_delete_bulk takes an array of log items that all need to
- * removed from the AIL. The caller is already holding the AIL lock, and done
- * all the checks necessary to ensure the items passed in via @log_items are
- * ready for deletion. This includes checking that the items are in the AIL.
- *
- * For each log item to be removed, unlink it from the AIL, clear the IN_AIL
- * flag from the item and reset the item's lsn to 0. If we remove the first
- * item in the AIL, update the log tail to match the new minimum LSN in the
- * AIL.
- *
- * This function will not drop the AIL lock until all items are removed from
- * the AIL to minimise the amount of lock traffic on the AIL. This does not
- * greatly increase the AIL hold time, but does significantly reduce the amount
- * of traffic on the lock, especially during IO completion.
- *
- * This function must be called with the AIL lock held. The lock is dropped
- * before returning.
- */
void
xfs_trans_ail_delete(
struct xfs_log_item *lip,
@@ -874,7 +853,7 @@ xfs_trans_ail_delete(
spin_lock(&ailp->ail_lock);
if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
spin_unlock(&ailp->ail_lock);
- if (!XFS_FORCED_SHUTDOWN(mp)) {
+ if (shutdown_type && !XFS_FORCED_SHUTDOWN(mp)) {
xfs_alert_tag(mp, XFS_PTAG_AILDELETE,
"%s: attempting to delete a log item that is not in the AIL",
__func__);
@@ -883,6 +862,7 @@ xfs_trans_ail_delete(
return;
}
+ /* xfs_ail_update_finish() drops the AIL lock */
tail_lsn = xfs_ail_delete_one(ailp, lip);
xfs_ail_update_finish(ailp, tail_lsn);
}
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index ab0a82e90825..cc046d9557ae 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -96,23 +96,6 @@ void xfs_ail_update_finish(struct xfs_ail *ailp, xfs_lsn_t old_lsn)
__releases(ailp->ail_lock);
void xfs_trans_ail_delete(struct xfs_log_item *lip, int shutdown_type);
-static inline void
-xfs_trans_ail_remove(
- struct xfs_log_item *lip)
-{
- struct xfs_ail *ailp = lip->li_ailp;
- xfs_lsn_t tail_lsn;
-
- spin_lock(&ailp->ail_lock);
- /* xfs_ail_update_finish() drops the AIL lock */
- if (test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
- tail_lsn = xfs_ail_delete_one(ailp, lip);
- xfs_ail_update_finish(ailp, tail_lsn);
- } else {
- spin_unlock(&ailp->ail_lock);
- }
-}
-
void xfs_ail_push(struct xfs_ail *, xfs_lsn_t);
void xfs_ail_push_all(struct xfs_ail *);
void xfs_ail_push_all_sync(struct xfs_ail *);
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 13/17] xfs: combine xfs_trans_ail_[remove|delete]()
2020-05-04 14:11 ` [PATCH v4 13/17] xfs: combine xfs_trans_ail_[remove|delete]() Brian Foster
@ 2020-05-05 23:35 ` Allison Collins
0 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-05-05 23:35 UTC (permalink / raw)
To: Brian Foster, linux-xfs
On 5/4/20 7:11 AM, Brian Foster wrote:
> Now that the functions and callers of
> xfs_trans_ail_[remove|delete]() have been fixed up appropriately,
> the only difference between the two is the shutdown behavior. There
> are only a few callers of the _remove() variant, so make the
> shutdown conditional on the parameter and combine the two functions.
>
> Suggested-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Ok, looks ok
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
> ---
> fs/xfs/xfs_buf_item.c | 2 +-
> fs/xfs/xfs_dquot.c | 2 +-
> fs/xfs/xfs_dquot_item.c | 2 +-
> fs/xfs/xfs_inode_item.c | 2 +-
> fs/xfs/xfs_trans_ail.c | 24 ++----------------------
> fs/xfs/xfs_trans_priv.h | 17 -----------------
> 6 files changed, 6 insertions(+), 43 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 47c547aca1f1..9e75e8d6042e 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -558,7 +558,7 @@ xfs_buf_item_put(
> * state.
> */
> if (aborted)
> - xfs_trans_ail_remove(lip);
> + xfs_trans_ail_delete(lip, 0);
> xfs_buf_item_relse(bip->bli_buf);
> return true;
> }
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 497a9dbef1c9..52e0f7245afc 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -1162,7 +1162,7 @@ xfs_qm_dqflush(
>
> out_abort:
> dqp->dq_flags &= ~XFS_DQ_DIRTY;
> - xfs_trans_ail_remove(lip);
> + xfs_trans_ail_delete(lip, 0);
> xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> out_unlock:
> xfs_dqfunlock(dqp);
> diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
> index 8bd46810d5db..349c92d26570 100644
> --- a/fs/xfs/xfs_dquot_item.c
> +++ b/fs/xfs/xfs_dquot_item.c
> @@ -343,7 +343,7 @@ xfs_qm_qoff_logitem_relse(
> ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags) ||
> test_bit(XFS_LI_ABORTED, &lip->li_flags) ||
> XFS_FORCED_SHUTDOWN(lip->li_mountp));
> - xfs_trans_ail_remove(lip);
> + xfs_trans_ail_delete(lip, 0);
> kmem_free(lip->li_lv_shadow);
> kmem_free(qoff);
> }
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 0e449d0a3d5c..1a02058178d1 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -768,7 +768,7 @@ xfs_iflush_abort(
> xfs_inode_log_item_t *iip = ip->i_itemp;
>
> if (iip) {
> - xfs_trans_ail_remove(&iip->ili_item);
> + xfs_trans_ail_delete(&iip->ili_item, 0);
> iip->ili_logged = 0;
> /*
> * Clear the ili_last_fields bits now that we know that the
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index cfba691664c7..bf09d4b4df58 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -841,27 +841,6 @@ xfs_ail_delete_one(
> return 0;
> }
>
> -/**
> - * Remove a log items from the AIL
> - *
> - * @xfs_trans_ail_delete_bulk takes an array of log items that all need to
> - * removed from the AIL. The caller is already holding the AIL lock, and done
> - * all the checks necessary to ensure the items passed in via @log_items are
> - * ready for deletion. This includes checking that the items are in the AIL.
> - *
> - * For each log item to be removed, unlink it from the AIL, clear the IN_AIL
> - * flag from the item and reset the item's lsn to 0. If we remove the first
> - * item in the AIL, update the log tail to match the new minimum LSN in the
> - * AIL.
> - *
> - * This function will not drop the AIL lock until all items are removed from
> - * the AIL to minimise the amount of lock traffic on the AIL. This does not
> - * greatly increase the AIL hold time, but does significantly reduce the amount
> - * of traffic on the lock, especially during IO completion.
> - *
> - * This function must be called with the AIL lock held. The lock is dropped
> - * before returning.
> - */
> void
> xfs_trans_ail_delete(
> struct xfs_log_item *lip,
> @@ -874,7 +853,7 @@ xfs_trans_ail_delete(
> spin_lock(&ailp->ail_lock);
> if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
> spin_unlock(&ailp->ail_lock);
> - if (!XFS_FORCED_SHUTDOWN(mp)) {
> + if (shutdown_type && !XFS_FORCED_SHUTDOWN(mp)) {
> xfs_alert_tag(mp, XFS_PTAG_AILDELETE,
> "%s: attempting to delete a log item that is not in the AIL",
> __func__);
> @@ -883,6 +862,7 @@ xfs_trans_ail_delete(
> return;
> }
>
> + /* xfs_ail_update_finish() drops the AIL lock */
> tail_lsn = xfs_ail_delete_one(ailp, lip);
> xfs_ail_update_finish(ailp, tail_lsn);
> }
> diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
> index ab0a82e90825..cc046d9557ae 100644
> --- a/fs/xfs/xfs_trans_priv.h
> +++ b/fs/xfs/xfs_trans_priv.h
> @@ -96,23 +96,6 @@ void xfs_ail_update_finish(struct xfs_ail *ailp, xfs_lsn_t old_lsn)
> __releases(ailp->ail_lock);
> void xfs_trans_ail_delete(struct xfs_log_item *lip, int shutdown_type);
>
> -static inline void
> -xfs_trans_ail_remove(
> - struct xfs_log_item *lip)
> -{
> - struct xfs_ail *ailp = lip->li_ailp;
> - xfs_lsn_t tail_lsn;
> -
> - spin_lock(&ailp->ail_lock);
> - /* xfs_ail_update_finish() drops the AIL lock */
> - if (test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
> - tail_lsn = xfs_ail_delete_one(ailp, lip);
> - xfs_ail_update_finish(ailp, tail_lsn);
> - } else {
> - spin_unlock(&ailp->ail_lock);
> - }
> -}
> -
> void xfs_ail_push(struct xfs_ail *, xfs_lsn_t);
> void xfs_ail_push_all(struct xfs_ail *);
> void xfs_ail_push_all_sync(struct xfs_ail *);
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 14/17] xfs: remove unused iflush stale parameter
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
` (12 preceding siblings ...)
2020-05-04 14:11 ` [PATCH v4 13/17] xfs: combine xfs_trans_ail_[remove|delete]() Brian Foster
@ 2020-05-04 14:11 ` Brian Foster
2020-05-04 14:11 ` [PATCH v4 15/17] xfs: random buffer write failure errortag Brian Foster
` (3 subsequent siblings)
17 siblings, 0 replies; 37+ messages in thread
From: Brian Foster @ 2020-05-04 14:11 UTC (permalink / raw)
To: linux-xfs
The stale parameter was used to control the now unused shutdown
parameter of xfs_trans_ail_remove().
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/xfs_icache.c | 2 +-
fs/xfs/xfs_inode.c | 2 +-
fs/xfs/xfs_inode_item.c | 7 +++----
fs/xfs/xfs_inode_item.h | 2 +-
4 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 8bf1d15be3f6..7032efcb6814 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -1128,7 +1128,7 @@ xfs_reclaim_inode(
if (XFS_FORCED_SHUTDOWN(ip->i_mount)) {
xfs_iunpin_wait(ip);
/* xfs_iflush_abort() drops the flush lock */
- xfs_iflush_abort(ip, false);
+ xfs_iflush_abort(ip);
goto reclaim;
}
if (xfs_ipincount(ip)) {
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 6fb3e26afa8b..e0d9a5bf7507 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3702,7 +3702,7 @@ xfs_iflush(
return 0;
abort:
- xfs_iflush_abort(ip, false);
+ xfs_iflush_abort(ip);
shutdown:
xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
return error;
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 1a02058178d1..cefa2484f0db 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -762,10 +762,9 @@ xfs_iflush_done(
*/
void
xfs_iflush_abort(
- xfs_inode_t *ip,
- bool stale)
+ struct xfs_inode *ip)
{
- xfs_inode_log_item_t *iip = ip->i_itemp;
+ struct xfs_inode_log_item *iip = ip->i_itemp;
if (iip) {
xfs_trans_ail_delete(&iip->ili_item, 0);
@@ -793,7 +792,7 @@ xfs_istale_done(
struct xfs_buf *bp,
struct xfs_log_item *lip)
{
- xfs_iflush_abort(INODE_ITEM(lip)->ili_inode, true);
+ xfs_iflush_abort(INODE_ITEM(lip)->ili_inode);
}
/*
diff --git a/fs/xfs/xfs_inode_item.h b/fs/xfs/xfs_inode_item.h
index 07a60e74c39c..a68c114b79a0 100644
--- a/fs/xfs/xfs_inode_item.h
+++ b/fs/xfs/xfs_inode_item.h
@@ -34,7 +34,7 @@ extern void xfs_inode_item_init(struct xfs_inode *, struct xfs_mount *);
extern void xfs_inode_item_destroy(struct xfs_inode *);
extern void xfs_iflush_done(struct xfs_buf *, struct xfs_log_item *);
extern void xfs_istale_done(struct xfs_buf *, struct xfs_log_item *);
-extern void xfs_iflush_abort(struct xfs_inode *, bool);
+extern void xfs_iflush_abort(struct xfs_inode *);
extern int xfs_inode_item_format_convert(xfs_log_iovec_t *,
struct xfs_inode_log_format *);
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v4 15/17] xfs: random buffer write failure errortag
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
` (13 preceding siblings ...)
2020-05-04 14:11 ` [PATCH v4 14/17] xfs: remove unused iflush stale parameter Brian Foster
@ 2020-05-04 14:11 ` Brian Foster
2020-05-04 14:11 ` [PATCH v4 16/17] xfs: remove unused shutdown types Brian Foster
` (2 subsequent siblings)
17 siblings, 0 replies; 37+ messages in thread
From: Brian Foster @ 2020-05-04 14:11 UTC (permalink / raw)
To: linux-xfs
Introduce an error tag to randomly fail async buffer writes. This is
primarily to facilitate testing of the XFS error configuration
mechanism.
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_errortag.h | 4 +++-
fs/xfs/xfs_buf.c | 6 ++++++
fs/xfs/xfs_error.c | 3 +++
3 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/libxfs/xfs_errortag.h b/fs/xfs/libxfs/xfs_errortag.h
index 79e6c4fb1d8a..2486dab19023 100644
--- a/fs/xfs/libxfs/xfs_errortag.h
+++ b/fs/xfs/libxfs/xfs_errortag.h
@@ -55,7 +55,8 @@
#define XFS_ERRTAG_FORCE_SCRUB_REPAIR 32
#define XFS_ERRTAG_FORCE_SUMMARY_RECALC 33
#define XFS_ERRTAG_IUNLINK_FALLBACK 34
-#define XFS_ERRTAG_MAX 35
+#define XFS_ERRTAG_BUF_IOERROR 35
+#define XFS_ERRTAG_MAX 36
/*
* Random factors for above tags, 1 means always, 2 means 1/2 time, etc.
@@ -95,5 +96,6 @@
#define XFS_RANDOM_FORCE_SCRUB_REPAIR 1
#define XFS_RANDOM_FORCE_SUMMARY_RECALC 1
#define XFS_RANDOM_IUNLINK_FALLBACK (XFS_RANDOM_DEFAULT/10)
+#define XFS_RANDOM_BUF_IOERROR XFS_RANDOM_DEFAULT
#endif /* __XFS_ERRORTAG_H_ */
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 8f0f605de579..61629efd56ee 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1289,6 +1289,12 @@ xfs_buf_bio_end_io(
struct bio *bio)
{
struct xfs_buf *bp = (struct xfs_buf *)bio->bi_private;
+ struct xfs_mount *mp = bp->b_mount;
+
+ if (!bio->bi_status &&
+ (bp->b_flags & XBF_WRITE) && (bp->b_flags & XBF_ASYNC) &&
+ XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BUF_IOERROR))
+ bio->bi_status = BLK_STS_IOERR;
/*
* don't overwrite existing errors - otherwise we can lose errors on
diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index a21e9cc6516a..7f6e20899473 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -53,6 +53,7 @@ static unsigned int xfs_errortag_random_default[] = {
XFS_RANDOM_FORCE_SCRUB_REPAIR,
XFS_RANDOM_FORCE_SUMMARY_RECALC,
XFS_RANDOM_IUNLINK_FALLBACK,
+ XFS_RANDOM_BUF_IOERROR,
};
struct xfs_errortag_attr {
@@ -162,6 +163,7 @@ XFS_ERRORTAG_ATTR_RW(buf_lru_ref, XFS_ERRTAG_BUF_LRU_REF);
XFS_ERRORTAG_ATTR_RW(force_repair, XFS_ERRTAG_FORCE_SCRUB_REPAIR);
XFS_ERRORTAG_ATTR_RW(bad_summary, XFS_ERRTAG_FORCE_SUMMARY_RECALC);
XFS_ERRORTAG_ATTR_RW(iunlink_fallback, XFS_ERRTAG_IUNLINK_FALLBACK);
+XFS_ERRORTAG_ATTR_RW(buf_ioerror, XFS_ERRTAG_BUF_IOERROR);
static struct attribute *xfs_errortag_attrs[] = {
XFS_ERRORTAG_ATTR_LIST(noerror),
@@ -199,6 +201,7 @@ static struct attribute *xfs_errortag_attrs[] = {
XFS_ERRORTAG_ATTR_LIST(force_repair),
XFS_ERRORTAG_ATTR_LIST(bad_summary),
XFS_ERRORTAG_ATTR_LIST(iunlink_fallback),
+ XFS_ERRORTAG_ATTR_LIST(buf_ioerror),
NULL,
};
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v4 16/17] xfs: remove unused shutdown types
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
` (14 preceding siblings ...)
2020-05-04 14:11 ` [PATCH v4 15/17] xfs: random buffer write failure errortag Brian Foster
@ 2020-05-04 14:11 ` Brian Foster
2020-05-05 23:37 ` Allison Collins
2020-05-04 14:11 ` [PATCH v4 17/17] xfs: remove unused iget_flags param from xfs_imap_to_bp() Brian Foster
2020-05-04 21:53 ` [PATCH v4 00/17] xfs: flush related error handling cleanups Dave Chinner
17 siblings, 1 reply; 37+ messages in thread
From: Brian Foster @ 2020-05-04 14:11 UTC (permalink / raw)
To: linux-xfs
Both types control shutdown messaging and neither is used in the
current codebase.
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/xfs_fsops.c | 5 +----
fs/xfs/xfs_mount.h | 2 --
2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 3e61d0cc23f8..ef1d5bb88b93 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -504,10 +504,7 @@ xfs_do_force_shutdown(
} else if (logerror) {
xfs_alert_tag(mp, XFS_PTAG_SHUTDOWN_LOGERROR,
"Log I/O Error Detected. Shutting down filesystem");
- } else if (flags & SHUTDOWN_DEVICE_REQ) {
- xfs_alert_tag(mp, XFS_PTAG_SHUTDOWN_IOERROR,
- "All device paths lost. Shutting down filesystem");
- } else if (!(flags & SHUTDOWN_REMOTE_REQ)) {
+ } else {
xfs_alert_tag(mp, XFS_PTAG_SHUTDOWN_IOERROR,
"I/O Error Detected. Shutting down filesystem");
}
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index b2e4598fdf7d..07b5ba7e5fbd 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -259,8 +259,6 @@ void xfs_do_force_shutdown(struct xfs_mount *mp, int flags, char *fname,
#define SHUTDOWN_LOG_IO_ERROR 0x0002 /* write attempt to the log failed */
#define SHUTDOWN_FORCE_UMOUNT 0x0004 /* shutdown from a forced unmount */
#define SHUTDOWN_CORRUPT_INCORE 0x0008 /* corrupt in-memory data structures */
-#define SHUTDOWN_REMOTE_REQ 0x0010 /* shutdown came from remote cell */
-#define SHUTDOWN_DEVICE_REQ 0x0020 /* failed all paths to the device */
/*
* Flags for xfs_mountfs
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 16/17] xfs: remove unused shutdown types
2020-05-04 14:11 ` [PATCH v4 16/17] xfs: remove unused shutdown types Brian Foster
@ 2020-05-05 23:37 ` Allison Collins
0 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-05-05 23:37 UTC (permalink / raw)
To: Brian Foster, linux-xfs
On 5/4/20 7:11 AM, Brian Foster wrote:
> Both types control shutdown messaging and neither is used in the
> current codebase.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com
Looks ok to me:
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
> ---
> fs/xfs/xfs_fsops.c | 5 +----
> fs/xfs/xfs_mount.h | 2 --
> 2 files changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 3e61d0cc23f8..ef1d5bb88b93 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -504,10 +504,7 @@ xfs_do_force_shutdown(
> } else if (logerror) {
> xfs_alert_tag(mp, XFS_PTAG_SHUTDOWN_LOGERROR,
> "Log I/O Error Detected. Shutting down filesystem");
> - } else if (flags & SHUTDOWN_DEVICE_REQ) {
> - xfs_alert_tag(mp, XFS_PTAG_SHUTDOWN_IOERROR,
> - "All device paths lost. Shutting down filesystem");
> - } else if (!(flags & SHUTDOWN_REMOTE_REQ)) {
> + } else {
> xfs_alert_tag(mp, XFS_PTAG_SHUTDOWN_IOERROR,
> "I/O Error Detected. Shutting down filesystem");
> }
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index b2e4598fdf7d..07b5ba7e5fbd 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -259,8 +259,6 @@ void xfs_do_force_shutdown(struct xfs_mount *mp, int flags, char *fname,
> #define SHUTDOWN_LOG_IO_ERROR 0x0002 /* write attempt to the log failed */
> #define SHUTDOWN_FORCE_UMOUNT 0x0004 /* shutdown from a forced unmount */
> #define SHUTDOWN_CORRUPT_INCORE 0x0008 /* corrupt in-memory data structures */
> -#define SHUTDOWN_REMOTE_REQ 0x0010 /* shutdown came from remote cell */
> -#define SHUTDOWN_DEVICE_REQ 0x0020 /* failed all paths to the device */
>
> /*
> * Flags for xfs_mountfs
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 17/17] xfs: remove unused iget_flags param from xfs_imap_to_bp()
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
` (15 preceding siblings ...)
2020-05-04 14:11 ` [PATCH v4 16/17] xfs: remove unused shutdown types Brian Foster
@ 2020-05-04 14:11 ` Brian Foster
2020-05-05 23:40 ` Allison Collins
2020-05-04 21:53 ` [PATCH v4 00/17] xfs: flush related error handling cleanups Dave Chinner
17 siblings, 1 reply; 37+ messages in thread
From: Brian Foster @ 2020-05-04 14:11 UTC (permalink / raw)
To: linux-xfs
iget_flags is unused in xfs_imap_to_bp(). Remove the parameter and
fix up the callers.
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_inode_buf.c | 5 ++---
fs/xfs/libxfs/xfs_inode_buf.h | 2 +-
fs/xfs/scrub/ialloc.c | 3 +--
fs/xfs/xfs_inode.c | 7 +++----
fs/xfs/xfs_log_recover.c | 2 +-
5 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index b102e611bf54..81a010422bea 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -161,8 +161,7 @@ xfs_imap_to_bp(
struct xfs_imap *imap,
struct xfs_dinode **dipp,
struct xfs_buf **bpp,
- uint buf_flags,
- uint iget_flags)
+ uint buf_flags)
{
struct xfs_buf *bp;
int error;
@@ -621,7 +620,7 @@ xfs_iread(
/*
* Get pointers to the on-disk inode and the buffer containing it.
*/
- error = xfs_imap_to_bp(mp, tp, &ip->i_imap, &dip, &bp, 0, iget_flags);
+ error = xfs_imap_to_bp(mp, tp, &ip->i_imap, &dip, &bp, 0);
if (error)
return error;
diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
index 9b373dcf9e34..d9b4781ac9fd 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.h
+++ b/fs/xfs/libxfs/xfs_inode_buf.h
@@ -48,7 +48,7 @@ struct xfs_imap {
int xfs_imap_to_bp(struct xfs_mount *, struct xfs_trans *,
struct xfs_imap *, struct xfs_dinode **,
- struct xfs_buf **, uint, uint);
+ struct xfs_buf **, uint);
int xfs_iread(struct xfs_mount *, struct xfs_trans *,
struct xfs_inode *, uint);
void xfs_dinode_calc_crc(struct xfs_mount *, struct xfs_dinode *);
diff --git a/fs/xfs/scrub/ialloc.c b/fs/xfs/scrub/ialloc.c
index 64c217eb06a7..6517d67e8d51 100644
--- a/fs/xfs/scrub/ialloc.c
+++ b/fs/xfs/scrub/ialloc.c
@@ -278,8 +278,7 @@ xchk_iallocbt_check_cluster(
&XFS_RMAP_OINFO_INODES);
/* Grab the inode cluster buffer. */
- error = xfs_imap_to_bp(mp, bs->cur->bc_tp, &imap, &dip, &cluster_bp,
- 0, 0);
+ error = xfs_imap_to_bp(mp, bs->cur->bc_tp, &imap, &dip, &cluster_bp, 0);
if (!xchk_btree_xref_process_error(bs->sc, bs->cur, 0, &error))
return error;
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index e0d9a5bf7507..4f915b91b9fd 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2172,7 +2172,7 @@ xfs_iunlink_update_inode(
ASSERT(xfs_verify_agino_or_null(mp, agno, next_agino));
- error = xfs_imap_to_bp(mp, tp, &ip->i_imap, &dip, &ibp, 0, 0);
+ error = xfs_imap_to_bp(mp, tp, &ip->i_imap, &dip, &ibp, 0);
if (error)
return error;
@@ -2302,7 +2302,7 @@ xfs_iunlink_map_ino(
return error;
}
- error = xfs_imap_to_bp(mp, tp, imap, dipp, bpp, 0, 0);
+ error = xfs_imap_to_bp(mp, tp, imap, dipp, bpp, 0);
if (error) {
xfs_warn(mp, "%s: xfs_imap_to_bp returned error %d.",
__func__, error);
@@ -3665,8 +3665,7 @@ xfs_iflush(
* If we get any other error, we effectively have a corruption situation
* and we cannot flush the inode. Abort the flush and shut down.
*/
- error = xfs_imap_to_bp(mp, NULL, &ip->i_imap, &dip, &bp, XBF_TRYLOCK,
- 0);
+ error = xfs_imap_to_bp(mp, NULL, &ip->i_imap, &dip, &bp, XBF_TRYLOCK);
if (error == -EAGAIN) {
xfs_ifunlock(ip);
return error;
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 11c3502b07b1..6a98fd9f00b3 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -4987,7 +4987,7 @@ xlog_recover_process_one_iunlink(
/*
* Get the on disk inode to find the next inode in the bucket.
*/
- error = xfs_imap_to_bp(mp, NULL, &ip->i_imap, &dip, &ibp, 0, 0);
+ error = xfs_imap_to_bp(mp, NULL, &ip->i_imap, &dip, &ibp, 0);
if (error)
goto fail_iput;
--
2.21.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 17/17] xfs: remove unused iget_flags param from xfs_imap_to_bp()
2020-05-04 14:11 ` [PATCH v4 17/17] xfs: remove unused iget_flags param from xfs_imap_to_bp() Brian Foster
@ 2020-05-05 23:40 ` Allison Collins
0 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-05-05 23:40 UTC (permalink / raw)
To: Brian Foster, linux-xfs
On 5/4/20 7:11 AM, Brian Foster wrote:
> iget_flags is unused in xfs_imap_to_bp(). Remove the parameter and
> fix up the callers.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Ok, looks fine to me
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
> ---
> fs/xfs/libxfs/xfs_inode_buf.c | 5 ++---
> fs/xfs/libxfs/xfs_inode_buf.h | 2 +-
> fs/xfs/scrub/ialloc.c | 3 +--
> fs/xfs/xfs_inode.c | 7 +++----
> fs/xfs/xfs_log_recover.c | 2 +-
> 5 files changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index b102e611bf54..81a010422bea 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -161,8 +161,7 @@ xfs_imap_to_bp(
> struct xfs_imap *imap,
> struct xfs_dinode **dipp,
> struct xfs_buf **bpp,
> - uint buf_flags,
> - uint iget_flags)
> + uint buf_flags)
> {
> struct xfs_buf *bp;
> int error;
> @@ -621,7 +620,7 @@ xfs_iread(
> /*
> * Get pointers to the on-disk inode and the buffer containing it.
> */
> - error = xfs_imap_to_bp(mp, tp, &ip->i_imap, &dip, &bp, 0, iget_flags);
> + error = xfs_imap_to_bp(mp, tp, &ip->i_imap, &dip, &bp, 0);
> if (error)
> return error;
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> index 9b373dcf9e34..d9b4781ac9fd 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.h
> +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> @@ -48,7 +48,7 @@ struct xfs_imap {
>
> int xfs_imap_to_bp(struct xfs_mount *, struct xfs_trans *,
> struct xfs_imap *, struct xfs_dinode **,
> - struct xfs_buf **, uint, uint);
> + struct xfs_buf **, uint);
> int xfs_iread(struct xfs_mount *, struct xfs_trans *,
> struct xfs_inode *, uint);
> void xfs_dinode_calc_crc(struct xfs_mount *, struct xfs_dinode *);
> diff --git a/fs/xfs/scrub/ialloc.c b/fs/xfs/scrub/ialloc.c
> index 64c217eb06a7..6517d67e8d51 100644
> --- a/fs/xfs/scrub/ialloc.c
> +++ b/fs/xfs/scrub/ialloc.c
> @@ -278,8 +278,7 @@ xchk_iallocbt_check_cluster(
> &XFS_RMAP_OINFO_INODES);
>
> /* Grab the inode cluster buffer. */
> - error = xfs_imap_to_bp(mp, bs->cur->bc_tp, &imap, &dip, &cluster_bp,
> - 0, 0);
> + error = xfs_imap_to_bp(mp, bs->cur->bc_tp, &imap, &dip, &cluster_bp, 0);
> if (!xchk_btree_xref_process_error(bs->sc, bs->cur, 0, &error))
> return error;
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index e0d9a5bf7507..4f915b91b9fd 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2172,7 +2172,7 @@ xfs_iunlink_update_inode(
>
> ASSERT(xfs_verify_agino_or_null(mp, agno, next_agino));
>
> - error = xfs_imap_to_bp(mp, tp, &ip->i_imap, &dip, &ibp, 0, 0);
> + error = xfs_imap_to_bp(mp, tp, &ip->i_imap, &dip, &ibp, 0);
> if (error)
> return error;
>
> @@ -2302,7 +2302,7 @@ xfs_iunlink_map_ino(
> return error;
> }
>
> - error = xfs_imap_to_bp(mp, tp, imap, dipp, bpp, 0, 0);
> + error = xfs_imap_to_bp(mp, tp, imap, dipp, bpp, 0);
> if (error) {
> xfs_warn(mp, "%s: xfs_imap_to_bp returned error %d.",
> __func__, error);
> @@ -3665,8 +3665,7 @@ xfs_iflush(
> * If we get any other error, we effectively have a corruption situation
> * and we cannot flush the inode. Abort the flush and shut down.
> */
> - error = xfs_imap_to_bp(mp, NULL, &ip->i_imap, &dip, &bp, XBF_TRYLOCK,
> - 0);
> + error = xfs_imap_to_bp(mp, NULL, &ip->i_imap, &dip, &bp, XBF_TRYLOCK);
> if (error == -EAGAIN) {
> xfs_ifunlock(ip);
> return error;
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 11c3502b07b1..6a98fd9f00b3 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -4987,7 +4987,7 @@ xlog_recover_process_one_iunlink(
> /*
> * Get the on disk inode to find the next inode in the bucket.
> */
> - error = xfs_imap_to_bp(mp, NULL, &ip->i_imap, &dip, &ibp, 0, 0);
> + error = xfs_imap_to_bp(mp, NULL, &ip->i_imap, &dip, &ibp, 0);
> if (error)
> goto fail_iput;
>
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v4 00/17] xfs: flush related error handling cleanups
2020-05-04 14:11 [PATCH v4 00/17] xfs: flush related error handling cleanups Brian Foster
` (16 preceding siblings ...)
2020-05-04 14:11 ` [PATCH v4 17/17] xfs: remove unused iget_flags param from xfs_imap_to_bp() Brian Foster
@ 2020-05-04 21:53 ` Dave Chinner
2020-05-05 11:58 ` Brian Foster
17 siblings, 1 reply; 37+ messages in thread
From: Dave Chinner @ 2020-05-04 21:53 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Mon, May 04, 2020 at 10:11:37AM -0400, Brian Foster wrote:
> Hi all,
>
> I think everything has been reviewed to this point. Only minor changes
> noted below in this release. A git repo is available here[1].
>
> The only outstanding feedback that I'm aware of is Dave's comment on
> patch 7 of v3 [2] regarding the shutdown assert check. I'm not aware of
> any means to get through xfs_wait_buftarg() with a dirty buffer that
> hasn't undergone the permanant error sequence and thus shut down the fs.
# echo 0 > /sys/fs/xfs/<dev>/fail_at_unmount
And now any error with a "retry forever" config (the default) will
be collected by xfs_buftarg_wait() without a preceeding shutdown as
xfs_buf_iodone_callback_error() will not treat it as a permanent
error during unmount. i.e. this doesn't trigger:
/* At unmount we may treat errors differently */
if ((mp->m_flags & XFS_MOUNT_UNMOUNTING) && mp->m_fail_unmount)
goto permanent_error;
and so the error handling just marks it with a write error and lets
it go for a write retry in future. These are then collected in
xfs_buftarg_wait() as nothing is going to retry them once unmount
gets to this point...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v4 00/17] xfs: flush related error handling cleanups
2020-05-04 21:53 ` [PATCH v4 00/17] xfs: flush related error handling cleanups Dave Chinner
@ 2020-05-05 11:58 ` Brian Foster
2020-05-05 22:36 ` Dave Chinner
0 siblings, 1 reply; 37+ messages in thread
From: Brian Foster @ 2020-05-05 11:58 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Tue, May 05, 2020 at 07:53:07AM +1000, Dave Chinner wrote:
> On Mon, May 04, 2020 at 10:11:37AM -0400, Brian Foster wrote:
> > Hi all,
> >
> > I think everything has been reviewed to this point. Only minor changes
> > noted below in this release. A git repo is available here[1].
> >
> > The only outstanding feedback that I'm aware of is Dave's comment on
> > patch 7 of v3 [2] regarding the shutdown assert check. I'm not aware of
> > any means to get through xfs_wait_buftarg() with a dirty buffer that
> > hasn't undergone the permanant error sequence and thus shut down the fs.
>
> # echo 0 > /sys/fs/xfs/<dev>/fail_at_unmount
>
> And now any error with a "retry forever" config (the default) will
> be collected by xfs_buftarg_wait() without a preceeding shutdown as
> xfs_buf_iodone_callback_error() will not treat it as a permanent
> error during unmount. i.e. this doesn't trigger:
>
> /* At unmount we may treat errors differently */
> if ((mp->m_flags & XFS_MOUNT_UNMOUNTING) && mp->m_fail_unmount)
> goto permanent_error;
>
> and so the error handling just marks it with a write error and lets
> it go for a write retry in future. These are then collected in
> xfs_buftarg_wait() as nothing is going to retry them once unmount
> gets to this point...
>
That doesn't accurately describe the behavior of that configuration,
though. "Retry forever" means that dirty buffers are going to cycle
through submission retries and the unmount is going to hang indefinitely
(on pushing the AIL). Indeed, preventing this unmount hang is the
original purpose of the fail at unmount knob (commit here[1]).
IOW, we don't get to xfs_wait_buftarg() in that scenario until all dirty
buffers are either written back successfully or the error configuration
changes to process the failures as permanent errors and shuts down the
fs. This can be confirmed easily with the buffer I/O error injection
patch (use a value of 1 to simulate persistent errors).
Brian
[1] e6b3bb78962e6 ("xfs: add "fail at unmount" error handling configuration")
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v4 00/17] xfs: flush related error handling cleanups
2020-05-05 11:58 ` Brian Foster
@ 2020-05-05 22:36 ` Dave Chinner
2020-05-06 11:04 ` Brian Foster
0 siblings, 1 reply; 37+ messages in thread
From: Dave Chinner @ 2020-05-05 22:36 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Tue, May 05, 2020 at 07:58:10AM -0400, Brian Foster wrote:
> On Tue, May 05, 2020 at 07:53:07AM +1000, Dave Chinner wrote:
> > On Mon, May 04, 2020 at 10:11:37AM -0400, Brian Foster wrote:
> > > Hi all,
> > >
> > > I think everything has been reviewed to this point. Only minor changes
> > > noted below in this release. A git repo is available here[1].
> > >
> > > The only outstanding feedback that I'm aware of is Dave's comment on
> > > patch 7 of v3 [2] regarding the shutdown assert check. I'm not aware of
> > > any means to get through xfs_wait_buftarg() with a dirty buffer that
> > > hasn't undergone the permanant error sequence and thus shut down the fs.
> >
> > # echo 0 > /sys/fs/xfs/<dev>/fail_at_unmount
> >
> > And now any error with a "retry forever" config (the default) will
> > be collected by xfs_buftarg_wait() without a preceeding shutdown as
> > xfs_buf_iodone_callback_error() will not treat it as a permanent
> > error during unmount. i.e. this doesn't trigger:
> >
> > /* At unmount we may treat errors differently */
> > if ((mp->m_flags & XFS_MOUNT_UNMOUNTING) && mp->m_fail_unmount)
> > goto permanent_error;
> >
> > and so the error handling just marks it with a write error and lets
> > it go for a write retry in future. These are then collected in
> > xfs_buftarg_wait() as nothing is going to retry them once unmount
> > gets to this point...
> >
>
> That doesn't accurately describe the behavior of that configuration,
> though. "Retry forever" means that dirty buffers are going to cycle
> through submission retries and the unmount is going to hang indefinitely
> (on pushing the AIL). Indeed, preventing this unmount hang is the
> original purpose of the fail at unmount knob (commit here[1]).
So this patch is relying on something else hanging before we get to
xfs_wait_buftarg and hence "we don't have to handle that here".
Please document that assumption along with the ASSERT, because if we
change AIL behaviour to prevent retry-forever hangs with freeze
and/or remount-ro we're going to hit this assert...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v4 00/17] xfs: flush related error handling cleanups
2020-05-05 22:36 ` Dave Chinner
@ 2020-05-06 11:04 ` Brian Foster
0 siblings, 0 replies; 37+ messages in thread
From: Brian Foster @ 2020-05-06 11:04 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Wed, May 06, 2020 at 08:36:43AM +1000, Dave Chinner wrote:
> On Tue, May 05, 2020 at 07:58:10AM -0400, Brian Foster wrote:
> > On Tue, May 05, 2020 at 07:53:07AM +1000, Dave Chinner wrote:
> > > On Mon, May 04, 2020 at 10:11:37AM -0400, Brian Foster wrote:
> > > > Hi all,
> > > >
> > > > I think everything has been reviewed to this point. Only minor changes
> > > > noted below in this release. A git repo is available here[1].
> > > >
> > > > The only outstanding feedback that I'm aware of is Dave's comment on
> > > > patch 7 of v3 [2] regarding the shutdown assert check. I'm not aware of
> > > > any means to get through xfs_wait_buftarg() with a dirty buffer that
> > > > hasn't undergone the permanant error sequence and thus shut down the fs.
> > >
> > > # echo 0 > /sys/fs/xfs/<dev>/fail_at_unmount
> > >
> > > And now any error with a "retry forever" config (the default) will
> > > be collected by xfs_buftarg_wait() without a preceeding shutdown as
> > > xfs_buf_iodone_callback_error() will not treat it as a permanent
> > > error during unmount. i.e. this doesn't trigger:
> > >
> > > /* At unmount we may treat errors differently */
> > > if ((mp->m_flags & XFS_MOUNT_UNMOUNTING) && mp->m_fail_unmount)
> > > goto permanent_error;
> > >
> > > and so the error handling just marks it with a write error and lets
> > > it go for a write retry in future. These are then collected in
> > > xfs_buftarg_wait() as nothing is going to retry them once unmount
> > > gets to this point...
> > >
> >
> > That doesn't accurately describe the behavior of that configuration,
> > though. "Retry forever" means that dirty buffers are going to cycle
> > through submission retries and the unmount is going to hang indefinitely
> > (on pushing the AIL). Indeed, preventing this unmount hang is the
> > original purpose of the fail at unmount knob (commit here[1]).
>
> So this patch is relying on something else hanging before we get to
> xfs_wait_buftarg and hence "we don't have to handle that here".
>
Yes, though it's the existing code that relies on the external push to
block until everything is written back or a permanent error is issued.
This patch just splits up and rate limits a warning message. I added the
assert because of the previous discussion around making sure this
situation is handled properly and because that error processing
dependency is not obvious.
> Please document that assumption along with the ASSERT, because if we
> change AIL behaviour to prevent retry-forever hangs with freeze
> and/or remount-ro we're going to hit this assert...
>
Sure, I'll add a comment..
Brian
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply [flat|nested] 37+ messages in thread