On Wed, Feb 06, 2019 at 10:08:35AM -0800, Darrick J. Wong wrote:
> On Mon, Feb 04, 2019 at 09:52:23AM -0500, Brian Foster wrote:
> > From: "Darrick J. Wong" <darrick.wong@xxxxxxxxxx>
> >
> > In xrep_findroot_block, we work out the btree type and correctness of a
> > given block by calling different btree verifiers on root block
> > candidates. However, we leave the NULL b_ops while ->verify_read
> > validates the block, which means that if the verifier calls
> > xfs_buf_verifier_error it'll crash on the null b_ops. Fix it to set
> > b_ops before calling the verifier and unsetting it if the verifier
> > fails.
> >
> > Furthermore, improve the documentation around xfs_buf_ensure_ops, which
> > is the function that is responsible for cleaning up the b_ops state of
> > buffers that go through xrep_findroot_block but don't match anything.
> >
> > [bfoster: Renamed xfs_buf_ensure_ops() and modified comment.]
>
> Heh, I already queued up the original patch for 5.0 so that we don't
> oops the kernel. :/
>
Yeah, I noticed.. not really a big deal.
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> > ---
> > fs/xfs/scrub/repair.c | 11 ++++++++---
> > fs/xfs/xfs_buf.c | 22 ++++++++++++++++------
> > fs/xfs/xfs_buf.h | 2 +-
> > fs/xfs/xfs_trans_buf.c | 2 +-
> > 4 files changed, 26 insertions(+), 11 deletions(-)
> >
...
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index eedc5e0156ff..222b5260ed35 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -776,13 +776,23 @@ _xfs_buf_read(
> > }
> >
> > /*
> > - * If the caller passed in an ops structure and the buffer doesn't have ops
> > - * assigned, set the ops and use them to verify the contents. If the contents
> > - * cannot be verified, we'll clear XBF_DONE. We assume the buffer has no
> > - * recorded errors and is already in XBF_DONE state.
> > + * Reverify a buffer found in cache without an attached ->b_ops.
> > + *
> > + * If the caller passed an ops structure and the buffer doesn't have ops
> > + * assigned, set the ops and use it to verify the contents. If verification
> > + * fails, clear XBF_DONE. We assume the buffer has no recorded errors and is
> > + * already in XBF_DONE state on entry.
> > + *
> > + * Under normal operations, every in-core buffer is verified on read I/O
> > + * completion. There are two scenarios that can lead to in-core buffers without
> > + * an assigned ->b_ops. The first is during log recovery of buffers on a V4
> > + * filesystem, though these buffers are purged at the end of recovery. The
> > + * other is online repair, which intentionally reads with a NULL buffer ops to
> > + * run several verifiers across an in-core buffer in order to establish buffer
> > + * type.
>
> I would like to add a sentence here explicitly saying that if repair
> can't establish the buffer type from the verifiers then the buffer is
> left in memory with null b_ops.
>
Ok.
> > */
> > int
> > -xfs_buf_ensure_ops(
> > +xfs_buf_reverify(
>
> I like the name and the comment better than my original version, so I'll
> rework this patch as a separate "rename and unfrobulate documentation"
> thing and send it out.
>
Sounds good to me. Thanks.
Brian
> --D
>
> > struct xfs_buf *bp,
> > const struct xfs_buf_ops *ops)
> > {
> > @@ -824,7 +834,7 @@ xfs_buf_read_map(
> > return bp;
> > }
> >
> > - xfs_buf_ensure_ops(bp, ops);
> > + xfs_buf_reverify(bp, ops);
> >
> > if (flags & XBF_ASYNC) {
> > /*
> > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> > index b9f5511ea998..1c436a85b71d 100644
> > --- a/fs/xfs/xfs_buf.h
> > +++ b/fs/xfs/xfs_buf.h
> > @@ -385,6 +385,6 @@ extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int);
> > #define xfs_getsize_buftarg(buftarg) block_size((buftarg)->bt_bdev)
> > #define xfs_readonly_buftarg(buftarg) bdev_read_only((buftarg)->bt_bdev)
> >
> > -int xfs_buf_ensure_ops(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
> > +int xfs_buf_reverify(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
> >
> > #endif /* __XFS_BUF_H__ */
> > diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> > index 629f1479c9d2..7d65ebf1e847 100644
> > --- a/fs/xfs/xfs_trans_buf.c
> > +++ b/fs/xfs/xfs_trans_buf.c
> > @@ -277,7 +277,7 @@ xfs_trans_read_buf_map(
> > * release this buffer when it kills the tranaction.
> > */
> > ASSERT(bp->b_ops != NULL);
> > - error = xfs_buf_ensure_ops(bp, ops);
> > + error = xfs_buf_reverify(bp, ops);
> > if (error) {
> > xfs_buf_ioerror_alert(bp, __func__);
> >
> > --
> > 2.17.2
> >
