Re: [PATCH 02/12] xfs: factor out buffer I/O failure simulation code

[Allison Collins <allison.henderson@oracle.com>]

On 4/17/20 8:08 AM, Brian Foster wrote:
> We use the same buffer I/O failure simulation 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>
Ok, looks ok to me:
Reviewed-by: Allison Collins <allison.henderson@oracle.com>

> ---
>   fs/xfs/xfs_buf.c      | 23 +++++++++++++++++++----
>   fs/xfs/xfs_buf.h      |  1 +
>   fs/xfs/xfs_buf_item.c | 22 +++-------------------
>   fs/xfs/xfs_inode.c    |  7 +------
>   4 files changed, 24 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 9ec3eaf1c618..93942d8e35dd 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1248,6 +1248,24 @@ 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_iofail(
> +	struct xfs_buf	*bp,
> +	int		flags)
> +{
> +	bp->b_flags |= flags;
> +	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 +1498,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_iofail(bp, 0);
>   		return -EIO;
>   	}
>   
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 9a04c53c2488..a6bce4702b2e 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_iofail(struct xfs_buf *, int);
>   
>   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..72d37a4609d8 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -471,28 +471,12 @@ 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_iofail(bp, XBF_ASYNC);
>   	}
>   }
>   
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index d1772786af29..b539ee221ce5 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3629,12 +3629,7 @@ xfs_iflush_cluster(
>   	 * xfs_buf_submit().
>   	 */
>   	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_iofail(bp, XBF_ASYNC);
>   	xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
>   
>   	/* abort the corrupt inode, as it was not attached to the buffer */
>