On Thu, Jan 16, 2020 at 08:21:48AM -0800, Christoph Hellwig wrote:
> > @@ -2960,6 +2960,10 @@ xfs_read_agf(
> > 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 && (flags & XBF_TRYLOCK)) {
>
> Given that EAGAIN is only returned in the XBF_TRYLOCK case the check
> for XBF_TRYLOCK should not be required.
Ok.
> > + *bpp = NULL;
> > + return 0;
>
> Also we should make sure in the lowest layers to never return a
> non-NULL bpp if returning an error to avoid this boilerplate code.
<nod>
At some point I was planning to audit the ALLOC_TRYLOCK cases to make
sure that they can handle an EAGAIN, so we can get rid of this chunk
entirely.
> > - if (!bp)
> > - return -ENOMEM;
> > + error = xfs_buf_read_map(target, &map, 1, flags, &bp, ops);
> > + if (error)
> > + return error;
> > error = bp->b_error;
> > if (error) {
> > xfs_buf_ioerror_alert(bp, __func__);
>
> The extra checking of bp->b_error shoudn't be required now. That almost
> means we might have to move the xfs_buf_ioerror_alert into
> xfs_buf_read_map.
>
> That also means xfs_buf_read can be turned into a simple inline
> function again.
Yeah, I'll add another patch to factor that out.
--D
