On Wed, Apr 22, 2020 at 01:54:21PM -0400, Brian Foster wrote:
> At unmount time, XFS emits a warning 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.
>
> We already have a ratelimit state defined for buffers failing
> writeback. Fold this state into the buftarg and reuse it for the
> unmount time errors.
>
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
Looks fine, but I suspect we both missed something here:
xfs_buf_ioerror_alert() was made a ratelimited printk in the last
cycle:
void
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);
}
Hence I think all these buffer error alerts can be brought under the
same rate limiting variable. Something like this in xfs_message.c:
void
xfs_buf_alert_ratelimited(
struct xfs_buf *bp,
const char *rlmsg,
const char *fmt,
...)
{
struct va_format vaf;
va_list args;
if (!___ratelimit(&bp->b_target->bt_ioerror_rl, rlmsg)
return;
va_start(args, fmt);
vaf.fmt = fmt;
vaf.args = &args;
__xfs_printk(KERN_ALERT, bp->b_mount, &vaf);
va_end(args);
}
and:
void
xfs_buf_ioerror_alert(
struct xfs_buf *bp,
xfs_failaddr_t func)
{
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);
}
> ---
> fs/xfs/xfs_buf.c | 13 +++++++++++--
> fs/xfs/xfs_buf.h | 1 +
> fs/xfs/xfs_buf_item.c | 10 +---------
> 3 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 7a6bc617f0a9..c28a93d2fd8c 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1684,10 +1684,12 @@ xfs_wait_buftarg(
> struct xfs_buf *bp;
> bp = list_first_entry(&dispose, struct xfs_buf, b_lru);
> list_del_init(&bp->b_lru);
> - if (bp->b_flags & XBF_WRITE_FAIL) {
> + if (bp->b_flags & XBF_WRITE_FAIL &&
> + ___ratelimit(&bp->b_target->bt_ioerror_rl,
> + "XFS: Corruption Alert")) {
> xfs_alert(btp->bt_mount,
> "Corruption Alert: Buffer at daddr 0x%llx had permanent write failures!",
> - (long long)bp->b_bn);
> + (long long)bp->b_bn);
> xfs_alert(btp->bt_mount,
> "Please run xfs_repair to determine the extent of the problem.");
> }
I think if we are tossing away metadata here, we should probably
shut down the filesystem once the loop has completed. That way we
get all the normal warnings about running xfs_repair and don't have
to open code it here...
> -
> STATIC uint
> xfs_buf_item_push(
> struct xfs_log_item *lip,
> @@ -518,7 +510,7 @@ xfs_buf_item_push(
>
> /* 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")) {
> + ___ratelimit(&bp->b_target->bt_ioerror_rl, "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);
This gets simplified to:
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);
}
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
