Message ID | 157976534245.2388944.13378396804109422541.stgit@magnolia (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xfs: make buffer functions return error codes | expand |
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;
On Thu, Jan 23, 2020 at 02:24:41PM -0800, Christoph Hellwig wrote: > 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? Actually ... it really needs to be in the next patch because it's the xfs_buf_get_map transition that makes it so that xfs_trans_read_buf can return EAGAIN. > > + /* 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. Huh? This patch /does/ remove the EFSBADCRC->EFSCORRUPTED code in the xfs_buf_read callers... <confused> > > + 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; Yes, I think so. --D
On Wed, Jan 22, 2020 at 11:42:22PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Convert xfs_buf_read_map() to return numeric error codes like most > everywhere else in xfs. This involves moving the open-coded logic that > reports metadata IO read / corruption errors and stales the buffer into > xfs_buf_read_map so that the logic is all in one place. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> ..... > diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c > index b5b3a78ef31c..56e7f8126cd7 100644 > --- a/fs/xfs/xfs_trans_buf.c > +++ b/fs/xfs/xfs_trans_buf.c > @@ -298,36 +298,17 @@ xfs_trans_read_buf_map( > return 0; > } > > - bp = xfs_buf_read_map(target, map, nmaps, flags, ops); > - 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); > - > + 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); > - xfs_buf_relse(bp); > - > - /* bad CRC means corrupted metadata */ > - if (error == -EFSBADCRC) > - error = -EFSCORRUPTED; > + xfs_force_shutdown(tp->t_mountp, > + SHUTDOWN_META_IO_ERROR); > + /* fall through */ > + default: > return error; > } Same question as Christoph - we're only trying to avoid ENOMEM and EAGAIN errors from shutting down the filesystem here, right? Every other type of IO error that could end up on bp->b_error would result in a shutdown, so perhaps this should be the other way around: switch (error) { case 0: break; default: /* shutdown stuff */ /* fall through */ case -ENOMEM: case -EAGAIN: return error; } Cheers, Dave.
On Fri, Jan 24, 2020 at 12:31:52PM +1100, Dave Chinner wrote: > On Wed, Jan 22, 2020 at 11:42:22PM -0800, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Convert xfs_buf_read_map() to return numeric error codes like most > > everywhere else in xfs. This involves moving the open-coded logic that > > reports metadata IO read / corruption errors and stales the buffer into > > xfs_buf_read_map so that the logic is all in one place. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > ..... > > > diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c > > index b5b3a78ef31c..56e7f8126cd7 100644 > > --- a/fs/xfs/xfs_trans_buf.c > > +++ b/fs/xfs/xfs_trans_buf.c > > @@ -298,36 +298,17 @@ xfs_trans_read_buf_map( > > return 0; > > } > > > > - bp = xfs_buf_read_map(target, map, nmaps, flags, ops); > > - 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); > > - > > + 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); > > - xfs_buf_relse(bp); > > - > > - /* bad CRC means corrupted metadata */ > > - if (error == -EFSBADCRC) > > - error = -EFSCORRUPTED; > > + xfs_force_shutdown(tp->t_mountp, > > + SHUTDOWN_META_IO_ERROR); > > + /* fall through */ > > + default: > > return error; > > } > > Same question as Christoph - we're only trying to avoid ENOMEM and > EAGAIN errors from shutting down the filesystem here, right? > Every other type of IO error that could end up on bp->b_error would > result in a shutdown, so perhaps this should be the other way > around: > > switch (error) { > case 0: > break; > default: > /* shutdown stuff */ > /* fall through */ > case -ENOMEM: > case -EAGAIN: > return error; > } I agree that ENOMEM ought to be on the list of things that don't immediately cause a shutdown if the transaction is dirty. --D > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
On Thu, Jan 23, 2020 at 04:23:21PM -0800, Darrick J. Wong wrote: > On Thu, Jan 23, 2020 at 02:24:41PM -0800, Christoph Hellwig wrote: > > 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? > > Actually ... it really needs to be in the next patch because it's the > xfs_buf_get_map transition that makes it so that xfs_trans_read_buf can > return EAGAIN. Now that I've reshuffled the whole patchset I realize that it more or less has to be this way because this particular change insulates the callers of xfs_read_agf() from needing to learn about EAGAIN right now. I /could/ change all of those callers in this patch instead of handling it separately in "xfs: make xfs_*read_agf return EAGAIN to ALLOC_FLAG_TRYLOCK callers", but now the patch would be changing the behavior of three separate API calls, and I'm trying to avoid monsters like that. (Anyway, onward to v5...) > > > + /* 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. > > Huh? This patch /does/ remove the EFSBADCRC->EFSCORRUPTED code in the > xfs_buf_read callers... <confused> The reshuffle makes adding this bit unnecessary since I converted xfs_buf_read_map earlier in the sequence. > > > + 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; > > Yes, I think so. > > --D
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c 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; ASSERT(!(*bpp)->b_error); xfs_buf_set_ref(*bpp, XFS_AGF_REF); diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c index 46f055804433..8b7f74b3bea2 100644 --- a/fs/xfs/libxfs/xfs_attr_remote.c +++ b/fs/xfs/libxfs/xfs_attr_remote.c @@ -422,16 +422,6 @@ xfs_attr_rmtval_get( 0, &bp, &xfs_attr3_rmt_buf_ops); if (error) return error; - error = bp->b_error; - if (error) { - xfs_buf_ioerror_alert(bp, __func__); - xfs_buf_relse(bp); - - /* bad CRC means corrupted metadata */ - if (error == -EFSBADCRC) - error = -EFSCORRUPTED; - return error; - } error = xfs_attr_rmtval_copyout(mp, bp, args->dp->i_ino, &offset, &valuelen, diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 09b8f00d4182..8a1c5c705b29 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -809,46 +809,76 @@ xfs_buf_reverify( return bp->b_error; } -xfs_buf_t * +int xfs_buf_read_map( struct xfs_buftarg *target, struct xfs_buf_map *map, int nmaps, xfs_buf_flags_t flags, + struct xfs_buf **bpp, const struct xfs_buf_ops *ops) { struct xfs_buf *bp; + int error; flags |= XBF_READ; + *bpp = NULL; bp = xfs_buf_get_map(target, map, nmaps, flags); if (!bp) - return NULL; + return -ENOMEM; trace_xfs_buf_read(bp, flags, _RET_IP_); if (!(bp->b_flags & XBF_DONE)) { + /* Initiate the buffer read and wait. */ XFS_STATS_INC(target->bt_mount, xb_get_read); bp->b_ops = ops; - _xfs_buf_read(bp, flags); - return bp; + error = _xfs_buf_read(bp, flags); + + /* Readahead iodone already dropped the buffer, so exit. */ + if (flags & XBF_ASYNC) + return 0; + } else { + /* Buffer already read; all we need to do is check it. */ + error = xfs_buf_reverify(bp, ops); + + /* Readahead already finished; drop the buffer and exit. */ + if (flags & XBF_ASYNC) { + xfs_buf_relse(bp); + return 0; + } + + /* We do not want read in the flags */ + bp->b_flags &= ~XBF_READ; + ASSERT(bp->b_ops != NULL || ops == NULL); } - xfs_buf_reverify(bp, ops); + /* + * 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 (error) { + if (!XFS_FORCED_SHUTDOWN(target->bt_mount)) + xfs_buf_ioerror_alert(bp, __func__); - if (flags & XBF_ASYNC) { - /* - * Read ahead call which is already satisfied, - * drop the buffer - */ + bp->b_flags &= ~XBF_DONE; + xfs_buf_stale(bp); xfs_buf_relse(bp); - return NULL; + + /* bad CRC means corrupted metadata */ + if (error == -EFSBADCRC) + error = -EFSCORRUPTED; + return error; } - /* We do not want read in the flags */ - bp->b_flags &= ~XBF_READ; - ASSERT(bp->b_ops != NULL || ops == NULL); - return bp; + *bpp = bp; + return 0; } /* @@ -862,11 +892,13 @@ xfs_buf_readahead_map( int nmaps, const struct xfs_buf_ops *ops) { + struct xfs_buf *bp; + if (bdi_read_congested(target->bt_bdev->bd_bdi)) return; xfs_buf_read_map(target, map, nmaps, - XBF_TRYLOCK|XBF_ASYNC|XBF_READ_AHEAD, ops); + XBF_TRYLOCK | XBF_ASYNC | XBF_READ_AHEAD, &bp, ops); } /* diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h index eace3e285157..14209db07684 100644 --- a/fs/xfs/xfs_buf.h +++ b/fs/xfs/xfs_buf.h @@ -195,13 +195,11 @@ struct xfs_buf *xfs_buf_incore(struct xfs_buftarg *target, struct xfs_buf *xfs_buf_get_map(struct xfs_buftarg *target, struct xfs_buf_map *map, int nmaps, xfs_buf_flags_t flags); -struct xfs_buf *xfs_buf_read_map(struct xfs_buftarg *target, - struct xfs_buf_map *map, int nmaps, - xfs_buf_flags_t flags, - const struct xfs_buf_ops *ops); -void xfs_buf_readahead_map(struct xfs_buftarg *target, - struct xfs_buf_map *map, int nmaps, - const struct xfs_buf_ops *ops); +int xfs_buf_read_map(struct xfs_buftarg *target, struct xfs_buf_map *map, + int nmaps, xfs_buf_flags_t flags, struct xfs_buf **bpp, + const struct xfs_buf_ops *ops); +void xfs_buf_readahead_map(struct xfs_buftarg *target, struct xfs_buf_map *map, + int nmaps, const struct xfs_buf_ops *ops); static inline int xfs_buf_get( @@ -231,16 +229,9 @@ xfs_buf_read( struct xfs_buf **bpp, const struct xfs_buf_ops *ops) { - struct xfs_buf *bp; DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); - *bpp = NULL; - bp = xfs_buf_read_map(target, &map, 1, flags, ops); - if (!bp) - return -ENOMEM; - - *bpp = bp; - return 0; + return xfs_buf_read_map(target, &map, 1, flags, bpp, ops); } static inline void diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index b29806846916..ac79537d3275 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -2749,11 +2749,6 @@ xlog_recover_buffer_pass2( buf_flags, &bp, NULL); if (error) return error; - error = bp->b_error; - if (error) { - xfs_buf_ioerror_alert(bp, "xlog_recover_do..(read#1)"); - goto out_release; - } /* * Recover the buffer only if we get an LSN from it and it's less than @@ -2954,11 +2949,6 @@ xlog_recover_inode_pass2( 0, &bp, &xfs_inode_buf_ops); if (error) goto error; - error = bp->b_error; - if (error) { - xfs_buf_ioerror_alert(bp, "xlog_recover_do..(read#2)"); - goto out_release; - } ASSERT(in_f->ilf_fields & XFS_ILOG_CORE); dip = xfs_buf_offset(bp, in_f->ilf_boffset); diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c index 4f10d764163b..b94d7b9b55d0 100644 --- a/fs/xfs/xfs_symlink.c +++ b/fs/xfs/xfs_symlink.c @@ -57,16 +57,6 @@ xfs_readlink_bmap_ilocked( &bp, &xfs_symlink_buf_ops); if (error) return error; - error = bp->b_error; - if (error) { - xfs_buf_ioerror_alert(bp, __func__); - xfs_buf_relse(bp); - - /* bad CRC means corrupted metadata */ - if (error == -EFSBADCRC) - error = -EFSCORRUPTED; - goto out; - } byte_cnt = XFS_SYMLINK_BUF_SPACE(mp, byte_cnt); if (pathlen < byte_cnt) byte_cnt = pathlen; diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c index b5b3a78ef31c..56e7f8126cd7 100644 --- a/fs/xfs/xfs_trans_buf.c +++ b/fs/xfs/xfs_trans_buf.c @@ -298,36 +298,17 @@ xfs_trans_read_buf_map( return 0; } - bp = xfs_buf_read_map(target, map, nmaps, flags, ops); - 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); - + 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); - xfs_buf_relse(bp); - - /* bad CRC means corrupted metadata */ - if (error == -EFSBADCRC) - error = -EFSCORRUPTED; + xfs_force_shutdown(tp->t_mountp, + SHUTDOWN_META_IO_ERROR); + /* fall through */ + default: return error; }