Re: [PATCH 05/12] xfs: make xfs_buf_read_map return an error code

[Christoph Hellwig <hch@xxxxxxxxxxxxx>]

On Wed, Jan 22, 2020 at 11:42:22PM -0800, Darrick J. Wong wrote:
> index fc93fd88ec89..df25024275a1 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -2956,14 +2956,13 @@ xfs_read_agf(
>  	trace_xfs_read_agf(mp, agno);
>  
>  	ASSERT(agno != NULLAGNUMBER);
> -	error = xfs_trans_read_buf(
> -			mp, tp, mp->m_ddev_targp,
> +	error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp,
>  			XFS_AG_DADDR(mp, agno, XFS_AGF_DADDR(mp)),
>  			XFS_FSS_TO_BB(mp, 1), flags, bpp, &xfs_agf_buf_ops);
> +	if (error == -EAGAIN)
> +		return 0;
>  	if (error)
>  		return error;
> -	if (!*bpp)
> -		return 0;

Shouldn't the change in calling conventions for xfs_trans_read_buf be
in another patch dealing just with xfs_trans_read_buf?

> +		/* bad CRC means corrupted metadata */
> +		if (error == -EFSBADCRC)
> +			error = -EFSCORRUPTED;
> +		return error;

Note that this coukd and should now also go away in the xfs_buf_read()
callers, not just the direct xfs_buf_read_map ones.

> +	error = xfs_buf_read_map(target, map, nmaps, flags, &bp, ops);
> +	switch (error) {
> +	case 0:
> +		break;
> +	case -EFSCORRUPTED:
> +	case -EIO:
>  		if (tp && (tp->t_flags & XFS_TRANS_DIRTY))
> +			xfs_force_shutdown(tp->t_mountp,
> +					SHUTDOWN_META_IO_ERROR);
> +		/* fall through */
> +	default:

Isn't it really EAGAIN the only special case here?  I.e. something
more like:

	if (error && error != -EAGAIN) {
  		if (tp && (tp->t_flags & XFS_TRANS_DIRTY))
			xfs_force_shutdown(tp->t_mountp,
					SHUTDOWN_META_IO_ERROR);
	}

	return error;