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 <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Dave Chinner <david@fromorbit.com>

1 files 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;
 }
 
 /*