From 2d3d0c53df99587e1d58759f805c3aae79fac453 Mon Sep 17 00:00:00 2001 From: Dave Chinner Date: Thu, 4 Dec 2014 09:43:13 +1100 Subject: [PATCH] xfs: lobotomise xfs_trans_read_buf_map() There's a case in that code where it checks for a buffer match in a transaction where the buffer is not marked done. i.e. trying to catch a buffer we have locked in the transaction but have not completed IO on. 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. Remove this code and re-order xfs_trans_read_buf_map() to remove duplicated IO dispatch and error handling code. Signed-off-by: Dave Chinner Reviewed-by: Christoph Hellwig Signed-off-by: Dave Chinner --- fs/xfs/xfs_trans_buf.c | 135 ++++++++++------------------------------- 1 file changed, 33 insertions(+), 102 deletions(-) diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c index e2b2216b1635..2f363cdd1e14 100644 --- a/fs/xfs/xfs_trans_buf.c +++ b/fs/xfs/xfs_trans_buf.c @@ -229,13 +229,6 @@ xfs_trans_getsb(xfs_trans_t *tp, return bp; } -#ifdef DEBUG -xfs_buftarg_t *xfs_error_target; -int xfs_do_error; -int xfs_req_num; -int xfs_error_mod = 33; -#endif - /* * Get and lock the buffer for the caller if it is not already * locked within the given transaction. If it has not yet been @@ -257,46 +250,11 @@ xfs_trans_read_buf_map( struct xfs_buf **bpp, const struct xfs_buf_ops *ops) { - xfs_buf_t *bp; - xfs_buf_log_item_t *bip; + struct xfs_buf *bp = NULL; + struct xfs_buf_log_item *bip; int error; *bpp = NULL; - if (!tp) { - bp = xfs_buf_read_map(target, map, nmaps, flags, ops); - if (!bp) - return (flags & XBF_TRYLOCK) ? - -EAGAIN : -ENOMEM; - - if (bp->b_error) { - error = bp->b_error; - xfs_buf_ioerror_alert(bp, __func__); - XFS_BUF_UNDONE(bp); - xfs_buf_stale(bp); - xfs_buf_relse(bp); - - /* bad CRC means corrupted metadata */ - if (error == -EFSBADCRC) - error = -EFSCORRUPTED; - return error; - } -#ifdef DEBUG - if (xfs_do_error) { - if (xfs_error_target == target) { - if (((xfs_req_num++) % xfs_error_mod) == 0) { - xfs_buf_relse(bp); - xfs_debug(mp, "Returning error!"); - return -EIO; - } - } - } -#endif - if (XFS_FORCED_SHUTDOWN(mp)) - goto shutdown_abort; - *bpp = bp; - return 0; - } - /* * If we find the buffer in the cache with this transaction * pointer in its b_fsprivate2 field, then we know we already @@ -305,49 +263,24 @@ xfs_trans_read_buf_map( * If the buffer is not yet read in, then we read it in, increment * the lock recursion count, and return it to the caller. */ - bp = xfs_trans_buf_item_match(tp, target, map, nmaps); - if (bp != NULL) { + if (tp) + bp = xfs_trans_buf_item_match(tp, target, map, nmaps); + if (bp) { ASSERT(xfs_buf_islocked(bp)); ASSERT(bp->b_transp == tp); ASSERT(bp->b_fspriv != NULL); ASSERT(!bp->b_error); - if (!(XFS_BUF_ISDONE(bp))) { - trace_xfs_trans_read_buf_io(bp, _RET_IP_); - ASSERT(!XFS_BUF_ISASYNC(bp)); - ASSERT(bp->b_iodone == NULL); - XFS_BUF_READ(bp); - bp->b_ops = ops; - - error = xfs_buf_submit_wait(bp); - if (error) { - if (!XFS_FORCED_SHUTDOWN(mp)) - xfs_buf_ioerror_alert(bp, __func__); - xfs_buf_relse(bp); - /* - * We can gracefully recover from most read - * errors. Ones we can't are those that happen - * after the transaction's already dirty. - */ - if (tp->t_flags & XFS_TRANS_DIRTY) - xfs_force_shutdown(tp->t_mountp, - SHUTDOWN_META_IO_ERROR); - /* bad CRC means corrupted metadata */ - if (error == -EFSBADCRC) - error = -EFSCORRUPTED; - return error; - } - } + ASSERT(bp->b_flags & XBF_DONE); + /* * We never locked this buf ourselves, so we shouldn't * brelse it either. Just get out. */ if (XFS_FORCED_SHUTDOWN(mp)) { trace_xfs_trans_read_buf_shut(bp, _RET_IP_); - *bpp = NULL; return -EIO; } - bip = bp->b_fspriv; bip->bli_recur++; @@ -358,17 +291,29 @@ xfs_trans_read_buf_map( } 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; } + + /* + * If we've had a read error, then the contents of the buffer are + * invalid and should not be used. To ensure that a followup read tries + * to pull the buffer from disk again, we clear the XBF_DONE flag and + * mark the buffer stale. This ensures that anyone who has a current + * reference to the buffer will interpret it's contents correctly and + * future cache lookups will also treat it as an empty, uninitialised + * buffer. + */ 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); - xfs_buf_ioerror_alert(bp, __func__); - if (tp->t_flags & XFS_TRANS_DIRTY) + + if (tp && (tp->t_flags & XFS_TRANS_DIRTY)) xfs_force_shutdown(tp->t_mountp, SHUTDOWN_META_IO_ERROR); xfs_buf_relse(bp); @@ -377,33 +322,19 @@ xfs_trans_read_buf_map( error = -EFSCORRUPTED; return error; } -#ifdef DEBUG - if (xfs_do_error && !(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!"); - return -EIO; - } - } + + if (XFS_FORCED_SHUTDOWN(mp)) { + xfs_buf_relse(bp); + trace_xfs_trans_read_buf_shut(bp, _RET_IP_); + return -EIO; } -#endif - if (XFS_FORCED_SHUTDOWN(mp)) - goto shutdown_abort; - _xfs_trans_bjoin(tp, bp, 1); + if (tp) + _xfs_trans_bjoin(tp, bp, 1); trace_xfs_trans_read_buf(bp->b_fspriv); - *bpp = bp; return 0; -shutdown_abort: - trace_xfs_trans_read_buf_shut(bp, _RET_IP_); - xfs_buf_relse(bp); - *bpp = NULL; - return -EIO; } /* -- 2.20.1