Re: [PATCH 4/6] xfs: fix buffer state management in xrep_findroot_block

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Aug 14, 2018 at 08:56:38AM +1000, Dave Chinner wrote:
> On Sat, Aug 11, 2018 at 08:35:22AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > We don't quite handle buffer state properly in online repair's findroot
> > routine.  If the buffer is already in-core we don't want to trash its
> > b_ops and state, so first we should try _get_buf to grab the buffer.  If
> > the buffer is loaded, we only want to verify the structure of the buffer
> > since it could be dirty and the crc hasn't yet been recalculated.
> > 
> > Only if the buffer hasn't been read in should try _read_buf, and if we
> > were the ones who read the buffer then we must be careful to oneshot the
> > buffer so that a subsequent _read_buf won't find a buffer with no ops.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> I don't know the history of how this came about, but IMO this isn't
> a particularly nice solution.

Ugh, yes.

> > ---
> >  fs/xfs/scrub/repair.c |   67 +++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 54 insertions(+), 13 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> > index 97c3077fb005..fae50dced8bc 100644
> > --- a/fs/xfs/scrub/repair.c
> > +++ b/fs/xfs/scrub/repair.c
> > @@ -697,6 +697,7 @@ xrep_findroot_block(
> >  	struct xfs_mount		*mp = ri->sc->mp;
> >  	struct xfs_buf			*bp;
> >  	struct xfs_btree_block		*btblock;
> > +	xfs_failaddr_t			fa;
> >  	xfs_daddr_t			daddr;
> >  	int				block_level;
> >  	int				error;
> > @@ -718,28 +719,68 @@ xrep_findroot_block(
> >  			return error;
> >  	}
> >  
> > -	error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp, daddr,
> > -			mp->m_bsize, 0, &bp, NULL);
> > -	if (error)
> > -		return error;
> > -
> >  	/*
> > -	 * Does this look like a block matching our fs and higher than any
> > -	 * other block we've found so far?  If so, reattach buffer verifiers
> > -	 * so the AIL won't complain if the buffer is also dirty.
> > +	 * Try to grab the buffer, on the off chance it's already in memory.
> > +	 * If the buffer doesn't have the DONE flag set it hasn't been read
> > +	 * into memory yet.  Drop the buffer and read the buffer with NULL
> > +	 * b_ops.  (This could race with another read_buf.)  If we get the
> > +	 * buffer back with NULL b_ops then we know that there weren't any
> > +	 * other readers.  There's a risk we won't match the buffer with any
> > +	 * of the findroot prototypes, so we want to encourage the buffer layer
> > +	 * to drop the buffer as soon as possible.
> >  	 */
> > +	bp = xfs_trans_get_buf(ri->sc->tp, mp->m_ddev_targp, daddr,
> > +			mp->m_bsize, 0);
> > +	if (!bp)
> > +		return -ENOMEM;
> > +	if (!(bp->b_flags & XBF_DONE)) {
> > +		xfs_trans_brelse(ri->sc->tp, bp);
> > +
> > +		error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp,
> > +				daddr, mp->m_bsize, 0, &bp, NULL);
> > +		if (error)
> > +			return error;
> > +		if (!bp->b_ops)
> > +			xfs_buf_oneshot(bp);
> > +	}
> 
> Let's look a little closer. xfs_trans_read_buf() ends up in
> xfs_buf_read_map(), which does:
> 
> ....
>         bp = xfs_buf_get_map(target, map, nmaps, flags);
>         if (bp) {
>                 trace_xfs_buf_read(bp, flags, _RET_IP_);
> 
>                 if (!(bp->b_flags & XBF_DONE)) {
>                         XFS_STATS_INC(target->bt_mount, xb_get_read);
>                         bp->b_ops = ops;
>                         _xfs_buf_read(bp, flags);
> 		} else if (flags & XBF_ASYNC) {
> .....
> 
> But what you are doing in the code above is trying to do is
> determine if we needed to call _xfs_buf_read() on the buffer, and if
> we do we use a different verify procedure on it.
> 
> So isn't there a simpler way to do this? e.g. pass a flag down to
> xfs_buf_read_map() that says "use these ops for just this read".
> 
> 	error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp,
> 				daddr, mp->m_bsize, XBF_ONESHOT_OPS,
> 				&bp, NULL);
> 
> and change xfs_buf_read_map() to do:
> 
> 
> ....
>         bp = xfs_buf_get_map(target, map, nmaps, flags);
>         if (bp) {
>                 trace_xfs_buf_read(bp, flags, _RET_IP_);
> 
>                 if (!(bp->b_flags & XBF_DONE)) {
>                         XFS_STATS_INC(target->bt_mount, xb_get_read);
> 			if (flags & XBF_ONESHOT_OPS)
> 				orig_ops = bp->b_ops;
>                         bp->b_ops = ops;
>                         _xfs_buf_read(bp, flags);
> 			if (flags & XBF_ONESHOT_OPS)
> 				bp->b_ops = orig_ops;
> 		} else if (flags & XBF_ASYNC) {
> 			ASSERT(!(flags & XBF_ONESHOT_OPS));
> ....
> 
> Now you get back the buffer with it's original ops on it even if had
> to be read from disk and you used a different verifier. Hence you
> know how to treat it after this because the ops will be null if it
> was not in core and had to be read from disk.
> 
> That also means you could supply fab->buf_ops to the
> xfs_trans_read_buf() call, knowing that they'll be used on read and
> you'll get a null bp->b_ops back despite the buffer already having
> been fully verified. i.e. if it fails verification, you'll get an
> error rather than having to having to run the verification yourself.
> That means you only need to run the ->verify_struct() op if you get
> back a non-null bp->b_ops, which further simplifies the function...

Hmm, that seems like a much better solution.  I'll look into it.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com