Re: [PATCH 03/46] xfs: refactor btree node scrubbing

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

On Thu, Nov 07, 2019 at 07:23:27PM +0100, Christoph Hellwig wrote:
> Break up xchk_da_btree_entry and handle looking up leaf node entries
> in the attr / dir callbacks, so that only the generic node handling
> is left in the common core code.  Note that the checks for the crc
> enabled blocks are removed, as the scrubbing code already remaps the
> magic numbers earlier.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/scrub/attr.c    | 11 +++++-----
>  fs/xfs/scrub/dabtree.c | 48 ++++++++++--------------------------------
>  fs/xfs/scrub/dabtree.h |  3 +--
>  fs/xfs/scrub/dir.c     | 12 ++++++++---
>  4 files changed, 27 insertions(+), 47 deletions(-)
> 
> diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
> index 0edc7f8eb96e..d9f0dd444b80 100644
> --- a/fs/xfs/scrub/attr.c
> +++ b/fs/xfs/scrub/attr.c
> @@ -398,15 +398,14 @@ xchk_xattr_block(
>  STATIC int
>  xchk_xattr_rec(
>  	struct xchk_da_btree		*ds,
> -	int				level,
> -	void				*rec)
> +	int				level)
>  {
>  	struct xfs_mount		*mp = ds->state->mp;
> -	struct xfs_attr_leaf_entry	*ent = rec;
> -	struct xfs_da_state_blk		*blk;
> +	struct xfs_da_state_blk		*blk = &ds->state->path.blk[level];
>  	struct xfs_attr_leaf_name_local	*lentry;
>  	struct xfs_attr_leaf_name_remote	*rentry;
>  	struct xfs_buf			*bp;
> +	struct xfs_attr_leaf_entry	*ent;
>  	xfs_dahash_t			calc_hash;
>  	xfs_dahash_t			hash;
>  	int				nameidx;
> @@ -414,7 +413,9 @@ xchk_xattr_rec(
>  	unsigned int			badflags;
>  	int				error;
>  
> -	blk = &ds->state->path.blk[level];
> +	ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
> +
> +	ent = xfs_attr3_leaf_entryp(blk->bp->b_addr) + blk->index;
>  
>  	/* Check the whole block, if necessary. */
>  	error = xchk_xattr_block(ds, level);
> diff --git a/fs/xfs/scrub/dabtree.c b/fs/xfs/scrub/dabtree.c
> index 77ff9f97bcda..d1248c223c7f 100644
> --- a/fs/xfs/scrub/dabtree.c
> +++ b/fs/xfs/scrub/dabtree.c
> @@ -77,40 +77,17 @@ xchk_da_set_corrupt(
>  			__return_address);
>  }
>  
> -/* Find an entry at a certain level in a da btree. */
> -STATIC void *
> -xchk_da_btree_entry(
> -	struct xchk_da_btree	*ds,
> -	int			level,
> -	int			rec)
> +static struct xfs_da_node_entry *
> +xchk_da_btree_node_entry(
> +	struct xchk_da_btree		*ds,
> +	int				level)
>  {
> -	char			*ents;
> -	struct xfs_da_state_blk	*blk;
> -	void			*baddr;
> +	struct xfs_da_state_blk		*blk = &ds->state->path.blk[level];
>  
> -	/* Dispatch the entry finding function. */
> -	blk = &ds->state->path.blk[level];
> -	baddr = blk->bp->b_addr;
> -	switch (blk->magic) {
> -	case XFS_ATTR_LEAF_MAGIC:
> -	case XFS_ATTR3_LEAF_MAGIC:
> -		ents = (char *)xfs_attr3_leaf_entryp(baddr);
> -		return ents + (rec * sizeof(struct xfs_attr_leaf_entry));
> -	case XFS_DIR2_LEAFN_MAGIC:
> -	case XFS_DIR3_LEAFN_MAGIC:
> -		ents = (char *)ds->dargs.dp->d_ops->leaf_ents_p(baddr);
> -		return ents + (rec * sizeof(struct xfs_dir2_leaf_entry));
> -	case XFS_DIR2_LEAF1_MAGIC:
> -	case XFS_DIR3_LEAF1_MAGIC:
> -		ents = (char *)ds->dargs.dp->d_ops->leaf_ents_p(baddr);
> -		return ents + (rec * sizeof(struct xfs_dir2_leaf_entry));
> -	case XFS_DA_NODE_MAGIC:
> -	case XFS_DA3_NODE_MAGIC:
> -		ents = (char *)ds->dargs.dp->d_ops->node_tree_p(baddr);
> -		return ents + (rec * sizeof(struct xfs_da_node_entry));
> -	}
> +	ASSERT(blk->magic == XFS_DA_NODE_MAGIC);
>  
> -	return NULL;
> +	return (void *)ds->dargs.dp->d_ops->node_tree_p(blk->bp->b_addr) +
> +		(blk->index * sizeof(struct xfs_da_node_entry));
>  }
>  
>  /* Scrub a da btree hash (key). */
> @@ -136,7 +113,7 @@ xchk_da_btree_hash(
>  
>  	/* Is this hash no larger than the parent hash? */
>  	blks = ds->state->path.blk;
> -	entry = xchk_da_btree_entry(ds, level - 1, blks[level - 1].index);

This eliminates the only user of blks, which means the variable can be
removed.  The rest looks fine though.

--D

> +	entry = xchk_da_btree_node_entry(ds, level - 1);
>  	parent_hash = be32_to_cpu(entry->hashval);
>  	if (parent_hash < hash)
>  		xchk_da_set_corrupt(ds, level);
> @@ -479,7 +456,6 @@ xchk_da_btree(
>  	struct xfs_mount		*mp = sc->mp;
>  	struct xfs_da_state_blk		*blks;
>  	struct xfs_da_node_entry	*key;
> -	void				*rec;
>  	xfs_dablk_t			blkno;
>  	int				level;
>  	int				error;
> @@ -538,9 +514,7 @@ xchk_da_btree(
>  			}
>  
>  			/* Dispatch record scrubbing. */
> -			rec = xchk_da_btree_entry(&ds, level,
> -					blks[level].index);
> -			error = scrub_fn(&ds, level, rec);
> +			error = scrub_fn(&ds, level);
>  			if (error)
>  				break;
>  			if (xchk_should_terminate(sc, &error) ||
> @@ -562,7 +536,7 @@ xchk_da_btree(
>  		}
>  
>  		/* Hashes in order for scrub? */
> -		key = xchk_da_btree_entry(&ds, level, blks[level].index);
> +		key = xchk_da_btree_node_entry(&ds, level);
>  		error = xchk_da_btree_hash(&ds, level, &key->hashval);
>  		if (error)
>  			goto out;
> diff --git a/fs/xfs/scrub/dabtree.h b/fs/xfs/scrub/dabtree.h
> index cb3f0003245b..1f3515c6d5a8 100644
> --- a/fs/xfs/scrub/dabtree.h
> +++ b/fs/xfs/scrub/dabtree.h
> @@ -28,8 +28,7 @@ struct xchk_da_btree {
>  	int			tree_level;
>  };
>  
> -typedef int (*xchk_da_btree_rec_fn)(struct xchk_da_btree *ds,
> -		int level, void *rec);
> +typedef int (*xchk_da_btree_rec_fn)(struct xchk_da_btree *ds, int level);
>  
>  /* Check for da btree operation errors. */
>  bool xchk_da_process_error(struct xchk_da_btree *ds, int level, int *error);
> diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
> index 1e2e11721eb9..97f274f7cd38 100644
> --- a/fs/xfs/scrub/dir.c
> +++ b/fs/xfs/scrub/dir.c
> @@ -179,14 +179,14 @@ xchk_dir_actor(
>  STATIC int
>  xchk_dir_rec(
>  	struct xchk_da_btree		*ds,
> -	int				level,
> -	void				*rec)
> +	int				level)
>  {
> +	struct xfs_da_state_blk		*blk = &ds->state->path.blk[level];
>  	struct xfs_mount		*mp = ds->state->mp;
> -	struct xfs_dir2_leaf_entry	*ent = rec;
>  	struct xfs_inode		*dp = ds->dargs.dp;
>  	struct xfs_dir2_data_entry	*dent;
>  	struct xfs_buf			*bp;
> +	struct xfs_dir2_leaf_entry	*ent;
>  	char				*p, *endp;
>  	xfs_ino_t			ino;
>  	xfs_dablk_t			rec_bno;
> @@ -198,6 +198,12 @@ xchk_dir_rec(
>  	unsigned int			tag;
>  	int				error;
>  
> +	ASSERT(blk->magic == XFS_DIR2_LEAF1_MAGIC ||
> +	       blk->magic == XFS_DIR2_LEAFN_MAGIC);
> +
> +	ent = (void *)ds->dargs.dp->d_ops->leaf_ents_p(blk->bp->b_addr) +
> +		(blk->index * sizeof(struct xfs_dir2_leaf_entry));
> +
>  	/* Check the hash of the entry. */
>  	error = xchk_da_btree_hash(ds, level, &ent->hashval);
>  	if (error)
> -- 
> 2.20.1
>