Re: [PATCH 06/25] xfs: scrub the shape of a metadata btree

[Dave Chinner <david@fromorbit.com>]

On Tue, Oct 03, 2017 at 01:41:27PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Create a function that can check the shape of a btree -- each block
> passes basic inspection and all the pointers look ok.  In the next patch
> we'll add the ability to check the actual keys and records stored within
> the btree.  Add some helper functions so that we report detailed scrub
> errors in a uniform manner in dmesg.  These are helper functions for
> subsequent patches.
.....
>  
> +/* Check a btree pointer.  Returns true if it's ok to use this pointer. */
> +static bool
> +xfs_scrub_btree_ptr_ok(
> +	struct xfs_scrub_btree		*bs,
> +	int				level,
> +	union xfs_btree_ptr		*ptr)
> +{
> +	struct xfs_btree_cur		*cur = bs->cur;
> +	xfs_daddr_t			daddr;
> +	xfs_daddr_t			eofs;
> +
> +	if (xfs_btree_ptr_is_null(cur, ptr)) {
> +		xfs_scrub_btree_set_corrupt(bs->sc, cur, level);
> +		return false;
> +	}
> +	if (cur->bc_flags & XFS_BTREE_LONG_PTRS) {
> +		daddr = XFS_FSB_TO_DADDR(cur->bc_mp, be64_to_cpu(ptr->l));
> +	} else {
> +		ASSERT(cur->bc_private.a.agno != NULLAGNUMBER);
> +		daddr = XFS_AGB_TO_DADDR(cur->bc_mp, cur->bc_private.a.agno,
> +				be32_to_cpu(ptr->s));
> +	}
> +	eofs = XFS_FSB_TO_BB(cur->bc_mp, cur->bc_mp->m_sb.sb_dblocks);
> +	if (daddr == 0 || daddr >= eofs) {
> +		xfs_scrub_btree_set_corrupt(bs->sc, cur, level);
> +		return false;
> +	}
> +
> +	return true;
> +}

There seems to be quite a bit of overlap here with
xfs_btree_check_ptr(). Indeed, for the short pointers the above code
fails to check it is within the bounds of the AG size. I'd suggest
both of these should use the same validity checking functions....

....
> +/*
> + * Grab and scrub a btree block given a btree pointer.  Returns block
> + * and buffer pointers (if applicable) if they're ok to use.
> + */
> +STATIC int
> +xfs_scrub_btree_get_block(
> +	struct xfs_scrub_btree		*bs,
> +	int				level,
> +	union xfs_btree_ptr		*pp,
> +	struct xfs_btree_block		**pblock,
> +	struct xfs_buf			**pbp)
> +{
> +	int				error;
> +
> +	error = xfs_btree_lookup_get_block(bs->cur, level, pp, pblock);
> +	if (!xfs_scrub_btree_op_ok(bs->sc, bs->cur, level, &error) || !pblock)
> +		return error;
> +
> +	xfs_btree_get_block(bs->cur, level, pbp);
> +	error = xfs_btree_check_block(bs->cur, *pblock, level, *pbp);
> +	if (!xfs_scrub_btree_op_ok(bs->sc, bs->cur, level, &error))
> +		return error;

xfs_btree_check_block() will throw error reports to dmesg for each
corrupt block that is found. Do we want scrub to do this, or should
it just report the corrupt block to userspace?

> +
> +	/*
> +	 * Check the block's siblings; this function absorbs error codes
> +	 * for us.
> +	 */
> +	return xfs_scrub_btree_block_check_siblings(bs, *pblock);
> +}
> +
>  /*
>   * Visit all nodes and leaves of a btree.  Check that all pointers and
>   * records are in order, that the keys reflect the records, and use a callback
> @@ -107,6 +253,93 @@ xfs_scrub_btree(
>  	struct xfs_owner_info		*oinfo,
>  	void				*private)
>  {
> -	xfs_scrub_btree_op_ok(sc, cur, 0, false);
> -	return -EOPNOTSUPP;
> +	struct xfs_scrub_btree		bs = {0};
> +	union xfs_btree_ptr		ptr;
> +	union xfs_btree_ptr		*pp;
> +	struct xfs_btree_block		*block;
> +	int				level;
> +	struct xfs_buf			*bp;
> +	int				i;
> +	int				error = 0;
> +
> +	/* Initialize scrub state */
> +	bs.cur = cur;
> +	bs.scrub_rec = scrub_fn;
> +	bs.oinfo = oinfo;
> +	bs.firstrec = true;
> +	bs.private = private;
> +	bs.sc = sc;
> +	for (i = 0; i < XFS_BTREE_MAXLEVELS; i++)
> +		bs.firstkey[i] = true;
> +	INIT_LIST_HEAD(&bs.to_check);
> +
> +	/* Don't try to check a tree with a height we can't handle. */
> +	if (cur->bc_nlevels > XFS_BTREE_MAXLEVELS) {
> +		xfs_scrub_btree_set_corrupt(sc, cur, 0);
> +		goto out;
> +	}
> +
> +	/* Make sure the root isn't in the superblock. */
> +	if (!(cur->bc_flags & XFS_BTREE_ROOT_IN_INODE)) {
> +		cur->bc_ops->init_ptr_from_cur(cur, &ptr);
> +		if (!xfs_scrub_btree_ptr_ok(&bs, cur->bc_nlevels - 1, &ptr))
> +			goto out;

Set corrupt if the init ptr is bad? And why do this check before
the code below that has another init_ptr_from_cur() call?

> +	}
> +
> +	/*
> +	 * Load the root of the btree.  The helper function absorbs
> +	 * error codes for us.
> +	 */
> +	level = cur->bc_nlevels - 1;
> +	cur->bc_ops->init_ptr_from_cur(cur, &ptr);

i.e.

	level = cur->bc_nlevels - 1;
	cur->bc_ops->init_ptr_from_cur(cur, &ptr);
	if (!(cur->bc_flags & XFS_BTREE_ROOT_IN_INODE) &&
	    !xfs_scrub_btree_ptr_ok(&bs, level, &ptr)) {
		xfs_scrub_btree_set_corrupt(sc, cur, 0);
		goto out;
	}

Which makes me ask the question - why aren't we validating the
initial pointer when the root is in an inode?

-Dave.
-- 
Dave Chinner
david@fromorbit.com