On Thu, Jan 16, 2020 at 10:50:20PM -0800, Christoph Hellwig wrote:
> > @@ -842,13 +845,15 @@ xfs_buf_read_map(
> > * drop the buffer
> > */
> > xfs_buf_relse(bp);
> > - return NULL;
> > + *bpp = NULL;
>
> We already set *bpp to NULL at the very beginning, so this line is
> redundant.
Will fix.
> > @@ -860,19 +865,18 @@ xfs_buf_read(
> > struct xfs_buf **bpp,
> > const struct xfs_buf_ops *ops)
> > {
> > int error;
> > DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> >
> > - *bpp = NULL;
> > - bp = xfs_buf_read_map(target, &map, 1, flags, ops);
> > - if (!bp)
> > - return -ENOMEM;
> > - error = bp->b_error;
> > + error = xfs_buf_read_map(target, &map, 1, flags, bpp, ops);
> > + if (error)
> > + return error;
> > + error = (*bpp)->b_error;
> > if (error) {
> > + xfs_buf_ioerror_alert(*bpp, __func__);
> > + xfs_buf_stale(*bpp);
> > + xfs_buf_relse(*bpp);
> > + *bpp = NULL;
> >
> > /* bad CRC means corrupted metadata */
> > if (error == -EFSBADCRC)
>
> I still think we have a problem here. We should not have to check
> ->b_error, and the xfs_buf_ioerror_alert should be either in the callers
> or in xfs_buf_read_map, as xfs_buf_read is just supposed to be a trivial
> wrapper for the single map case, not add functionality of its own.
Yeah. I've redone the patchset to keep xfs_buf_read() as a static
inline function, then refactored the ioerror/stale/relse bits as a
separate patch on the end.
--D
