Re: [PATCH] xfs: lobotomise xfs_trans_read_buf_map()

[Christoph Hellwig <hch@infradead.org>]

On Tue, Dec 02, 2014 at 09:34:50AM +1100, Dave Chinner wrote:
> The only way we can find a buffer that has not had IO completed on
> it is if it had readahead issued on it, but we never do readahead on
> buffers that we have already joined into a transaction. Hence this
> condition cannot occur, and buffers locked and joined into a
> transaction should always be marked done and not under IO.

Should we add an ASSERT that would trigger when someone tries
to issue readahead on a buffer with b_transp set?

>  	bp = xfs_buf_read_map(target, map, nmaps, flags, ops);
> -	if (bp == NULL) {
> -		*bpp = NULL;
> -		return (flags & XBF_TRYLOCK) ?
> -					0 : -ENOMEM;
> +	if (!bp) {
> +		if (!(flags & XBF_TRYLOCK))
> +			return -ENOMEM;
> +		return tp ? 0 : -EAGAIN;

Can you fix the inconsistent return for the trylock case in a follow on
patch?  This difference doesn't look intentional to me, and I would
be surprised if it's correctly handled in the callers.

>  	}
> +
>  	if (bp->b_error) {
>  		error = bp->b_error;
> +		if (!XFS_FORCED_SHUTDOWN(mp))
> +			xfs_buf_ioerror_alert(bp, __func__);
> +		bp->b_flags &= ~XBF_DONE;
>  		xfs_buf_stale(bp);
> -		XFS_BUF_DONE(bp);

The old non-tp case did a XFS_BUF_UNDONE, which you open code here,
while the with-tp case did a XFS_BUF_DONE.  I think this change needs
a little explanation.

>  #ifdef DEBUG
> -	if (xfs_do_error && !(tp->t_flags & XFS_TRANS_DIRTY)) {
> +	if (xfs_do_error && (!tp || !(tp->t_flags & XFS_TRANS_DIRTY))) {
>  		if (xfs_error_target == target) {
>  			if (((xfs_req_num++) % xfs_error_mod) == 0) {
> -				xfs_force_shutdown(tp->t_mountp,
> -						   SHUTDOWN_META_IO_ERROR);
>  				xfs_buf_relse(bp);
> -				xfs_debug(mp, "Returning trans error!");
> +				xfs_debug(mp, "Returning error!");
>  				return -EIO;
>  			}
>  		}

I would suggest to kill this xfs_do_error error code, it's the last
use of the never initialized xfs_do_error and xfs_error_target
variables.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs