[2/8] xfs: calculate inode cluster geometry once during inobt scrub
diff mbox series

Message ID 154147729891.32496.12191035015614129432.stgit@magnolia
State New
Headers show
Series
  • xfs-5.0: inode btree scrub fixes
Related show

Commit Message

Darrick J. Wong Nov. 6, 2018, 4:08 a.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

Hoist all the inode cluster geometry information into struct
xchk_iallocbt instead of recomputing them over and over.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/ialloc.c |   42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

Comments

Dave Chinner Nov. 6, 2018, 9:32 p.m. UTC | #1
On Mon, Nov 05, 2018 at 08:08:18PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Hoist all the inode cluster geometry information into struct
> xchk_iallocbt instead of recomputing them over and over.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/ialloc.c |   42 ++++++++++++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 14 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/ialloc.c b/fs/xfs/scrub/ialloc.c
> index 76ae8338a42e..3c12a0fe3b38 100644
> --- a/fs/xfs/scrub/ialloc.c
> +++ b/fs/xfs/scrub/ialloc.c
> @@ -45,8 +45,18 @@ xchk_setup_ag_iallocbt(
>  /* Inode btree scrubber. */
>  
>  struct xchk_iallocbt {
> +	/* owner info for inode blocks */
> +	struct xfs_owner_info	oinfo;
> +
>  	/* Number of inodes we see while scanning inobt. */
>  	unsigned long long	inodes;
> +
> +	/* Blocks and inodes per inode cluster. */
> +	unsigned int		blocks_per_cluster;
> +	unsigned int		inodes_per_cluster;
> +
> +	/* Block alignment of inode clusters. */
> +	unsigned int		cluster_align;
>  };
>  
>  /*
> @@ -189,32 +199,30 @@ xchk_iallocbt_check_cluster_freemask(
>  STATIC int
>  xchk_iallocbt_check_freemask(
>  	struct xchk_btree		*bs,
> +	struct xchk_iallocbt		*iabt,
>  	struct xfs_inobt_rec_incore	*irec)
>  {
> -	struct xfs_owner_info		oinfo;
>  	struct xfs_imap			imap;
>  	struct xfs_mount		*mp = bs->cur->bc_mp;
>  	struct xfs_dinode		*dip;
>  	struct xfs_buf			*bp;
>  	xfs_ino_t			fsino;
> -	xfs_agino_t			nr_inodes;
> +	unsigned int			nr_inodes;
>  	xfs_agino_t			agino;
>  	xfs_agino_t			chunkino;
>  	xfs_agino_t			clusterino;
>  	xfs_agblock_t			agbno;
> -	int				blks_per_cluster;
>  	uint16_t			holemask;
>  	uint16_t			ir_holemask;
>  	int				error = 0;
>  
>  	/* Make sure the freemask matches the inode records. */
> -	blks_per_cluster = xfs_icluster_size_fsb(mp);
> -	nr_inodes = XFS_OFFBNO_TO_AGINO(mp, blks_per_cluster, 0);
> -	xfs_rmap_ag_owner(&oinfo, XFS_RMAP_OWN_INODES);
> +	nr_inodes = min_t(unsigned int, iabt->inodes_per_cluster,
> +			XFS_INODES_PER_CHUNK);

That's a bug fix not just a mechanical change, right? Can you either
call it out in the commit message, or put it in a separate patch?

Apart from that, the patch is good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

> @@ -440,9 +448,15 @@ xchk_iallocbt(
>  	struct xfs_owner_info	oinfo;
>  	struct xchk_iallocbt	iabt = {
>  		.inodes		= 0,
> +		.cluster_align	= xfs_ialloc_cluster_alignment(sc->mp),
> +		.blocks_per_cluster = xfs_icluster_size_fsb(sc->mp),
>  	};
>  	int			error;
>  
> +	xfs_rmap_ag_owner(&iabt.oinfo, XFS_RMAP_OWN_INODES);
> +	iabt.inodes_per_cluster = XFS_OFFBNO_TO_AGINO(sc->mp,
> +			iabt.blocks_per_cluster, 0);

As an aside, this Seems like an obtuse way to calculate inodes per
cluster. It is just:

	inodes_per_cluster = blocks_per_cluster << mp->m_sb.sb_inopblog;

I guess that's exactly what the macro degenerates to, but there's
lots of code with that exact open coded calculation for inodes per
cluster.

/me wonders if we should just calculate these two values at mount
time and stuff them in the struct xfs_mount...

Cheers,

Dave.
Darrick J. Wong Nov. 7, 2018, 3:40 a.m. UTC | #2
On Wed, Nov 07, 2018 at 08:32:18AM +1100, Dave Chinner wrote:
> On Mon, Nov 05, 2018 at 08:08:18PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Hoist all the inode cluster geometry information into struct
> > xchk_iallocbt instead of recomputing them over and over.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/scrub/ialloc.c |   42 ++++++++++++++++++++++++++++--------------
> >  1 file changed, 28 insertions(+), 14 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/scrub/ialloc.c b/fs/xfs/scrub/ialloc.c
> > index 76ae8338a42e..3c12a0fe3b38 100644
> > --- a/fs/xfs/scrub/ialloc.c
> > +++ b/fs/xfs/scrub/ialloc.c
> > @@ -45,8 +45,18 @@ xchk_setup_ag_iallocbt(
> >  /* Inode btree scrubber. */
> >  
> >  struct xchk_iallocbt {
> > +	/* owner info for inode blocks */
> > +	struct xfs_owner_info	oinfo;
> > +
> >  	/* Number of inodes we see while scanning inobt. */
> >  	unsigned long long	inodes;
> > +
> > +	/* Blocks and inodes per inode cluster. */
> > +	unsigned int		blocks_per_cluster;
> > +	unsigned int		inodes_per_cluster;
> > +
> > +	/* Block alignment of inode clusters. */
> > +	unsigned int		cluster_align;
> >  };
> >  
> >  /*
> > @@ -189,32 +199,30 @@ xchk_iallocbt_check_cluster_freemask(
> >  STATIC int
> >  xchk_iallocbt_check_freemask(
> >  	struct xchk_btree		*bs,
> > +	struct xchk_iallocbt		*iabt,
> >  	struct xfs_inobt_rec_incore	*irec)
> >  {
> > -	struct xfs_owner_info		oinfo;
> >  	struct xfs_imap			imap;
> >  	struct xfs_mount		*mp = bs->cur->bc_mp;
> >  	struct xfs_dinode		*dip;
> >  	struct xfs_buf			*bp;
> >  	xfs_ino_t			fsino;
> > -	xfs_agino_t			nr_inodes;
> > +	unsigned int			nr_inodes;
> >  	xfs_agino_t			agino;
> >  	xfs_agino_t			chunkino;
> >  	xfs_agino_t			clusterino;
> >  	xfs_agblock_t			agbno;
> > -	int				blks_per_cluster;
> >  	uint16_t			holemask;
> >  	uint16_t			ir_holemask;
> >  	int				error = 0;
> >  
> >  	/* Make sure the freemask matches the inode records. */
> > -	blks_per_cluster = xfs_icluster_size_fsb(mp);
> > -	nr_inodes = XFS_OFFBNO_TO_AGINO(mp, blks_per_cluster, 0);
> > -	xfs_rmap_ag_owner(&oinfo, XFS_RMAP_OWN_INODES);
> > +	nr_inodes = min_t(unsigned int, iabt->inodes_per_cluster,
> > +			XFS_INODES_PER_CHUNK);
> 
> That's a bug fix not just a mechanical change, right? Can you either
> call it out in the commit message, or put it in a separate patch?

I'm pulling this into a separate patch.

> Apart from that, the patch is good.
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> > @@ -440,9 +448,15 @@ xchk_iallocbt(
> >  	struct xfs_owner_info	oinfo;
> >  	struct xchk_iallocbt	iabt = {
> >  		.inodes		= 0,
> > +		.cluster_align	= xfs_ialloc_cluster_alignment(sc->mp),
> > +		.blocks_per_cluster = xfs_icluster_size_fsb(sc->mp),
> >  	};
> >  	int			error;
> >  
> > +	xfs_rmap_ag_owner(&iabt.oinfo, XFS_RMAP_OWN_INODES);
> > +	iabt.inodes_per_cluster = XFS_OFFBNO_TO_AGINO(sc->mp,
> > +			iabt.blocks_per_cluster, 0);
> 
> As an aside, this Seems like an obtuse way to calculate inodes per
> cluster. It is just:
> 
> 	inodes_per_cluster = blocks_per_cluster << mp->m_sb.sb_inopblog;
> 
> I guess that's exactly what the macro degenerates to, but there's
> lots of code with that exact open coded calculation for inodes per
> cluster.
> 
> /me wonders if we should just calculate these two values at mount
> time and stuff them in the struct xfs_mount...

FWIW I went with:

#define	XFS_FSB_TO_INO(mp,b)	(((b) << (mp)->m_sb.sb_inopblog))

and converted the existing XFS_OFFBNO_TO_AGINO(..., 0) callers to use
it.  I'll work on moving all that inode geometry stuff to a separate
structure, but that'll come after this series.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Patch
diff mbox series

diff --git a/fs/xfs/scrub/ialloc.c b/fs/xfs/scrub/ialloc.c
index 76ae8338a42e..3c12a0fe3b38 100644
--- a/fs/xfs/scrub/ialloc.c
+++ b/fs/xfs/scrub/ialloc.c
@@ -45,8 +45,18 @@  xchk_setup_ag_iallocbt(
 /* Inode btree scrubber. */
 
 struct xchk_iallocbt {
+	/* owner info for inode blocks */
+	struct xfs_owner_info	oinfo;
+
 	/* Number of inodes we see while scanning inobt. */
 	unsigned long long	inodes;
+
+	/* Blocks and inodes per inode cluster. */
+	unsigned int		blocks_per_cluster;
+	unsigned int		inodes_per_cluster;
+
+	/* Block alignment of inode clusters. */
+	unsigned int		cluster_align;
 };
 
 /*
@@ -189,32 +199,30 @@  xchk_iallocbt_check_cluster_freemask(
 STATIC int
 xchk_iallocbt_check_freemask(
 	struct xchk_btree		*bs,
+	struct xchk_iallocbt		*iabt,
 	struct xfs_inobt_rec_incore	*irec)
 {
-	struct xfs_owner_info		oinfo;
 	struct xfs_imap			imap;
 	struct xfs_mount		*mp = bs->cur->bc_mp;
 	struct xfs_dinode		*dip;
 	struct xfs_buf			*bp;
 	xfs_ino_t			fsino;
-	xfs_agino_t			nr_inodes;
+	unsigned int			nr_inodes;
 	xfs_agino_t			agino;
 	xfs_agino_t			chunkino;
 	xfs_agino_t			clusterino;
 	xfs_agblock_t			agbno;
-	int				blks_per_cluster;
 	uint16_t			holemask;
 	uint16_t			ir_holemask;
 	int				error = 0;
 
 	/* Make sure the freemask matches the inode records. */
-	blks_per_cluster = xfs_icluster_size_fsb(mp);
-	nr_inodes = XFS_OFFBNO_TO_AGINO(mp, blks_per_cluster, 0);
-	xfs_rmap_ag_owner(&oinfo, XFS_RMAP_OWN_INODES);
+	nr_inodes = min_t(unsigned int, iabt->inodes_per_cluster,
+			XFS_INODES_PER_CHUNK);
 
 	for (agino = irec->ir_startino;
 	     agino < irec->ir_startino + XFS_INODES_PER_CHUNK;
-	     agino += blks_per_cluster * mp->m_sb.sb_inopblock) {
+	     agino += iabt->blocks_per_cluster * mp->m_sb.sb_inopblock) {
 		fsino = XFS_AGINO_TO_INO(mp, bs->cur->bc_private.a.agno, agino);
 		chunkino = agino - irec->ir_startino;
 		agbno = XFS_AGINO_TO_AGBNO(mp, agino);
@@ -235,17 +243,17 @@  xchk_iallocbt_check_freemask(
 		/* If any part of this is a hole, skip it. */
 		if (ir_holemask) {
 			xchk_xref_is_not_owned_by(bs->sc, agbno,
-					blks_per_cluster, &oinfo);
+					iabt->blocks_per_cluster, &iabt->oinfo);
 			continue;
 		}
 
-		xchk_xref_is_owned_by(bs->sc, agbno, blks_per_cluster,
-				&oinfo);
+		xchk_xref_is_owned_by(bs->sc, agbno, iabt->blocks_per_cluster,
+				&iabt->oinfo);
 
 		/* Grab the inode cluster buffer. */
 		imap.im_blkno = XFS_AGB_TO_DADDR(mp, bs->cur->bc_private.a.agno,
 				agbno);
-		imap.im_len = XFS_FSB_TO_BB(mp, blks_per_cluster);
+		imap.im_len = XFS_FSB_TO_BB(mp, iabt->blocks_per_cluster);
 		imap.im_boffset = 0;
 
 		error = xfs_imap_to_bp(mp, bs->cur->bc_tp, &imap,
@@ -311,8 +319,8 @@  xchk_iallocbt_rec(
 
 	/* Make sure this record is aligned to cluster and inoalignmnt size. */
 	agbno = XFS_AGINO_TO_AGBNO(mp, irec.ir_startino);
-	if ((agbno & (xfs_ialloc_cluster_alignment(mp) - 1)) ||
-	    (agbno & (xfs_icluster_size_fsb(mp) - 1)))
+	if ((agbno & (iabt->cluster_align - 1)) ||
+	    (agbno & (iabt->blocks_per_cluster - 1)))
 		xchk_btree_set_corrupt(bs->sc, bs->cur, 0);
 
 	iabt->inodes += irec.ir_count;
@@ -353,7 +361,7 @@  xchk_iallocbt_rec(
 		xchk_btree_set_corrupt(bs->sc, bs->cur, 0);
 
 check_freemask:
-	error = xchk_iallocbt_check_freemask(bs, &irec);
+	error = xchk_iallocbt_check_freemask(bs, iabt, &irec);
 	if (error)
 		goto out;
 
@@ -440,9 +448,15 @@  xchk_iallocbt(
 	struct xfs_owner_info	oinfo;
 	struct xchk_iallocbt	iabt = {
 		.inodes		= 0,
+		.cluster_align	= xfs_ialloc_cluster_alignment(sc->mp),
+		.blocks_per_cluster = xfs_icluster_size_fsb(sc->mp),
 	};
 	int			error;
 
+	xfs_rmap_ag_owner(&iabt.oinfo, XFS_RMAP_OWN_INODES);
+	iabt.inodes_per_cluster = XFS_OFFBNO_TO_AGINO(sc->mp,
+			iabt.blocks_per_cluster, 0);
+
 	xfs_rmap_ag_owner(&oinfo, XFS_RMAP_OWN_INOBT);
 	cur = which == XFS_BTNUM_INO ? sc->sa.ino_cur : sc->sa.fino_cur;
 	error = xchk_btree(sc, cur, xchk_iallocbt_rec, &oinfo, &iabt);