Message ID | 157956102137.1166689.2159908930036102057.stgit@magnolia (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xfs: make buffer functions return error codes | expand |
On Mon, Jan 20, 2020 at 02:57:01PM -0800, Darrick J. Wong wrote: > @@ -831,7 +833,8 @@ xfs_buf_read_map( > XFS_STATS_INC(target->bt_mount, xb_get_read); > bp->b_ops = ops; > _xfs_buf_read(bp, flags); > - return bp; > + *bpp = bp; > + return 0; _xfs_buf_read can return an error, and we are losing that here. So we should return the value from _xfs_buf_read, an ensure *bpp is NULL if it returns an error. That also means all the b_error check in the callers of xfs_buf_read_map and xfs_buf_read (and with that the biggest wart in the buffer cache API) can go away.
On Tue, Jan 21, 2020 at 02:52:28PM -0800, Christoph Hellwig wrote: > On Mon, Jan 20, 2020 at 02:57:01PM -0800, Darrick J. Wong wrote: > > @@ -831,7 +833,8 @@ xfs_buf_read_map( > > XFS_STATS_INC(target->bt_mount, xb_get_read); > > bp->b_ops = ops; > > _xfs_buf_read(bp, flags); > > - return bp; > > + *bpp = bp; > > + return 0; > > _xfs_buf_read can return an error, and we are losing that here. So > we should return the value from _xfs_buf_read, an ensure *bpp is NULL > if it returns an error. That also means all the b_error check in the > callers of xfs_buf_read_map and xfs_buf_read (and with that the biggest > wart in the buffer cache API) can go away. I rearrange responsibility for dealing with buffer error handling in the patch "xfs: move buffer read io error logging to xfs_buf_read_map" later in this series. Was that not what you were expecting? Though looking at that patch I guess we could set @error directly to the return values of xfs_buf_reverify/_xfs_buf_read. --D
On Tue, Jan 21, 2020 at 04:20:46PM -0800, Darrick J. Wong wrote: > I rearrange responsibility for dealing with buffer error handling in the > patch "xfs: move buffer read io error logging to xfs_buf_read_map" later > in this series. Was that not what you were expecting? I defintively don't expect a patch talking about logging to change error handling behavior. And yes, I also expect that if we change a function to return an error code that is actually uses that to return errors. > Though looking at that patch I guess we could set @error directly to the > return values of xfs_buf_reverify/_xfs_buf_read. Yes. Code outside xfs_buf.c and maybe xfs_buf_item.c really ever access b_error.
On Wed, Jan 22, 2020 at 02:12:07PM -0800, Christoph Hellwig wrote: > On Tue, Jan 21, 2020 at 04:20:46PM -0800, Darrick J. Wong wrote: > > I rearrange responsibility for dealing with buffer error handling in the > > patch "xfs: move buffer read io error logging to xfs_buf_read_map" later > > in this series. Was that not what you were expecting? > > I defintively don't expect a patch talking about logging to change error > handling behavior. And yes, I also expect that if we change a function > to return an error code that is actually uses that to return errors. Fair enough, I'll fold that one in here. > > Though looking at that patch I guess we could set @error directly to the > > return values of xfs_buf_reverify/_xfs_buf_read. > > Yes. Code outside xfs_buf.c and maybe xfs_buf_item.c really ever access > b_error. <nod> --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/xfs_buf.c b/fs/xfs/xfs_buf.c index 09b8f00d4182..460821844ee5 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -809,21 +809,23 @@ 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; 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_); @@ -831,7 +833,8 @@ xfs_buf_read_map( XFS_STATS_INC(target->bt_mount, xb_get_read); bp->b_ops = ops; _xfs_buf_read(bp, flags); - return bp; + *bpp = bp; + return 0; } xfs_buf_reverify(bp, ops); @@ -842,13 +845,14 @@ xfs_buf_read_map( * drop the buffer */ xfs_buf_relse(bp); - return NULL; + return 0; } /* 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 +866,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_trans_buf.c b/fs/xfs/xfs_trans_buf.c index b5b3a78ef31c..a2af6dec310d 100644 --- a/fs/xfs/xfs_trans_buf.c +++ b/fs/xfs/xfs_trans_buf.c @@ -298,12 +298,9 @@ 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; - } + error = xfs_buf_read_map(target, map, nmaps, flags, &bp, ops); + if (error) + return error; /* * If we've had a read error, then the contents of the buffer are