On Thu, Jul 09, 2020 at 05:04:49PM +0200, Christoph Hellwig wrote:
> Now that all the actual error handling is in a single place,
> xfs_buf_ioend_disposition just needs to return true if took ownership of
> the buffer, or false if not instead of the tristate. Also move the
Could you capture the meaning of the return values in the comment prior
to xfs_buf_ioend_handle_error, please? It took me a while to figure out
that "return false" means that the caller owns the buffer and log items
attached to it and needs to clear all the state that we set up for the
buffer write; and that this is true even for buffers that we just
permanent-failed.
Otherwise, this looks fairly straightforward to me.
--D
> error check back in the caller to optimize for the fast path, and give
> the function a better fitting name.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/xfs_buf.c | 34 ++++++----------------------------
> 1 file changed, 6 insertions(+), 28 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index e3e80615c5ed9e..4a9034a9175504 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1224,30 +1224,14 @@ xfs_buf_ioerror_permanent(
> *
> * 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_IOEND_FINISH: run callback completions
> - * XBF_IOEND_DONE: resubmitted immediately, do not run any completions
> - * XBF_IOEND_FAIL: transient error, run failure callback completions and then
> - * release the buffer
> */
> -enum xfs_buf_ioend_disposition {
> - XBF_IOEND_FINISH,
> - XBF_IOEND_DONE,
> - XBF_IOEND_FAIL,
> -};
> -
> -static enum xfs_buf_ioend_disposition
> -xfs_buf_ioend_disposition(
> +static bool
> +xfs_buf_ioend_handle_error(
> struct xfs_buf *bp)
> {
> struct xfs_mount *mp = bp->b_mount;
> struct xfs_error_cfg *cfg;
>
> - if (likely(!bp->b_error))
> - return XBF_IOEND_FINISH;
> -
> /*
> * If we've already decided to shutdown the filesystem because of I/O
> * errors, there's no point in giving this a retry.
> @@ -1293,18 +1277,18 @@ xfs_buf_ioend_disposition(
> ASSERT(list_empty(&bp->b_li_list));
> xfs_buf_ioerror(bp, 0);
> xfs_buf_relse(bp);
> - return XBF_IOEND_FAIL;
> + return true;
>
> resubmit:
> xfs_buf_ioerror(bp, 0);
> bp->b_flags |= (XBF_WRITE | XBF_DONE | XBF_WRITE_FAIL);
> xfs_buf_submit(bp);
> - return XBF_IOEND_DONE;
> + return true;
> out_stale:
> xfs_buf_stale(bp);
> bp->b_flags |= XBF_DONE;
> trace_xfs_buf_error_relse(bp, _RET_IP_);
> - return XBF_IOEND_FINISH;
> + return false;
> }
>
> static void
> @@ -1342,14 +1326,8 @@ xfs_buf_ioend(
> bp->b_flags |= XBF_DONE;
> }
>
> - switch (xfs_buf_ioend_disposition(bp)) {
> - case XBF_IOEND_DONE:
> + if (unlikely(bp->b_error) && xfs_buf_ioend_handle_error(bp))
> return;
> - case XBF_IOEND_FAIL:
> - return;
> - default:
> - break;
> - }
>
> /* clear the retry state */
> bp->b_last_error = 0;
> --
> 2.26.2
>
