Re: [PATCH 01/11] xfs: separate inode geometry

[Dave Chinner <david@fromorbit.com>]

On Wed, May 29, 2019 at 03:26:20PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Separate the inode geometry information into a distinct structure.

I like the idea, but have lots of comments on the patch....

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_format.h       |   33 +++++++++++-
>  fs/xfs/libxfs/xfs_ialloc.c       |  109 ++++++++++++++++++++------------------
>  fs/xfs/libxfs/xfs_ialloc.h       |    6 +-
>  fs/xfs/libxfs/xfs_ialloc_btree.c |   15 +++--
>  fs/xfs/libxfs/xfs_inode_buf.c    |    2 -
>  fs/xfs/libxfs/xfs_sb.c           |   24 +++++---
>  fs/xfs/libxfs/xfs_trans_resv.c   |   17 +++---
>  fs/xfs/libxfs/xfs_trans_space.h  |    7 +-
>  fs/xfs/libxfs/xfs_types.c        |    4 +
>  fs/xfs/scrub/ialloc.c            |   22 ++++----
>  fs/xfs/scrub/quota.c             |    2 -
>  fs/xfs/xfs_fsops.c               |    4 +
>  fs/xfs/xfs_inode.c               |   17 +++---
>  fs/xfs/xfs_itable.c              |   11 ++--
>  fs/xfs/xfs_log_recover.c         |   23 ++++----
>  fs/xfs/xfs_mount.c               |   49 +++++++++--------
>  fs/xfs/xfs_mount.h               |   17 ------
>  fs/xfs/xfs_super.c               |    6 +-
>  18 files changed, 205 insertions(+), 163 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index 9bb3c48843ec..66f527b1c461 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -1071,7 +1071,7 @@ static inline void xfs_dinode_put_rdev(struct xfs_dinode *dip, xfs_dev_t rdev)
>  #define	XFS_INO_MASK(k)			(uint32_t)((1ULL << (k)) - 1)
>  #define	XFS_INO_OFFSET_BITS(mp)		(mp)->m_sb.sb_inopblog
>  #define	XFS_INO_AGBNO_BITS(mp)		(mp)->m_sb.sb_agblklog
> -#define	XFS_INO_AGINO_BITS(mp)		(mp)->m_agino_log
> +#define	XFS_INO_AGINO_BITS(mp)		((mp)->m_ino_geo.ig_agino_log)
>  #define	XFS_INO_AGNO_BITS(mp)		(mp)->m_agno_log
>  #define	XFS_INO_BITS(mp)		\
>  	XFS_INO_AGNO_BITS(mp) + XFS_INO_AGINO_BITS(mp)
> @@ -1694,4 +1694,35 @@ struct xfs_acl {
>  #define SGI_ACL_FILE_SIZE	(sizeof(SGI_ACL_FILE)-1)
>  #define SGI_ACL_DEFAULT_SIZE	(sizeof(SGI_ACL_DEFAULT)-1)
>  
> +struct xfs_ino_geometry {
> +	/* Maximum inode count in this filesystem. */
> +	uint64_t	ig_maxicount;

Naming is hard. What's the point of adding "ig_" prefix when the
variables mostly already have an "i" somewhere in them that means
"inode"?  And when they are referenced as igeo->ig_i...., then we've
got so much redudant namespace in the code.....

This is one of the reasons when the struct xfs_da_geometry is not
namespaced - it's clear from the code context it's
directory/attribute geometry info, so it doesn't need lots of extra
namespace gunk.

> +
> +	/* Minimum inode buffer size, in bytes. */
> +	unsigned int	ig_min_cluster_size;

What does the "minimum" in this variable mean? cluster size is fixed
for a filesystem, there's no minimum or maximum size....

> +
> +	/* Inode cluster sizes, adjusted to be at least 1 fsb. */
> +	unsigned int	ig_inodes_per_cluster;
> +	unsigned int	ig_blocks_per_cluster;
> +
> +	/* Inode cluster alignment. */
> +	unsigned int	ig_cluster_align;
> +	unsigned int	ig_cluster_align_inodes;
> +
> +	unsigned int	ig_inobt_mxr[2]; /* max inobt btree records */
> +	unsigned int	ig_inobt_mnr[2]; /* min inobt btree records */
> +	unsigned int	ig_in_maxlevels; /* max inobt btree levels. */

inobt_maxlevels?

> +
> +	/* Minimum inode allocation size */
> +	unsigned int	ig_ialloc_inos;
> +	unsigned int	ig_ialloc_blks;

What's "minimum" about these values?

> +	/* Minimum inode blocks for a sparse allocation. */
> +	unsigned int	ig_ialloc_min_blks;
> +
> +	unsigned int	ig_inoalign_mask;/* mask sb_inoalignmt if used */

This goes with the cluster alignment variables, it's always set by
mkfs and used to convert inode numbers to cluster agbnos...

> +	unsigned int	ig_agino_log;	/* #bits for agino in inum */
> +	unsigned int	ig_sinoalign;	/* stripe unit inode alignment */

And this one should be renamed ialloc_align and moved up with the
the other ialloc variables....


> +};
> +
>  #endif /* __XFS_FORMAT_H__ */
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index fe9898875097..c881e0521331 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -299,7 +299,7 @@ xfs_ialloc_inode_init(
>  	 * sizes, manipulate the inodes in buffers  which are multiples of the
>  	 * blocks size.
>  	 */
> -	nbufs = length / mp->m_blocks_per_cluster;
> +	nbufs = length / mp->m_ino_geo.ig_blocks_per_cluster;
>  
>  	/*
>  	 * Figure out what version number to use in the inodes we create.  If
> @@ -343,9 +343,10 @@ xfs_ialloc_inode_init(
>  		 * Get the block.
>  		 */
>  		d = XFS_AGB_TO_DADDR(mp, agno, agbno +
> -				(j * mp->m_blocks_per_cluster));
> +				(j * mp->m_ino_geo.ig_blocks_per_cluster));
>  		fbuf = xfs_trans_get_buf(tp, mp->m_ddev_targp, d,
> -					 mp->m_bsize * mp->m_blocks_per_cluster,
> +					 mp->m_bsize *
> +					 mp->m_ino_geo.ig_blocks_per_cluster,
>  					 XBF_UNMAPPED);

This doesn't improve readability of the code. Please use a local
igeom variable rather than repeatedly using mp->m_ino_geo.ig_....
in the function.


> @@ -690,10 +693,10 @@ xfs_ialloc_ag_alloc(
>  		 * but not to use them in the actual exact allocation.
>  		 */
>  		args.alignment = 1;
> -		args.minalignslop = args.mp->m_cluster_align - 1;
> +		args.minalignslop = args.mp->m_ino_geo.ig_cluster_align - 1;

Ummm, why not igeo->... , like:

>  
>  		/* Allow space for the inode btree to split. */
> -		args.minleft = args.mp->m_in_maxlevels - 1;
> +		args.minleft = igeo->ig_in_maxlevels - 1;

3 lines down?

>  		if ((error = xfs_alloc_vextent(&args)))
>  			return error;
>  
> @@ -720,12 +723,12 @@ xfs_ialloc_ag_alloc(
>  		 * pieces, so don't need alignment anyway.
>  		 */
>  		isaligned = 0;
> -		if (args.mp->m_sinoalign) {
> +		if (igeo->ig_sinoalign) {
>  			ASSERT(!(args.mp->m_flags & XFS_MOUNT_NOALIGN));
>  			args.alignment = args.mp->m_dalign;
>  			isaligned = 1;
>  		} else
> -			args.alignment = args.mp->m_cluster_align;
> +			args.alignment = args.mp->m_ino_geo.ig_cluster_align;

Ditto (and others).

>  	int			noroom = 0;
>  	xfs_agnumber_t		start_agno;
>  	struct xfs_perag	*pag;
> +	struct xfs_ino_geometry	*igeo = &mp->m_ino_geo;
>  	int			okalloc = 1;
>  
>  	if (*IO_agbp) {
> @@ -1733,9 +1737,9 @@ xfs_dialloc(
>  	 * Read rough value of mp->m_icount by percpu_counter_read_positive,
>  	 * which will sacrifice the preciseness but improve the performance.
>  	 */
> -	if (mp->m_maxicount &&
> -	    percpu_counter_read_positive(&mp->m_icount) + mp->m_ialloc_inos
> -							> mp->m_maxicount) {
> +	if (mp->m_ino_geo.ig_maxicount &&

igeo?

> +	    percpu_counter_read_positive(&mp->m_icount) + igeo->ig_ialloc_inos
> +							> igeo->ig_maxicount) {
>  		noroom = 1;
>  		okalloc = 0;
>  	}
> @@ -1852,7 +1856,8 @@ xfs_difree_inode_chunk(
>  	if (!xfs_inobt_issparse(rec->ir_holemask)) {
>  		/* not sparse, calculate extent info directly */
>  		xfs_bmap_add_free(tp, XFS_AGB_TO_FSB(mp, agno, sagbno),
> -				  mp->m_ialloc_blks, &XFS_RMAP_OINFO_INODES);
> +				  mp->m_ino_geo.ig_ialloc_blks,
> +				  &XFS_RMAP_OINFO_INODES);
>  		return;
>  	}
>  
> @@ -2261,7 +2266,7 @@ xfs_imap_lookup(
>  
>  	/* check that the returned record contains the required inode */
>  	if (rec.ir_startino > agino ||
> -	    rec.ir_startino + mp->m_ialloc_inos <= agino)
> +	    rec.ir_startino + mp->m_ino_geo.ig_ialloc_inos <= agino)
>  		return -EINVAL;
>  
>  	/* for untrusted inodes check it is allocated first */
> @@ -2352,7 +2357,7 @@ xfs_imap(
>  	 * If the inode cluster size is the same as the blocksize or
>  	 * smaller we get to the buffer by simple arithmetics.
>  	 */
> -	if (mp->m_blocks_per_cluster == 1) {
> +	if (mp->m_ino_geo.ig_blocks_per_cluster == 1) {

igeo...

>  		offset = XFS_INO_TO_OFFSET(mp, ino);
>  		ASSERT(offset < mp->m_sb.sb_inopblock);
>  
> @@ -2368,8 +2373,8 @@ xfs_imap(
>  	 * find the location. Otherwise we have to do a btree
>  	 * lookup to find the location.
>  	 */
> -	if (mp->m_inoalign_mask) {
> -		offset_agbno = agbno & mp->m_inoalign_mask;
> +	if (mp->m_ino_geo.ig_inoalign_mask) {
> +		offset_agbno = agbno & mp->m_ino_geo.ig_inoalign_mask;

and here too.

>  		chunk_agbno = agbno - offset_agbno;
>  	} else {
>  		error = xfs_imap_lookup(mp, tp, agno, agino, agbno,
> @@ -2381,13 +2386,13 @@ xfs_imap(
>  out_map:
>  	ASSERT(agbno >= chunk_agbno);
>  	cluster_agbno = chunk_agbno +
> -		((offset_agbno / mp->m_blocks_per_cluster) *
> -		 mp->m_blocks_per_cluster);
> +		((offset_agbno / mp->m_ino_geo.ig_blocks_per_cluster) *
> +		 mp->m_ino_geo.ig_blocks_per_cluster);

And here.

>  	offset = ((agbno - cluster_agbno) * mp->m_sb.sb_inopblock) +
>  		XFS_INO_TO_OFFSET(mp, ino);
>  
>  	imap->im_blkno = XFS_AGB_TO_DADDR(mp, agno, cluster_agbno);
> -	imap->im_len = XFS_FSB_TO_BB(mp, mp->m_blocks_per_cluster);
> +	imap->im_len = XFS_FSB_TO_BB(mp, mp->m_ino_geo.ig_blocks_per_cluster);

and here...

>  	imap->im_boffset = (unsigned short)(offset << mp->m_sb.sb_inodelog);
>  
>  	/*
> @@ -2409,7 +2414,7 @@ xfs_imap(
>  }
>  
>  /*
> - * Compute and fill in value of m_in_maxlevels.
> + * Compute and fill in value of m_ino_geo.ig_in_maxlevels.
>   */
>  void
>  xfs_ialloc_compute_maxlevels(
> @@ -2418,8 +2423,8 @@ xfs_ialloc_compute_maxlevels(
>  	uint		inodes;
>  
>  	inodes = (1LL << XFS_INO_AGINO_BITS(mp)) >> XFS_INODES_PER_CHUNK_LOG;
> -	mp->m_in_maxlevels = xfs_btree_compute_maxlevels(mp->m_inobt_mnr,
> -							 inodes);
> +	mp->m_ino_geo.ig_in_maxlevels = xfs_btree_compute_maxlevels(
> +			mp->m_ino_geo.ig_inobt_mnr, inodes);


Once we take away the macro:

	inode = (1LL << igeo->agino_log) >> XFS_INODES_PER_CHUNK_LOG
	igeo->inobt_maxlevels = xfs_btree_compute_maxlevels(igeo->inobt_mnr,
							inodes);

So, shouldn't we just pass igeo into this function now?

>  }
>  
>  /*
> diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
> index e936b7cc9389..b74fa2addd51 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.h
> +++ b/fs/xfs/libxfs/xfs_ialloc.h
> @@ -28,9 +28,9 @@ static inline int
>  xfs_icluster_size_fsb(
>  	struct xfs_mount	*mp)
>  {
> -	if (mp->m_sb.sb_blocksize >= mp->m_inode_cluster_size)
> +	if (mp->m_sb.sb_blocksize >= mp->m_ino_geo.ig_min_cluster_size)
>  		return 1;
> -	return mp->m_inode_cluster_size >> mp->m_sb.sb_blocklog;
> +	return mp->m_ino_geo.ig_min_cluster_size >> mp->m_sb.sb_blocklog;
>  }

The return value of this is placed in the mp->m_ino_geo structure.
This should pass in the igeo structure the result is written into.
It's other caller should be using the value in the igeo structure,
not calling this function.

>  
> index bc2dfacd2f4a..79cc5cf21e1b 100644
> --- a/fs/xfs/libxfs/xfs_ialloc_btree.c
> +++ b/fs/xfs/libxfs/xfs_ialloc_btree.c
> @@ -28,7 +28,7 @@ xfs_inobt_get_minrecs(
>  	struct xfs_btree_cur	*cur,
>  	int			level)
>  {
> -	return cur->bc_mp->m_inobt_mnr[level != 0];
> +	return cur->bc_mp->m_ino_geo.ig_inobt_mnr[level != 0];
>  }

Put a igeo pointer in the inobt union of the btree cursor?

	return cur->bc_private.a.igeo->inobt_mnr[level != 0];

>  
>  STATIC struct xfs_btree_cur *
> @@ -164,7 +164,7 @@ xfs_inobt_get_maxrecs(
>  	struct xfs_btree_cur	*cur,
>  	int			level)
>  {
> -	return cur->bc_mp->m_inobt_mxr[level != 0];
> +	return cur->bc_mp->m_ino_geo.ig_inobt_mxr[level != 0];
>  }
>  
>  STATIC void
> @@ -281,10 +281,11 @@ xfs_inobt_verify(
>  
>  	/* level verification */
>  	level = be16_to_cpu(block->bb_level);
> -	if (level >= mp->m_in_maxlevels)
> +	if (level >= mp->m_ino_geo.ig_in_maxlevels)
>  		return __this_address;
>  
> -	return xfs_btree_sblock_verify(bp, mp->m_inobt_mxr[level != 0]);
> +	return xfs_btree_sblock_verify(bp,
> +			mp->m_ino_geo.ig_inobt_mxr[level != 0]);
>  }
>  
>  static void
> @@ -546,7 +547,7 @@ xfs_inobt_max_size(
>  	xfs_agblock_t		agblocks = xfs_ag_block_count(mp, agno);
>  
>  	/* Bail out if we're uninitialized, which can happen in mkfs. */
> -	if (mp->m_inobt_mxr[0] == 0)
> +	if (mp->m_ino_geo.ig_inobt_mxr[0] == 0)
>  		return 0;
>  
>  	/*
> @@ -558,7 +559,7 @@ xfs_inobt_max_size(
>  	    XFS_FSB_TO_AGNO(mp, mp->m_sb.sb_logstart) == agno)
>  		agblocks -= mp->m_sb.sb_logblocks;
>  
> -	return xfs_btree_calc_size(mp->m_inobt_mnr,
> +	return xfs_btree_calc_size(mp->m_ino_geo.ig_inobt_mnr,
>  				(uint64_t)agblocks * mp->m_sb.sb_inopblock /
>  					XFS_INODES_PER_CHUNK);
>  }
> @@ -619,5 +620,5 @@ xfs_iallocbt_calc_size(
>  	struct xfs_mount	*mp,
>  	unsigned long long	len)
>  {
> -	return xfs_btree_calc_size(mp->m_inobt_mnr, len);
> +	return xfs_btree_calc_size(mp->m_ino_geo.ig_inobt_mnr, len);

Should pass igeo into this function now, not xfs_mount.

>  }
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index e021d5133ccb..641aa1c2f1ae 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -36,7 +36,7 @@ xfs_inobp_check(
>  	int		j;
>  	xfs_dinode_t	*dip;
>  
> -	j = mp->m_inode_cluster_size >> mp->m_sb.sb_inodelog;
> +	j = mp->m_ino_geo.ig_min_cluster_size >> mp->m_sb.sb_inodelog;

isn't that "inodes per cluster"?

>  
>  	for (i = 0; i < j; i++) {
>  		dip = xfs_buf_offset(bp, i * mp->m_sb.sb_inodesize);
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index e76a3e5d28d7..9416fc741788 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -804,16 +804,18 @@ const struct xfs_buf_ops xfs_sb_quiet_buf_ops = {
>   */
>  void
>  xfs_sb_mount_common(
> -	struct xfs_mount *mp,
> -	struct xfs_sb	*sbp)
> +	struct xfs_mount	*mp,
> +	struct xfs_sb		*sbp)
>  {
> +	struct xfs_ino_geometry	*igeo = &mp->m_ino_geo;
> +
>  	mp->m_agfrotor = mp->m_agirotor = 0;
>  	mp->m_maxagi = mp->m_sb.sb_agcount;
>  	mp->m_blkbit_log = sbp->sb_blocklog + XFS_NBBYLOG;
>  	mp->m_blkbb_log = sbp->sb_blocklog - BBSHIFT;
>  	mp->m_sectbb_log = sbp->sb_sectlog - BBSHIFT;
>  	mp->m_agno_log = xfs_highbit32(sbp->sb_agcount - 1) + 1;
> -	mp->m_agino_log = sbp->sb_inopblog + sbp->sb_agblklog;
> +	mp->m_ino_geo.ig_agino_log = sbp->sb_inopblog + sbp->sb_agblklog;

igeo.

>  
> @@ -307,7 +308,8 @@ xfs_calc_iunlink_remove_reservation(
>  	struct xfs_mount        *mp)
>  {
>  	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> -	       2 * max_t(uint, XFS_FSB_TO_B(mp, 1), mp->m_inode_cluster_size);
> +	       2 * max_t(uint, XFS_FSB_TO_B(mp, 1),
> +			 mp->m_ino_geo.ig_min_cluster_size);
>  }

I'm starting to think a M_IGEO(mp)-like macro is in order here....

>  
> diff --git a/fs/xfs/scrub/ialloc.c b/fs/xfs/scrub/ialloc.c
> index 9b47117180cb..fa7386bf76e9 100644
> --- a/fs/xfs/scrub/ialloc.c
> +++ b/fs/xfs/scrub/ialloc.c
> @@ -230,7 +230,7 @@ xchk_iallocbt_check_cluster(
>  	int				error = 0;
>  
>  	nr_inodes = min_t(unsigned int, XFS_INODES_PER_CHUNK,
> -			mp->m_inodes_per_cluster);
> +			mp->m_ino_geo.ig_inodes_per_cluster);

igeo.... (many uses in this function)

> @@ -355,6 +356,7 @@ xchk_iallocbt_rec_alignment(
>  {
>  	struct xfs_mount		*mp = bs->sc->mp;
>  	struct xchk_iallocbt		*iabt = bs->private;
> +	struct xfs_ino_geometry		*ig = &mp->m_ino_geo;

igeo, for consistency with the rest of the code.

> @@ -2567,7 +2568,8 @@ xfs_ifree_cluster(
>  		 * to mark all the active inodes on the buffer stale.
>  		 */
>  		bp = xfs_trans_get_buf(tp, mp->m_ddev_targp, blkno,
> -					mp->m_bsize * mp->m_blocks_per_cluster,
> +					mp->m_bsize *
> +						igeo->ig_blocks_per_cluster,
>  					XBF_UNMAPPED);

Back off the indent, don't use another line :)

> @@ -3476,19 +3478,20 @@ xfs_iflush_cluster(
>  	int			cilist_size;
>  	struct xfs_inode	**cilist;
>  	struct xfs_inode	*cip;
> +	struct xfs_ino_geometry	*igeo = &mp->m_ino_geo;
>  	int			nr_found;
>  	int			clcount = 0;
>  	int			i;
>  
>  	pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, ip->i_ino));
>  
> -	inodes_per_cluster = mp->m_inode_cluster_size >> mp->m_sb.sb_inodelog;
> +	inodes_per_cluster = igeo->ig_min_cluster_size >> mp->m_sb.sb_inodelog;

that's igeo->inodes_per_cluster again, right?

>  	cilist_size = inodes_per_cluster * sizeof(xfs_inode_t *);
>  	cilist = kmem_alloc(cilist_size, KM_MAYFAIL|KM_NOFS);
>  	if (!cilist)
>  		goto out_put;
>  
> -	mask = ~(((mp->m_inode_cluster_size >> mp->m_sb.sb_inodelog)) - 1);
> +	mask = ~(((igeo->ig_min_cluster_size >> mp->m_sb.sb_inodelog)) - 1);

Isn't that:

	mask = ~(inodes_per_cluster - 1);

>  	first_index = XFS_INO_TO_AGINO(mp, ip->i_ino) & mask;
>  	rcu_read_lock();
>  	/* really need a gang lookup range call here */
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 1e1a0af1dd34..cff28ee73deb 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -167,6 +167,7 @@ xfs_bulkstat_ichunk_ra(
>  	xfs_agnumber_t			agno,
>  	struct xfs_inobt_rec_incore	*irec)
>  {
> +	struct xfs_ino_geometry		*igeo = &mp->m_ino_geo;
>  	xfs_agblock_t			agbno;
>  	struct blk_plug			plug;
>  	int				i;	/* inode chunk index */
> @@ -174,12 +175,14 @@ xfs_bulkstat_ichunk_ra(
>  	agbno = XFS_AGINO_TO_AGBNO(mp, irec->ir_startino);
>  
>  	blk_start_plug(&plug);
> -	for (i = 0; i < XFS_INODES_PER_CHUNK;
> -	     i += mp->m_inodes_per_cluster, agbno += mp->m_blocks_per_cluster) {
> -		if (xfs_inobt_maskn(i, mp->m_inodes_per_cluster) &
> +	for (i = 0;
> +	     i < XFS_INODES_PER_CHUNK;
> +	     i += igeo->ig_inodes_per_cluster,
> +			agbno += igeo->ig_blocks_per_cluster) {
> +		if (xfs_inobt_maskn(i, igeo->ig_inodes_per_cluster) &
>  		    ~irec->ir_free) {
>  			xfs_btree_reada_bufs(mp, agno, agbno,
> -					mp->m_blocks_per_cluster,
> +					igeo->ig_blocks_per_cluster,
>  					&xfs_inode_buf_ops);
>  		}

That's a mess :(

	for (i = 0; i < XFS_INODES_PER_CHUNK; i += igeo->inodes_per_cluster) {
		if (xfs_inobt_maskn(i, igeo->inodes_per_cluster) &
							~irec->ir_free) {
			xfs_btree_reada_bufs(mp, agno, agbno,
					igeo->ig_blocks_per_cluster,
					&xfs_inode_buf_ops);
		}
		agbno += igeo->blocks_per_cluster;
	}

> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 9329f5adbfbe..15118e531184 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2882,19 +2882,19 @@ xlog_recover_buffer_pass2(
>  	 *
>  	 * Also make sure that only inode buffers with good sizes stay in
>  	 * the buffer cache.  The kernel moves inodes in buffers of 1 block
> -	 * or mp->m_inode_cluster_size bytes, whichever is bigger.  The inode
> +	 * or ig_min_cluster_size bytes, whichever is bigger.  The inode
>  	 * buffers in the log can be a different size if the log was generated
>  	 * by an older kernel using unclustered inode buffers or a newer kernel
>  	 * running with a different inode cluster size.  Regardless, if the
> -	 * the inode buffer size isn't max(blocksize, mp->m_inode_cluster_size)
> -	 * for *our* value of mp->m_inode_cluster_size, then we need to keep
> +	 * the inode buffer size isn't max(blocksize, ig_min_cluster_size)
> +	 * for *our* value of ig_min_cluster_size, then we need to keep
>  	 * the buffer out of the buffer cache so that the buffer won't
>  	 * overlap with future reads of those inodes.
>  	 */
>  	if (XFS_DINODE_MAGIC ==
>  	    be16_to_cpu(*((__be16 *)xfs_buf_offset(bp, 0))) &&
>  	    (BBTOB(bp->b_io_length) != max(log->l_mp->m_sb.sb_blocksize,
> -			(uint32_t)log->l_mp->m_inode_cluster_size))) {
> +			(uint32_t)log->l_mp->m_ino_geo.ig_min_cluster_size))) {

cluster size is already an unsigned int so the cast ca go.

>  		xfs_buf_stale(bp);
>  		error = xfs_bwrite(bp);
>  	} else {
> @@ -3849,6 +3849,7 @@ xlog_recover_do_icreate_pass2(
>  {
>  	struct xfs_mount	*mp = log->l_mp;
>  	struct xfs_icreate_log	*icl;
> +	struct xfs_ino_geometry	*igeo = &mp->m_ino_geo;
>  	xfs_agnumber_t		agno;
>  	xfs_agblock_t		agbno;
>  	unsigned int		count;
> @@ -3898,10 +3899,10 @@ xlog_recover_do_icreate_pass2(
>  
>  	/*
>  	 * The inode chunk is either full or sparse and we only support
> -	 * m_ialloc_min_blks sized sparse allocations at this time.
> +	 * m_ino_geo.ig_ialloc_min_blks sized sparse allocations at this time.
>  	 */
> -	if (length != mp->m_ialloc_blks &&
> -	    length != mp->m_ialloc_min_blks) {
> +	if (length != igeo->ig_ialloc_blks &&
> +	    length != igeo->ig_ialloc_min_blks) {
>  		xfs_warn(log->l_mp,
>  			 "%s: unsupported chunk length", __FUNCTION__);
>  		return -EINVAL;
> @@ -3921,13 +3922,13 @@ xlog_recover_do_icreate_pass2(
>  	 * buffers for cancellation so we don't overwrite anything written after
>  	 * a cancellation.
>  	 */
> -	bb_per_cluster = XFS_FSB_TO_BB(mp, mp->m_blocks_per_cluster);
> -	nbufs = length / mp->m_blocks_per_cluster;
> +	bb_per_cluster = XFS_FSB_TO_BB(mp, igeo->ig_blocks_per_cluster);
> +	nbufs = length / igeo->ig_blocks_per_cluster;
>  	for (i = 0, cancel_count = 0; i < nbufs; i++) {
>  		xfs_daddr_t	daddr;
>  
> -		daddr = XFS_AGB_TO_DADDR(mp, agno,
> -					 agbno + i * mp->m_blocks_per_cluster);
> +		daddr = XFS_AGB_TO_DADDR(mp, agno, agbno +
> +				i * igeo->ig_blocks_per_cluster);

makes no sense to change the line break location.

		daddr = XFS_AGB_TO_DADDR(mp, agno,
				agbno + i * igeo->ig_blocks_per_cluster);


>   */
>  STATIC void
> -xfs_set_maxicount(xfs_mount_t *mp)
> +xfs_set_maxicount(
> +	struct xfs_mount	*mp)
>  {
> -	xfs_sb_t	*sbp = &(mp->m_sb);
> -	uint64_t	icount;
> +	struct xfs_sb		*sbp = &(mp->m_sb);

kill the ().

> +	struct xfs_ino_geometry	*igeo = &mp->m_ino_geo;
> +	uint64_t		icount;
>  
>  	if (sbp->sb_imax_pct) {
>  		/*
> @@ -445,11 +447,11 @@ xfs_set_maxicount(xfs_mount_t *mp)
>  		 */
>  		icount = sbp->sb_dblocks * sbp->sb_imax_pct;
>  		do_div(icount, 100);
> -		do_div(icount, mp->m_ialloc_blks);
> -		mp->m_maxicount = (icount * mp->m_ialloc_blks)  <<
> -				   sbp->sb_inopblog;
> +		do_div(icount, igeo->ig_ialloc_blks);
> +		igeo->ig_maxicount = XFS_FSB_TO_INO(mp,
> +				icount * igeo->ig_ialloc_blks);
>  	} else {
> -		mp->m_maxicount = 0;
> +		igeo->ig_maxicount = 0;
>  	}
>  }
>  
> @@ -518,18 +520,18 @@ xfs_set_inoalignment(xfs_mount_t *mp)
>  {
>  	if (xfs_sb_version_hasalign(&mp->m_sb) &&
>  		mp->m_sb.sb_inoalignmt >= xfs_icluster_size_fsb(mp))
> -		mp->m_inoalign_mask = mp->m_sb.sb_inoalignmt - 1;
> +		mp->m_ino_geo.ig_inoalign_mask = mp->m_sb.sb_inoalignmt - 1;
>  	else
> -		mp->m_inoalign_mask = 0;
> +		mp->m_ino_geo.ig_inoalign_mask = 0;
>  	/*
>  	 * If we are using stripe alignment, check whether
>  	 * the stripe unit is a multiple of the inode alignment
>  	 */
> -	if (mp->m_dalign && mp->m_inoalign_mask &&
> -	    !(mp->m_dalign & mp->m_inoalign_mask))
> -		mp->m_sinoalign = mp->m_dalign;
> +	if (mp->m_dalign && mp->m_ino_geo.ig_inoalign_mask &&
> +	    !(mp->m_dalign & mp->m_ino_geo.ig_inoalign_mask))
> +		mp->m_ino_geo.ig_sinoalign = mp->m_dalign;
>  	else
> -		mp->m_sinoalign = 0;
> +		mp->m_ino_geo.ig_sinoalign = 0;

should pass in igeo to this function....

>  }
>  
>  /*
> @@ -683,6 +685,7 @@ xfs_mountfs(
>  {
>  	struct xfs_sb		*sbp = &(mp->m_sb);
>  	struct xfs_inode	*rip;
> +	struct xfs_ino_geometry	*igeo = &mp->m_ino_geo;
>  	uint64_t		resblks;
>  	uint			quotamount = 0;
>  	uint			quotaflags = 0;
> @@ -797,18 +800,20 @@ xfs_mountfs(
>  	 * has set the inode alignment value appropriately for larger cluster
>  	 * sizes.
>  	 */
> -	mp->m_inode_cluster_size = XFS_INODE_BIG_CLUSTER_SIZE;
> +	igeo->ig_min_cluster_size = XFS_INODE_BIG_CLUSTER_SIZE;
>  	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> -		int	new_size = mp->m_inode_cluster_size;
> +		int	new_size = igeo->ig_min_cluster_size;
>  
>  		new_size *= mp->m_sb.sb_inodesize / XFS_DINODE_MIN_SIZE;
>  		if (mp->m_sb.sb_inoalignmt >= XFS_B_TO_FSBT(mp, new_size))
> -			mp->m_inode_cluster_size = new_size;
> +			igeo->ig_min_cluster_size = new_size;
>  	}
> -	mp->m_blocks_per_cluster = xfs_icluster_size_fsb(mp);
> -	mp->m_inodes_per_cluster = XFS_FSB_TO_INO(mp, mp->m_blocks_per_cluster);
> -	mp->m_cluster_align = xfs_ialloc_cluster_alignment(mp);
> -	mp->m_cluster_align_inodes = XFS_FSB_TO_INO(mp, mp->m_cluster_align);
> +	igeo->ig_blocks_per_cluster = xfs_icluster_size_fsb(mp);
> +	igeo->ig_inodes_per_cluster = XFS_FSB_TO_INO(mp,
> +			igeo->ig_blocks_per_cluster);
> +	igeo->ig_cluster_align = xfs_ialloc_cluster_alignment(mp);
> +	igeo->ig_cluster_align_inodes = XFS_FSB_TO_INO(mp,
> +			igeo->ig_cluster_align);

Can we separate out all the igeo initialsation into a single init
function rather than being spread out over several functions?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com