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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
>
> > @@ -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@xxxxxxxxxxxxx
