From: Brian Foster <bfoster@redhat.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 13/30 V2] xfs: handle buffer log item IO errors directly
Date: Fri, 5 Jun 2020 12:24:52 -0400 [thread overview]
Message-ID: <20200605162452.GA23747@bfoster> (raw)
In-Reply-To: <20200605013238.GA2040@dread.disaster.area>
On Fri, Jun 05, 2020 at 11:32:38AM +1000, Dave Chinner wrote:
>
> From: Dave Chinner <dchinner@redhat.com>
>
> Currently when a buffer with attached log items has an IO error
> it called ->iop_error for each attched log item. These all call
> xfs_set_li_failed() to handle the error, but we are about to change
> the way log items manage buffers. hence we first need to remove the
> per-item dependency on buffer handling done by xfs_set_li_failed().
>
> We already have specific buffer type IO completion routines, so move
> the log item error handling out of the generic error handling and
> into the log item specific functions so we can implement per-type
> error handling easily.
>
> This requires a more complex return value from the error handling
> code so that we can take the correct action the failure handling
> requires. This results in some repeated boilerplate in the
> functions, but that can be cleaned up later once all the changes
> cascade through this code.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> V2: fix buffer retry selection logic braino.
>
Looks good:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_buf_item.c | 214 +++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 144 insertions(+), 70 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 09bfe9c52dbdb..f80fc5bd3bffa 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -986,21 +986,24 @@ xfs_buf_do_callbacks_fail(
> spin_unlock(&ailp->ail_lock);
> }
>
> +/*
> + * Decide if we're going to retry the write after a failure, and prepare
> + * the buffer for retrying the write.
> + */
> static bool
> -xfs_buf_iodone_callback_error(
> +xfs_buf_ioerror_fail_without_retry(
> struct xfs_buf *bp)
> {
> struct xfs_mount *mp = bp->b_mount;
> static ulong lasttime;
> static xfs_buftarg_t *lasttarg;
> - struct xfs_error_cfg *cfg;
>
> /*
> * If we've already decided to shutdown the filesystem because of
> * I/O errors, there's no point in giving this a retry.
> */
> if (XFS_FORCED_SHUTDOWN(mp))
> - goto out_stale;
> + return true;
>
> if (bp->b_target != lasttarg ||
> time_after(jiffies, (lasttime + 5*HZ))) {
> @@ -1011,91 +1014,114 @@ xfs_buf_iodone_callback_error(
>
> /* synchronous writes will have callers process the error */
> if (!(bp->b_flags & XBF_ASYNC))
> - goto out_stale;
> -
> - trace_xfs_buf_item_iodone_async(bp, _RET_IP_);
> -
> - cfg = xfs_error_get_cfg(mp, XFS_ERR_METADATA, bp->b_error);
> + return true;
> + return false;
> +}
>
> - /*
> - * If the write was asynchronous then no one will be looking for the
> - * error. If this is the first failure of this type, clear the error
> - * state and write the buffer out again. This means we always retry an
> - * async write failure at least once, but we also need to set the buffer
> - * up to behave correctly now for repeated failures.
> - */
> - if (!(bp->b_flags & (XBF_STALE | XBF_WRITE_FAIL)) ||
> - bp->b_last_error != bp->b_error) {
> - bp->b_flags |= (XBF_WRITE | XBF_DONE | XBF_WRITE_FAIL);
> - bp->b_last_error = bp->b_error;
> - if (cfg->retry_timeout != XFS_ERR_RETRY_FOREVER &&
> - !bp->b_first_retry_time)
> - bp->b_first_retry_time = jiffies;
> +static bool
> +xfs_buf_ioerror_retry(
> + struct xfs_buf *bp,
> + struct xfs_error_cfg *cfg)
> +{
> + if ((bp->b_flags & (XBF_STALE | XBF_WRITE_FAIL)) &&
> + bp->b_last_error == bp->b_error)
> + return false;
>
> - xfs_buf_ioerror(bp, 0);
> - xfs_buf_submit(bp);
> - return true;
> - }
> + bp->b_flags |= (XBF_WRITE | XBF_DONE | XBF_WRITE_FAIL);
> + bp->b_last_error = bp->b_error;
> + if (cfg->retry_timeout != XFS_ERR_RETRY_FOREVER &&
> + !bp->b_first_retry_time)
> + bp->b_first_retry_time = jiffies;
> + return true;
> +}
>
> - /*
> - * Repeated failure on an async write. Take action according to the
> - * error configuration we have been set up to use.
> - */
> +/*
> + * Account for this latest trip around the retry handler, and decide if
> + * we've failed enough times to constitute a permanent failure.
> + */
> +static bool
> +xfs_buf_ioerror_permanent(
> + struct xfs_buf *bp,
> + struct xfs_error_cfg *cfg)
> +{
> + struct xfs_mount *mp = bp->b_mount;
>
> if (cfg->max_retries != XFS_ERR_RETRY_FOREVER &&
> ++bp->b_retries > cfg->max_retries)
> - goto permanent_error;
> + return true;
> if (cfg->retry_timeout != XFS_ERR_RETRY_FOREVER &&
> time_after(jiffies, cfg->retry_timeout + bp->b_first_retry_time))
> - goto permanent_error;
> + return true;
>
> /* At unmount we may treat errors differently */
> if ((mp->m_flags & XFS_MOUNT_UNMOUNTING) && mp->m_fail_unmount)
> - goto permanent_error;
> -
> - /*
> - * Still a transient error, run IO completion failure callbacks and let
> - * the higher layers retry the buffer.
> - */
> - xfs_buf_do_callbacks_fail(bp);
> - xfs_buf_ioerror(bp, 0);
> - xfs_buf_relse(bp);
> - return true;
> + return true;
>
> - /*
> - * Permanent error - we need to trigger a shutdown if we haven't already
> - * to indicate that inconsistency will result from this action.
> - */
> -permanent_error:
> - xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR);
> -out_stale:
> - xfs_buf_stale(bp);
> - bp->b_flags |= XBF_DONE;
> - trace_xfs_buf_error_relse(bp, _RET_IP_);
> return false;
> }
>
> -static inline bool
> -xfs_buf_had_callback_errors(
> +/*
> + * On a sync write or shutdown we just want to stale the buffer and let the
> + * caller handle the error in bp->b_error appropriately.
> + *
> + * If the write was asynchronous then no one will be looking for the error. If
> + * this is the first failure of this type, clear the error state and write the
> + * buffer out again. This means we always retry an async write failure at least
> + * once, but we also need to set the buffer up to behave correctly now for
> + * repeated failures.
> + *
> + * If we get repeated async write failures, then we take action according to the
> + * error configuration we have been set up to use.
> + *
> + * Multi-state return value:
> + *
> + * XBF_IOERROR_FINISH: clear IO error retry state and run callback completions
> + * XBF_IOERROR_DONE: resubmitted immediately, do not run any completions
> + * XBF_IOERROR_FAIL: transient error, run failure callback completions and then
> + * release the buffer
> + */
> +enum {
> + XBF_IOERROR_FINISH,
> + XBF_IOERROR_DONE,
> + XBF_IOERROR_FAIL,
> +};
> +
> +static int
> +xfs_buf_iodone_error(
> struct xfs_buf *bp)
> {
> + struct xfs_mount *mp = bp->b_mount;
> + struct xfs_error_cfg *cfg;
>
> - /*
> - * If there is an error, process it. Some errors require us to run
> - * callbacks after failure processing is done so we detect that and take
> - * appropriate action.
> - */
> - if (bp->b_error && xfs_buf_iodone_callback_error(bp))
> - return true;
> + if (xfs_buf_ioerror_fail_without_retry(bp))
> + goto out_stale;
> +
> + trace_xfs_buf_item_iodone_async(bp, _RET_IP_);
> +
> + cfg = xfs_error_get_cfg(mp, XFS_ERR_METADATA, bp->b_error);
> + if (xfs_buf_ioerror_retry(bp, cfg)) {
> + xfs_buf_ioerror(bp, 0);
> + xfs_buf_submit(bp);
> + return XBF_IOERROR_DONE;
> + }
>
> /*
> - * Successful IO or permanent error. Either way, we can clear the
> - * retry state here in preparation for the next error that may occur.
> + * Permanent error - we need to trigger a shutdown if we haven't already
> + * to indicate that inconsistency will result from this action.
> */
> - bp->b_last_error = 0;
> - bp->b_retries = 0;
> - bp->b_first_retry_time = 0;
> - return false;
> + if (xfs_buf_ioerror_permanent(bp, cfg)) {
> + xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR);
> + goto out_stale;
> + }
> +
> + /* Still considered a transient error. Caller will schedule retries. */
> + return XBF_IOERROR_FAIL;
> +
> +out_stale:
> + xfs_buf_stale(bp);
> + bp->b_flags |= XBF_DONE;
> + trace_xfs_buf_error_relse(bp, _RET_IP_);
> + return XBF_IOERROR_FINISH;
> }
>
> static void
> @@ -1122,6 +1148,15 @@ xfs_buf_item_done(
> xfs_buf_rele(bp);
> }
>
> +static inline void
> +xfs_buf_clear_ioerror_retry_state(
> + struct xfs_buf *bp)
> +{
> + bp->b_last_error = 0;
> + bp->b_retries = 0;
> + bp->b_first_retry_time = 0;
> +}
> +
> /*
> * Inode buffer iodone callback function.
> */
> @@ -1129,9 +1164,22 @@ void
> xfs_buf_inode_iodone(
> struct xfs_buf *bp)
> {
> - if (xfs_buf_had_callback_errors(bp))
> + if (bp->b_error) {
> + int ret = xfs_buf_iodone_error(bp);
> +
> + if (ret == XBF_IOERROR_FINISH)
> + goto finish_iodone;
> + if (ret == XBF_IOERROR_DONE)
> + return;
> + ASSERT(ret == XBF_IOERROR_FAIL);
> + xfs_buf_do_callbacks_fail(bp);
> + xfs_buf_ioerror(bp, 0);
> + xfs_buf_relse(bp);
> return;
> + }
>
> +finish_iodone:
> + xfs_buf_clear_ioerror_retry_state(bp);
> xfs_buf_item_done(bp);
> xfs_iflush_done(bp);
> xfs_buf_ioend_finish(bp);
> @@ -1144,9 +1192,22 @@ void
> xfs_buf_dquot_iodone(
> struct xfs_buf *bp)
> {
> - if (xfs_buf_had_callback_errors(bp))
> + if (bp->b_error) {
> + int ret = xfs_buf_iodone_error(bp);
> +
> + if (ret == XBF_IOERROR_FINISH)
> + goto finish_iodone;
> + if (ret == XBF_IOERROR_DONE)
> + return;
> + ASSERT(ret == XBF_IOERROR_FAIL);
> + xfs_buf_do_callbacks_fail(bp);
> + xfs_buf_ioerror(bp, 0);
> + xfs_buf_relse(bp);
> return;
> + }
>
> +finish_iodone:
> + xfs_buf_clear_ioerror_retry_state(bp);
> /* a newly allocated dquot buffer might have a log item attached */
> xfs_buf_item_done(bp);
> xfs_dquot_done(bp);
> @@ -1163,9 +1224,22 @@ void
> xfs_buf_iodone(
> struct xfs_buf *bp)
> {
> - if (xfs_buf_had_callback_errors(bp))
> + if (bp->b_error) {
> + int ret = xfs_buf_iodone_error(bp);
> +
> + if (ret == XBF_IOERROR_FINISH)
> + goto finish_iodone;
> + if (ret == XBF_IOERROR_DONE)
> + return;
> + ASSERT(ret == XBF_IOERROR_FAIL);
> + xfs_buf_do_callbacks_fail(bp);
> + xfs_buf_ioerror(bp, 0);
> + xfs_buf_relse(bp);
> return;
> + }
>
> +finish_iodone:
> + xfs_buf_clear_ioerror_retry_state(bp);
> xfs_buf_item_done(bp);
> xfs_buf_ioend_finish(bp);
> }
>
next prev parent reply other threads:[~2020-06-05 16:25 UTC|newest]
Thread overview: 79+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-04 7:45 [PATCH 00/30] xfs: rework inode flushing to make inode reclaim fully asynchronous Dave Chinner
2020-06-04 7:45 ` [PATCH 01/30] xfs: Don't allow logging of XFS_ISTALE inodes Dave Chinner
2020-06-04 7:45 ` [PATCH 02/30] xfs: remove logged flag from inode log item Dave Chinner
2020-06-04 7:45 ` [PATCH 03/30] xfs: add an inode item lock Dave Chinner
2020-06-09 13:13 ` Brian Foster
2020-06-04 7:45 ` [PATCH 04/30] xfs: mark inode buffers in cache Dave Chinner
2020-06-04 14:04 ` Brian Foster
2020-06-04 7:45 ` [PATCH 05/30] xfs: mark dquot " Dave Chinner
2020-06-04 7:45 ` [PATCH 06/30] xfs: mark log recovery buffers for completion Dave Chinner
2020-06-04 7:45 ` [PATCH 07/30] xfs: call xfs_buf_iodone directly Dave Chinner
2020-06-04 7:45 ` [PATCH 08/30] xfs: clean up whacky buffer log item list reinit Dave Chinner
2020-06-04 7:45 ` [PATCH 09/30] xfs: make inode IO completion buffer centric Dave Chinner
2020-06-04 7:45 ` [PATCH 10/30] xfs: use direct calls for dquot IO completion Dave Chinner
2020-06-04 7:45 ` [PATCH 11/30] xfs: clean up the buffer iodone callback functions Dave Chinner
2020-06-04 7:45 ` [PATCH 12/30] xfs: get rid of log item callbacks Dave Chinner
2020-06-04 7:45 ` [PATCH 13/30] xfs: handle buffer log item IO errors directly Dave Chinner
2020-06-04 14:05 ` Brian Foster
2020-06-05 0:59 ` Dave Chinner
2020-06-05 1:32 ` [PATCH 13/30 V2] " Dave Chinner
2020-06-05 16:24 ` Brian Foster [this message]
2020-06-04 7:45 ` [PATCH 14/30] xfs: unwind log item error flagging Dave Chinner
2020-06-04 7:45 ` [PATCH 15/30] xfs: move xfs_clear_li_failed out of xfs_ail_delete_one() Dave Chinner
2020-06-04 7:45 ` [PATCH 16/30] xfs: pin inode backing buffer to the inode log item Dave Chinner
2020-06-04 14:05 ` Brian Foster
2020-06-04 7:45 ` [PATCH 17/30] xfs: make inode reclaim almost non-blocking Dave Chinner
2020-06-04 18:06 ` Brian Foster
2020-06-04 7:45 ` [PATCH 18/30] xfs: remove IO submission from xfs_reclaim_inode() Dave Chinner
2020-06-04 18:08 ` Brian Foster
2020-06-04 22:53 ` Dave Chinner
2020-06-05 16:25 ` Brian Foster
2020-06-04 7:45 ` [PATCH 19/30] xfs: allow multiple reclaimers per AG Dave Chinner
2020-06-05 16:26 ` Brian Foster
2020-06-05 21:07 ` Dave Chinner
2020-06-08 16:44 ` Brian Foster
2020-06-04 7:45 ` [PATCH 20/30] xfs: don't block inode reclaim on the ILOCK Dave Chinner
2020-06-05 16:26 ` Brian Foster
2020-06-04 7:45 ` [PATCH 21/30] xfs: remove SYNC_TRYLOCK from inode reclaim Dave Chinner
2020-06-05 16:26 ` Brian Foster
2020-06-04 7:45 ` [PATCH 22/30] xfs: remove SYNC_WAIT from xfs_reclaim_inodes() Dave Chinner
2020-06-05 16:26 ` Brian Foster
2020-06-05 21:09 ` Dave Chinner
2020-06-04 7:45 ` [PATCH 23/30] xfs: clean up inode reclaim comments Dave Chinner
2020-06-05 16:26 ` Brian Foster
2020-06-04 7:46 ` [PATCH 24/30] xfs: rework stale inodes in xfs_ifree_cluster Dave Chinner
2020-06-05 18:27 ` Brian Foster
2020-06-05 21:32 ` Dave Chinner
2020-06-08 16:44 ` Brian Foster
2020-06-04 7:46 ` [PATCH 25/30] xfs: attach inodes to the cluster buffer when dirtied Dave Chinner
2020-06-08 16:45 ` Brian Foster
2020-06-08 21:05 ` Dave Chinner
2020-06-04 7:46 ` [PATCH 26/30] xfs: xfs_iflush() is no longer necessary Dave Chinner
2020-06-08 16:45 ` Brian Foster
2020-06-08 21:37 ` Dave Chinner
2020-06-08 22:26 ` [PATCH 26/30 V2] " Dave Chinner
2020-06-09 13:11 ` Brian Foster
2020-06-04 7:46 ` [PATCH 27/30] xfs: rename xfs_iflush_int() Dave Chinner
2020-06-08 17:37 ` Brian Foster
2020-06-04 7:46 ` [PATCH 28/30] xfs: rework xfs_iflush_cluster() dirty inode iteration Dave Chinner
2020-06-09 13:11 ` Brian Foster
2020-06-09 22:01 ` Dave Chinner
2020-06-10 13:06 ` Brian Foster
2020-06-10 23:40 ` Dave Chinner
2020-06-11 13:56 ` Brian Foster
2020-06-15 1:01 ` Dave Chinner
2020-06-15 14:21 ` Brian Foster
2020-06-16 14:41 ` Brian Foster
2020-06-11 1:56 ` [PATCH 28/30 V2] " Dave Chinner
2020-06-04 7:46 ` [PATCH 29/30] xfs: factor xfs_iflush_done Dave Chinner
2020-06-09 13:12 ` Brian Foster
2020-06-09 22:14 ` Dave Chinner
2020-06-10 13:08 ` Brian Foster
2020-06-11 0:16 ` Dave Chinner
2020-06-11 14:07 ` Brian Foster
2020-06-15 1:49 ` Dave Chinner
2020-06-15 5:20 ` Amir Goldstein
2020-06-15 14:31 ` Brian Foster
2020-06-11 1:58 ` [PATCH 29/30 V2] " Dave Chinner
2020-06-04 7:46 ` [PATCH 30/30] xfs: remove xfs_inobp_check() Dave Chinner
2020-06-09 13:12 ` Brian Foster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200605162452.GA23747@bfoster \
--to=bfoster@redhat.com \
--cc=david@fromorbit.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).