Re: [PATCH 03/13] xfs: refactor xfs_buf_ioend

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

On Thu, Jul 09, 2020 at 05:04:43PM +0200, Christoph Hellwig wrote:
> Refactor the logic for the different I/O completions to prepare for
> more code sharing.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I think it's worth mentioning in the commit log that this patch leaves
the log recovery buffer completion code totally in charge of the buffer
state.  Not sure where exactly that part is going, though I guess your
eventual goal is to remove xlog_recover_iodone (and clean up the buffer
log item disposal)...?

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

--D

> ---
>  fs/xfs/xfs_buf.c         | 41 +++++++++++++++++-----------------------
>  fs/xfs/xfs_log_recover.c | 14 +++++++-------
>  2 files changed, 24 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 1bce6457a9b943..7c22d63f43f754 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1199,33 +1199,26 @@ xfs_buf_ioend(
>  		if (!bp->b_error)
>  			bp->b_flags |= XBF_DONE;
>  		xfs_buf_ioend_finish(bp);
> -		return;
> -	}
> -
> -	if (!bp->b_error) {
> -		bp->b_flags &= ~XBF_WRITE_FAIL;
> -		bp->b_flags |= XBF_DONE;
> -	}
> -
> -	/*
> -	 * If this is a log recovery buffer, we aren't doing transactional IO
> -	 * yet so we need to let it handle IO completions.
> -	 */
> -	if (bp->b_flags & _XBF_LOGRECOVERY) {
> +	} else if (bp->b_flags & _XBF_LOGRECOVERY) {
> +		/*
> +		 * If this is a log recovery buffer, we aren't doing
> +		 * transactional I/O yet so we need to let the log recovery code
> +		 * handle I/O completions:
> +		 */
>  		xlog_recover_iodone(bp);
> -		return;
> -	}
> -
> -	if (bp->b_flags & _XBF_INODES) {
> -		xfs_buf_inode_iodone(bp);
> -		return;
> -	}
> +	} else {
> +		if (!bp->b_error) {
> +			bp->b_flags &= ~XBF_WRITE_FAIL;
> +			bp->b_flags |= XBF_DONE;
> +		}
>  
> -	if (bp->b_flags & _XBF_DQUOTS) {
> -		xfs_buf_dquot_iodone(bp);
> -		return;
> +		if (bp->b_flags & _XBF_INODES)
> +			xfs_buf_inode_iodone(bp);
> +		else if (bp->b_flags & _XBF_DQUOTS)
> +			xfs_buf_dquot_iodone(bp);
> +		else
> +			xfs_buf_iodone(bp);
>  	}
> -	xfs_buf_iodone(bp);
>  }
>  
>  static void
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 52a65a74208ffe..cf6dabb98f2327 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -269,15 +269,15 @@ void
>  xlog_recover_iodone(
>  	struct xfs_buf	*bp)
>  {
> -	if (bp->b_error) {
> +	if (!bp->b_error) {
> +		bp->b_flags |= XBF_DONE;
> +	} else if (!XFS_FORCED_SHUTDOWN(bp->b_mount)) {
>  		/*
> -		 * We're not going to bother about retrying
> -		 * this during recovery. One strike!
> +		 * We're not going to bother about retrying this during
> +		 * recovery. One strike!
>  		 */
> -		if (!XFS_FORCED_SHUTDOWN(bp->b_mount)) {
> -			xfs_buf_ioerror_alert(bp, __this_address);
> -			xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR);
> -		}
> +		xfs_buf_ioerror_alert(bp, __this_address);
> +		xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR);
>  	}
>  
>  	/*
> -- 
> 2.26.2
>