[02/34] xfs: refactor btree node scrubbing

Message ID	20191101220719.29100-3-hch@lst.de (mailing list archive)
State	Superseded, archived
Headers	show Return-Path: <SRS0=5kOL=YZ=vger.kernel.org=linux-xfs-owner@kernel.org> From: Christoph Hellwig <hch@lst.de> To: linux-xfs@vger.kernel.org Subject: [PATCH 02/34] xfs: refactor btree node scrubbing Date: Fri, 1 Nov 2019 15:06:47 -0700 Message-Id: <20191101220719.29100-3-hch@lst.de> In-Reply-To: <20191101220719.29100-1-hch@lst.de> References: <20191101220719.29100-1-hch@lst.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-xfs-owner@vger.kernel.org Precedence: bulk
Series	[01/34] xfs: move incore structures out of xfs_da_format.h \| expand [01/34] xfs: move incore structures out of xfs_da_format.h [02/34] xfs: refactor btree node scrubbing [03/34] xfs: devirtualize ->node_hdr_from_disk [04/34] xfs: devirtualize ->node_hdr_to_disk [05/34] xfs: add a btree entries pointer to struct xfs_da3_icnode_hdr [06/34] xfs: move the node header size to struct xfs_da_geometry [07/34] xfs: devirtualize ->leaf_hdr_from_disk [08/34] xfs: devirtualize ->leaf_hdr_to_disk [09/34] xfs: add a entries pointer to struct xfs_dir3_icleaf_hdr [10/34] xfs: move the dir2 leaf header size to struct xfs_da_geometry [11/34] xfs: move the max dir2 leaf entries count to struct xfs_da_geometry [12/34] xfs: devirtualize ->free_hdr_from_disk [13/34] xfs: devirtualize ->free_hdr_to_disk [14/34] xfs: make the xfs_dir3_icfree_hdr available to xfs_dir2_node_addname_int [15/34] xfs: add a bests pointer to struct xfs_dir3_icfree_hdr [16/34] xfs: move the dir2 free header size to struct xfs_da_geometry [17/34] xfs: move the max dir2 free bests count to struct xfs_da_geometry [18/34] xfs: devirtualize ->db_to_fdb and ->db_to_fdindex [19/34] xfs: devirtualize ->sf_get_parent_ino and ->sf_put_parent_ino [20/34] xfs: devirtualize ->sf_entsize and ->sf_nextentry [21/34] xfs: devirtualize ->sf_get_ino and ->sf_put_ino [22/34] xfs: devirtualize ->sf_get_ftype and ->sf_put_ftype [23/34] xfs: remove the ->data_dot_entry_p and ->data_dotdot_entry_p methods [24/34] xfs: remove the unused ->data_first_entry_p method [25/34] xfs: remove the ->data_entry_entry_p method [26/34] xfs: devirtualize ->data_entsize [27/34] xfs: devirtualize ->data_entry_tag_p [28/34] xfs: move the dir2 data block fixed offsets to struct xfs_da_geometry [29/34] xfs: cleanup xfs_dir2_data_entsize [30/34] xfs: devirtualize ->data_bestfree_p [31/34] xfs: devirtualize ->data_get_ftype and ->data_put_ftype [32/34] xfs: remove the now unused dir ops infrastructure [33/34] xfs: merge xfs_dir2_data_freescan and xfs_dir2_data_freescan_int [34/34] xfs: always pass a valid hdr to xfs_dir3_leaf_check_int

Message ID

20191101220719.29100-3-hch@lst.de (mailing list archive)

State

Superseded, archived

Headers

From: Christoph Hellwig <hch@lst.de>
To: linux-xfs@vger.kernel.org
Subject: [PATCH 02/34] xfs: refactor btree node scrubbing
Date: Fri,  1 Nov 2019 15:06:47 -0700
Message-Id: <20191101220719.29100-3-hch@lst.de>
In-Reply-To: <20191101220719.29100-1-hch@lst.de>
References: <20191101220719.29100-1-hch@lst.de>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-xfs-owner@vger.kernel.org
Precedence: bulk

Series

[01/34] xfs: move incore structures out of xfs_da_format.h | expand

Commit Message

Christoph Hellwig Nov. 1, 2019, 10:06 p.m. UTC

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    | 12 ++++++-----
 fs/xfs/scrub/dabtree.c | 48 ++++++++++--------------------------------
 fs/xfs/scrub/dabtree.h |  3 +--
 fs/xfs/scrub/dir.c     | 12 ++++++++---
 4 files changed, 28 insertions(+), 47 deletions(-)

Comments

Darrick J. Wong Nov. 4, 2019, 6:36 p.m. UTC | #1

On Fri, Nov 01, 2019 at 03:06:47PM -0700, 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    | 12 ++++++-----
>  fs/xfs/scrub/dabtree.c | 48 ++++++++++--------------------------------
>  fs/xfs/scrub/dabtree.h |  3 +--
>  fs/xfs/scrub/dir.c     | 12 ++++++++---
>  4 files changed, 28 insertions(+), 47 deletions(-)
> 
> diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
> index 0edc7f8eb96e..035d5734e0af 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,10 @@ xchk_xattr_rec(
>  	unsigned int			badflags;
>  	int				error;
>  
> -	blk = &ds->state->path.blk[level];
> +	ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
> +
> +	ent = (void *)xfs_attr3_leaf_entryp(blk->bp->b_addr) +
> +		(blk->index * sizeof(struct xfs_attr_leaf_entry));

Seeing as this function returns a pointer to struct xfs_attr_leaf_entry,
why not clean this up to:

ent = xfs_attr3_leaf_entryp(...)[blk->index]; ?

Though looking at that, I wonder if a better option would be to
incorporate the index as an argument to xfs_attr3_leaf_entryp?

(And yes, that's material for a separate patch...)

>  
>  	/* 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;

gcc complained that blks is no longer necessary.

--D

> -	entry = xchk_da_btree_entry(ds, level - 1, blks[level - 1].index);
> +	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
>

Christoph Hellwig Nov. 5, 2019, 1:35 a.m. UTC | #2

On Mon, Nov 04, 2019 at 10:36:18AM -0800, Darrick J. Wong wrote:
> Seeing as this function returns a pointer to struct xfs_attr_leaf_entry,
> why not clean this up to:
> 
> ent = xfs_attr3_leaf_entryp(...)[blk->index]; ?

Well, it should really be xfs_attr3_leaf_entryp + blk->index.  But
otherwise agreed and fixed up for the next version.

diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
index 0edc7f8eb96e..035d5734e0af 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,10 @@  xchk_xattr_rec(
 	unsigned int			badflags;
 	int				error;
 
-	blk = &ds->state->path.blk[level];
+	ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
+
+	ent = (void *)xfs_attr3_leaf_entryp(blk->bp->b_addr) +
+		(blk->index * sizeof(struct xfs_attr_leaf_entry));
 
 	/* 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);
+	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)

[02/34] xfs: refactor btree node scrubbing

Commit Message

Comments

Patch