Re: [PATCH 05/22] xfs: recover AG btree roots from rmap data

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

On Thu, May 17, 2018 at 08:36:22AM +1000, Dave Chinner wrote:
> On Wed, May 16, 2018 at 11:37:29AM -0700, Darrick J. Wong wrote:
> > On Wed, May 16, 2018 at 06:51:52PM +1000, Dave Chinner wrote:
> > > On Tue, May 15, 2018 at 03:34:10PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > Add a helper function to help us recover btree roots from the rmap data.
> > > > Callers pass in a list of rmap owner codes, buffer ops, and magic
> > > > numbers.  We iterate the rmap records looking for owner matches, and
> > > > then read the matching blocks to see if the magic number & uuid match.
> > > > If so, we then read-verify the block, and if that passes then we retain
> > > > a pointer to the block with the highest level, assuming that by the end
> > > > of the call we will have found the root.  This will be used to reset the
> > > > AGF/AGI btree root fields during their rebuild procedures.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> .....
> > > > +	/* Ignore this block if it's lower in the tree than we've seen. */
> > > > +	if (fab->root != NULLAGBLOCK &&
> > > > +	    xfs_btree_get_level(btblock) < fab->height)
> > > > +		goto out;
> > > > +
> > > > +	/* Make sure we pass the verifiers. */
> > > > +	bp->b_ops->verify_read(bp);
> > > > +	if (bp->b_error)
> > > > +		goto out;
> > > > +	fab->root = agbno;
> > > > +	fab->height = xfs_btree_get_level(btblock) + 1;
> > > > +	*found_it = true;
> > > > +
> > > > +	trace_xfs_repair_findroot_block(mp, ri->sc->sa.agno, agbno,
> > > > +			be32_to_cpu(btblock->bb_magic), fab->height - 1);
> > > > +out:
> > > > +	xfs_trans_brelse(ri->sc->tp, bp);
> > > 
> > > So we release the buffer once we've found it, which also unlocks it.
> > > That means when we come back to it later, it may have been accessed
> > > and changed by something else and no longer be the block we are
> > > looking for. How do you protect against this sort of race given we
> > > are unlocking the buffer? Perhaps it should be held on the fab
> > > structure, and released when a better candidate is found?
> > 
> > The two callers of this function are the AGF and AGI repair functions.
> > AGF repair holds the locked AGF buffer, and AGI repair holds the locked
> > AGF & AGI buffers, which should be enough to prevent anyone else from
> > accessing the AG btrees.  They keep the all the AG header buffers locked
> > until they're completely finished with rebuilding the headers (i.e.
> > xfs_scrub_teardown) and it's safe for the shape to change.
> > 
> > How about I add to the comment for this function:
> > 
> > /*
> >  * The caller must lock the applicable per-AG header buffers (AGF, AGI)
> >  * to prevent other threads from changing the shape of the btrees that
> >  * we are looking for.  It must maintain those locks until it's safe for
> >  * other threads to change the btrees' shapes.
> >  */
> 
> That's helpful. :) Can you sprinkle some checks like
> ASSERT(xfs_buf_islocked(agbp)) to remind readers of the
> leaf/callback functions that they expect the AGF/AGI to be locked on
> entry?

Ok, will do.

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html