On Thu, Aug 09, 2018 at 09:08:39AM -0400, Brian Foster wrote:
> On Tue, Aug 07, 2018 at 08:57:06PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > In xrep_findroot_block, if we find a candidate root block with sibling
> > pointers or sibling blocks on the same tree level, we should not return
> > that block as a tree root because root blocks cannot have siblings.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > fs/xfs/scrub/repair.c | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> >
> > diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> > index 85b048b341a0..6c199e2ebb81 100644
> > --- a/fs/xfs/scrub/repair.c
> > +++ b/fs/xfs/scrub/repair.c
> > @@ -727,6 +727,23 @@ xrep_findroot_block(
> > bp->b_ops->verify_read(bp);
> > if (bp->b_error)
> > goto out;
> > +
> > + /* Root blocks can't have siblings. */
> > + if (btblock->bb_u.s.bb_leftsib != cpu_to_be32(NULLAGBLOCK) ||
> > + btblock->bb_u.s.bb_rightsib != cpu_to_be32(NULLAGBLOCK))
> > + goto out;
> > +
> > + /*
> > + * If we find a second block at this level, ignore this level because
> > + * it can't possibly be a root level. Maybe we'll find a higher level,
> > + * or maybe the rmap information is garbage.
> > + */
> > + if (fab->root != NULLAGBLOCK &&
> > + fab->height == xfs_btree_get_level(btblock) + 1) {
> > + fab->root = NULLAGBLOCK;
> > + goto out;
> > + }
>
> Ok, but is this enough? Won't resetting fab->root like this mean that
> we'd just reassign it to the next block we find at this level? I'm
> wondering if we should maintain ->height independently and anticipate
> that (height == <valid> && root == NULLAGBLOCK) means we couldn't find a
> valid root. That may also allow for more efficient height filtering
> during the query.
Working through this again, I think we could just set fab->root = 0
when we encounter this situation. I think the two height checks could
be combined as well, so I'll retest and repost.
Roughly, I think this whole section could be restructured as:
/* Make sure we pass the verifiers. */
...
/* Root blocks can't have siblings. */
...
/* If we've recorded a root candidate... */
block_level = xfs_btree_get_level(btblock);
if (fab->root != NULLAGBLOCK) {
/*
* ...and this no-sibling root block candidate has the same
* level as the recorded candidate, there's no way we're going
* to accept any candidates at this tree level. Stash a root
* block of zero because the height is still valid, but no AG
* btree can root at agblock 0. Callers should verify the root
* agbno.
*/
if (block_level + 1 == fab->height) {
fab->root = 0;
goto out;
}
/*
* ...and this no-sibling root block is lower in the tree than
* the recorded root block candidate, just ignore it. There's
* still a strong chance that something is wrong with the btree
* itself, but that's not what we're fixing right now.
*/
if (block_level < fab->height)
goto out;
}
/* Record the candidate root block */
...
--D
>
> Brian
>
> > +
> > fab->root = agbno;
> > fab->height = xfs_btree_get_level(btblock) + 1;
> > *found_it = true;
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
