Re: [PATCH v3 02/17] xfs: factor out buffer I/O failure code

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Apr 29, 2020 at 01:21:38PM -0400, Brian Foster wrote:
> 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>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  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
>