On Mon, Oct 01, 2018 at 03:48:22PM -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 | 61 +++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 48 insertions(+), 13 deletions(-)
>
>
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index 9f08dd9bf1d5..6eb66b3543ff 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
...
> @@ -735,18 +736,52 @@ xrep_findroot_block(
> goto out;
> bp->b_ops = fab->buf_ops;
>
> - /* 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;
> +
> + /*
> + * This block passes the magic/uuid and verifier tests for this btree
> + * type. We don't need the caller to try the other tree types.
> + */
> + *done_with_block = true;
> +
> + block_level = xfs_btree_get_level(btblock);
> + if (block_level + 1 == fab->height) {
> + /*
> + * This block claims to be at the same level as the root we
> + * found previously. There can't be two candidate roots, so
> + * we'll throw away both of them and hope we later find a block
> + * even higher in the tree.
> + */
> + fab->root = NULLAGBLOCK;
> + goto out;
> + } else if (block_level < fab->height) {
> + /*
> + * This block is lower in the tree than the root we found
> + * previously, so just ignore it.
> + */
Nit: can we combine these two comments into one above the whole if/else
statement? It makes the code slightly more readable than multi-line
comments in each branch, IMO. E.g.:
/*
* Compare the current block level with the height of the current
* candidate root block. If the level matches the current candidate,
* we know the current candidate can't be a root so we must invalidate
* it. If the level is lower than the current candidate, just ignore
* this block.
*/
block_level = xfs_btree_get_level(btblock);
if (block_level + 1 == fab->height) {
fab->root = NULLAGBLOCK;
goto out;
} else if (block_level < fab->height) {
goto out;
}
With that:
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> + goto out;
> + }
> +
> + /*
> + * This is the highest block in the tree that we've found so far.
> + * Update the btree height to reflect what we've learned from this
> + * block.
> + */
> + fab->height = block_level + 1;
> +
> + /*
> + * If this block doesn't have sibling pointers, then it's the new root
> + * block candidate. Otherwise, the root will be found farther up the
> + * tree.
> + */
> + if (btblock->bb_u.s.bb_leftsib == cpu_to_be32(NULLAGBLOCK) &&
> + btblock->bb_u.s.bb_rightsib == cpu_to_be32(NULLAGBLOCK))
> + fab->root = agbno;
> + else
> + fab->root = NULLAGBLOCK;
>
> trace_xrep_findroot_block(mp, ri->sc->sa.agno, agbno,
> be32_to_cpu(btblock->bb_magic), fab->height - 1);
> @@ -768,7 +803,7 @@ xrep_findroot_rmap(
> struct xrep_findroot *ri = priv;
> struct xrep_find_ag_btree *fab;
> xfs_agblock_t b;
> - bool found_it;
> + bool done;
> int error = 0;
>
> /* Ignore anything that isn't AG metadata. */
> @@ -777,16 +812,16 @@ xrep_findroot_rmap(
>
> /* Otherwise scan each block + btree type. */
> for (b = 0; b < rec->rm_blockcount; b++) {
> - found_it = false;
> + done = false;
> for (fab = ri->btree_info; fab->buf_ops; fab++) {
> if (rec->rm_owner != fab->rmap_owner)
> continue;
> error = xrep_findroot_block(ri, fab,
> rec->rm_owner, rec->rm_startblock + b,
> - &found_it);
> + &done);
> if (error)
> return error;
> - if (found_it)
> + if (done)
> break;
> }
> }
>
