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>

diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index e2b2216..2f363cd 100644 (file)
--- 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;
 }
 
 /*