Re: [PATCH 12/13] xfs: remove xlog_recover_iodone

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

On Thu, Jul 09, 2020 at 05:04:52PM +0200, Christoph Hellwig wrote:
> The log recovery I/O completion handler does not substancially differ from

"substantially"

> the normal one except for the fact that we:
> 
>  a) never retry failed writes
>  b) can have log items that aren't on the AIL
>  c) never have inode/dquot log items attached and thus don't need to
>     handle them
> 
> Add conditionals for (a) and (b) to the ioend code, while (c) doesn't
> need special handling anyway.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

With that one typo fixed:

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

--D

> ---
>  fs/xfs/libxfs/xfs_log_recover.h |  1 -
>  fs/xfs/xfs_buf.c                | 20 ++++++++++++--------
>  fs/xfs/xfs_buf_item.c           |  4 ++++
>  fs/xfs/xfs_buf_item_recover.c   |  2 +-
>  fs/xfs/xfs_log_recover.c        | 25 -------------------------
>  5 files changed, 17 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
> index 641132d0e39ddd..3cca2bfe714cb2 100644
> --- a/fs/xfs/libxfs/xfs_log_recover.h
> +++ b/fs/xfs/libxfs/xfs_log_recover.h
> @@ -121,7 +121,6 @@ struct xlog_recover {
>  void xlog_buf_readahead(struct xlog *log, xfs_daddr_t blkno, uint len,
>  		const struct xfs_buf_ops *ops);
>  bool xlog_is_buffer_cancelled(struct xlog *log, xfs_daddr_t blkno, uint len);
> -void xlog_recover_iodone(struct xfs_buf *bp);
>  
>  void xlog_recover_release_intent(struct xlog *log, unsigned short intent_type,
>  		uint64_t intent_id);
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 8bbd28f39a927b..1172d5fa06aad2 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1241,6 +1241,15 @@ xfs_buf_ioend_handle_error(
>  
>  	xfs_buf_ioerror_alert_ratelimited(bp);
>  
> +	/*
> +	 * We're not going to bother about retrying this during recovery.
> +	 * One strike!
> +	 */
> +	if (bp->b_flags & _XBF_LOGRECOVERY) {
> +		xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR);
> +		return false;
> +	}
> +
>  	/*
>  	 * Synchronous writes will have callers process the error.
>  	 */
> @@ -1310,13 +1319,6 @@ xfs_buf_ioend(
>  			bp->b_ops->verify_read(bp);
>  		if (!bp->b_error)
>  			bp->b_flags |= XBF_DONE;
> -	} 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);
>  	} else {
>  		if (!bp->b_error) {
>  			bp->b_flags &= ~XBF_WRITE_FAIL;
> @@ -1343,9 +1345,11 @@ xfs_buf_ioend(
>  			xfs_buf_inode_iodone(bp);
>  		else if (bp->b_flags & _XBF_DQUOTS)
>  			xfs_buf_dquot_iodone(bp);
> +
>  	}
>  
> -	bp->b_flags &= ~(XBF_READ | XBF_WRITE | XBF_READ_AHEAD);
> +	bp->b_flags &= ~(XBF_READ | XBF_WRITE | XBF_READ_AHEAD |
> +			 _XBF_LOGRECOVERY);
>  
>  	if (bp->b_flags & XBF_ASYNC)
>  		xfs_buf_relse(bp);
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index ccfd747d32e410..ab87c1294f7584 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -967,8 +967,12 @@ xfs_buf_item_done(
>  	 * xfs_trans_ail_delete() takes care of these.
>  	 *
>  	 * Either way, AIL is useless if we're forcing a shutdown.
> +	 *
> +	 * Note that log recovery writes might have buffer items that are not on
> +	 * the AIL during normal operations.
>  	 */
>  	xfs_trans_ail_delete(&bp->b_log_item->bli_item,
> +			     (bp->b_flags & _XBF_LOGRECOVERY) ? 0 :
>  			     SHUTDOWN_CORRUPT_INCORE);
>  	xfs_buf_item_relse(bp);
>  }
> diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
> index 74c851f60eeeb1..1bbae3ab1322f7 100644
> --- a/fs/xfs/xfs_buf_item_recover.c
> +++ b/fs/xfs/xfs_buf_item_recover.c
> @@ -414,7 +414,7 @@ xlog_recover_validate_buf_type(
>  	 *
>  	 * Write verifiers update the metadata LSN from log items attached to
>  	 * the buffer. Therefore, initialize a bli purely to carry the LSN to
> -	 * the verifier. We'll clean it up in our ->iodone() callback.
> +	 * the verifier.
>  	 */
>  	if (bp->b_ops) {
>  		struct xfs_buf_log_item	*bip;
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 741a2c247bc585..b181f3253e6e74 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -265,31 +265,6 @@ xlog_header_check_mount(
>  	return 0;
>  }
>  
> -void
> -xlog_recover_iodone(
> -	struct xfs_buf	*bp)
> -{
> -	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!
> -		 */
> -		xfs_buf_ioerror_alert(bp, __this_address);
> -		xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR);
> -	}
> -
> -	/*
> -	 * On v5 supers, a bli could be attached to update the metadata LSN.
> -	 * Clean it up.
> -	 */
> -	if (bp->b_log_item)
> -		xfs_buf_item_relse(bp);
> -	ASSERT(bp->b_log_item == NULL);
> -	bp->b_flags &= ~_XBF_LOGRECOVERY;
> -}
> -
>  /*
>   * This routine finds (to an approximation) the first block in the physical
>   * log which contains the given cycle.  It uses a binary search algorithm.
> -- 
> 2.26.2
>