diff mbox series

[2/5] xfs: refactor inode geometry setup routines

Message ID 155960227220.1194435.7625646115020669657.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: refactor inode geometry | expand

Commit Message

Darrick J. Wong June 3, 2019, 10:51 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Migrate all of the inode geometry setup code from xfs_mount.c into a
single libxfs function that we can share with xfsprogs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_ialloc.c |   90 +++++++++++++++++++++++++++++++++++++-------
 fs/xfs/libxfs/xfs_ialloc.h |    8 ----
 fs/xfs/xfs_mount.c         |   83 -----------------------------------------
 3 files changed, 78 insertions(+), 103 deletions(-)

Comments

Dave Chinner June 4, 2019, 12:17 a.m. UTC | #1 
On Mon, Jun 03, 2019 at 03:51:12PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Migrate all of the inode geometry setup code from xfs_mount.c into a
> single libxfs function that we can share with xfsprogs.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_ialloc.c |   90 +++++++++++++++++++++++++++++++++++++-------
>  fs/xfs/libxfs/xfs_ialloc.h |    8 ----
>  fs/xfs/xfs_mount.c         |   83 -----------------------------------------
>  3 files changed, 78 insertions(+), 103 deletions(-)

I probably would have moved it to libxfs/xfs_inode_buf.c and
named it xfs_inode_setup_geometry(), but moving it here has some
advantages so I'm happy to leave it here. :)

> 
> - * Compute and fill in value of m_ino_geo.inobt_maxlevels.
> - */
> -void
> -xfs_ialloc_compute_maxlevels(
> -	xfs_mount_t	*mp)		/* file system mount structure */
> -{
> -	uint		inodes;
> -
> -	inodes = (1LL << XFS_INO_AGINO_BITS(mp)) >> XFS_INODES_PER_CHUNK_LOG;
> -	M_IGEO(mp)->inobt_maxlevels = xfs_btree_compute_maxlevels(
> -			M_IGEO(mp)->inobt_mnr, inodes);
> -}
> -
>  /*
>   * Log specified fields for the ag hdr (inode section). The growth of the agi
>   * structure over time requires that we interpret the buffer as two logical
> @@ -2773,3 +2759,79 @@ xfs_ialloc_count_inodes(
>  	*freecount = ci.freecount;
>  	return 0;
>  }
> +
> +/*
> + * Initialize inode-related geometry information.
> + *
> + * Compute the inode btree min and max levels and set maxicount.
> + *
> + * Set the inode cluster size.  This may still be overridden by the file
> + * system block size if it is larger than the chosen cluster size.
> + *
> + * For v5 filesystems, scale the cluster size with the inode size to keep a
> + * constant ratio of inode per cluster buffer, but only if mkfs has set the
> + * inode alignment value appropriately for larger cluster sizes.
> + *
> + * Then compute the inode cluster alignment information.
> + */
> +void
> +xfs_ialloc_setup_geometry(
> +	struct xfs_mount	*mp)
> +{
> +	struct xfs_sb		*sbp = &mp->m_sb;
> +	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
> +	uint64_t		icount;
> +	uint			inodes;
> +
> +	/* Compute and fill in value of m_ino_geo.inobt_maxlevels. */
> +	inodes = (1LL << XFS_INO_AGINO_BITS(mp)) >> XFS_INODES_PER_CHUNK_LOG;
> +	igeo->inobt_maxlevels = xfs_btree_compute_maxlevels(igeo->inobt_mnr,
> +			inodes);

Hmmm - any reason why you didn't move the inobt_mnr/mxr
initalisation here as well?

> +
> +	/* Set the maximum inode count for this filesystem. */
> +	if (sbp->sb_imax_pct) {
> +		/*
> +		 * Make sure the maximum inode count is a multiple
> +		 * of the units we allocate inodes in.
> +		 */
> +		icount = sbp->sb_dblocks * sbp->sb_imax_pct;
> +		do_div(icount, 100);
> +		do_div(icount, igeo->ialloc_blks);
> +		igeo->maxicount = XFS_FSB_TO_INO(mp,
> +				icount * igeo->ialloc_blks);
> +	} else {
> +		igeo->maxicount = 0;
> +	}
> +
> +	igeo->inode_cluster_size = XFS_INODE_BIG_CLUSTER_SIZE;
> +	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> +		int	new_size = igeo->inode_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))
> +			igeo->inode_cluster_size = new_size;
> +	}
> +	igeo->blocks_per_cluster = xfs_icluster_size_fsb(mp);
> +	igeo->inodes_per_cluster = XFS_FSB_TO_INO(mp,
> +			igeo->blocks_per_cluster);
> +	igeo->cluster_align = xfs_ialloc_cluster_alignment(mp);

I'll comment on xfs_icluster_size_fsb() and
xfs_ialloc_cluster_alignment() here knowing that you make them
private/static in the next patch: I'd actually remove them and open
code them here. xfs_icluster_size_fsb() is only called from this
function and xfs_ialloc_cluster_alignment(), and
xfs_ialloc_cluster_alignment() is only called from here.

Given that they are both very short functions, I'd just open code
them directly here and get rid of them completely like you have with
xfs_ialloc_compute_maxlevels(). That way everyone is kinda forced to
use the pre-calculated geometry rather than trying to do it
themselves and maybe get it wrong...

Otherwise than that, this looks good....

-Dave.
Darrick J. Wong June 4, 2019, 12:56 a.m. UTC | #2 
On Tue, Jun 04, 2019 at 10:17:43AM +1000, Dave Chinner wrote:
> On Mon, Jun 03, 2019 at 03:51:12PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Migrate all of the inode geometry setup code from xfs_mount.c into a
> > single libxfs function that we can share with xfsprogs.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_ialloc.c |   90 +++++++++++++++++++++++++++++++++++++-------
> >  fs/xfs/libxfs/xfs_ialloc.h |    8 ----
> >  fs/xfs/xfs_mount.c         |   83 -----------------------------------------
> >  3 files changed, 78 insertions(+), 103 deletions(-)
> 
> I probably would have moved it to libxfs/xfs_inode_buf.c and
> named it xfs_inode_setup_geometry(), but moving it here has some
> advantages so I'm happy to leave it here. :)

<nod>

> > 
> > - * Compute and fill in value of m_ino_geo.inobt_maxlevels.
> > - */
> > -void
> > -xfs_ialloc_compute_maxlevels(
> > -	xfs_mount_t	*mp)		/* file system mount structure */
> > -{
> > -	uint		inodes;
> > -
> > -	inodes = (1LL << XFS_INO_AGINO_BITS(mp)) >> XFS_INODES_PER_CHUNK_LOG;
> > -	M_IGEO(mp)->inobt_maxlevels = xfs_btree_compute_maxlevels(
> > -			M_IGEO(mp)->inobt_mnr, inodes);
> > -}
> > -
> >  /*
> >   * Log specified fields for the ag hdr (inode section). The growth of the agi
> >   * structure over time requires that we interpret the buffer as two logical
> > @@ -2773,3 +2759,79 @@ xfs_ialloc_count_inodes(
> >  	*freecount = ci.freecount;
> >  	return 0;
> >  }
> > +
> > +/*
> > + * Initialize inode-related geometry information.
> > + *
> > + * Compute the inode btree min and max levels and set maxicount.
> > + *
> > + * Set the inode cluster size.  This may still be overridden by the file
> > + * system block size if it is larger than the chosen cluster size.
> > + *
> > + * For v5 filesystems, scale the cluster size with the inode size to keep a
> > + * constant ratio of inode per cluster buffer, but only if mkfs has set the
> > + * inode alignment value appropriately for larger cluster sizes.
> > + *
> > + * Then compute the inode cluster alignment information.
> > + */
> > +void
> > +xfs_ialloc_setup_geometry(
> > +	struct xfs_mount	*mp)
> > +{
> > +	struct xfs_sb		*sbp = &mp->m_sb;
> > +	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
> > +	uint64_t		icount;
> > +	uint			inodes;
> > +
> > +	/* Compute and fill in value of m_ino_geo.inobt_maxlevels. */
> > +	inodes = (1LL << XFS_INO_AGINO_BITS(mp)) >> XFS_INODES_PER_CHUNK_LOG;
> > +	igeo->inobt_maxlevels = xfs_btree_compute_maxlevels(igeo->inobt_mnr,
> > +			inodes);
> 
> Hmmm - any reason why you didn't move the inobt_mnr/mxr
> initalisation here as well?

Oops, I'll move that too.

> 
> > +
> > +	/* Set the maximum inode count for this filesystem. */
> > +	if (sbp->sb_imax_pct) {
> > +		/*
> > +		 * Make sure the maximum inode count is a multiple
> > +		 * of the units we allocate inodes in.
> > +		 */
> > +		icount = sbp->sb_dblocks * sbp->sb_imax_pct;
> > +		do_div(icount, 100);
> > +		do_div(icount, igeo->ialloc_blks);
> > +		igeo->maxicount = XFS_FSB_TO_INO(mp,
> > +				icount * igeo->ialloc_blks);
> > +	} else {
> > +		igeo->maxicount = 0;
> > +	}
> > +
> > +	igeo->inode_cluster_size = XFS_INODE_BIG_CLUSTER_SIZE;
> > +	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > +		int	new_size = igeo->inode_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))
> > +			igeo->inode_cluster_size = new_size;
> > +	}
> > +	igeo->blocks_per_cluster = xfs_icluster_size_fsb(mp);
> > +	igeo->inodes_per_cluster = XFS_FSB_TO_INO(mp,
> > +			igeo->blocks_per_cluster);
> > +	igeo->cluster_align = xfs_ialloc_cluster_alignment(mp);
> 
> I'll comment on xfs_icluster_size_fsb() and
> xfs_ialloc_cluster_alignment() here knowing that you make them
> private/static in the next patch: I'd actually remove them and open
> code them here. xfs_icluster_size_fsb() is only called from this
> function and xfs_ialloc_cluster_alignment(), and
> xfs_ialloc_cluster_alignment() is only called from here.
> 
> Given that they are both very short functions, I'd just open code
> them directly here and get rid of them completely like you have with
> xfs_ialloc_compute_maxlevels(). That way everyone is kinda forced to
> use the pre-calculated geometry rather than trying to do it
> themselves and maybe get it wrong...

Ok.

--D

> Otherwise than that, this looks good....
> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 49f556cf244b..81d33ba10619 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -2413,20 +2413,6 @@  xfs_imap(
 	return 0;
 }
 
-/*
- * Compute and fill in value of m_ino_geo.inobt_maxlevels.
- */
-void
-xfs_ialloc_compute_maxlevels(
-	xfs_mount_t	*mp)		/* file system mount structure */
-{
-	uint		inodes;
-
-	inodes = (1LL << XFS_INO_AGINO_BITS(mp)) >> XFS_INODES_PER_CHUNK_LOG;
-	M_IGEO(mp)->inobt_maxlevels = xfs_btree_compute_maxlevels(
-			M_IGEO(mp)->inobt_mnr, inodes);
-}
-
 /*
  * Log specified fields for the ag hdr (inode section). The growth of the agi
  * structure over time requires that we interpret the buffer as two logical
@@ -2773,3 +2759,79 @@  xfs_ialloc_count_inodes(
 	*freecount = ci.freecount;
 	return 0;
 }
+
+/*
+ * Initialize inode-related geometry information.
+ *
+ * Compute the inode btree min and max levels and set maxicount.
+ *
+ * Set the inode cluster size.  This may still be overridden by the file
+ * system block size if it is larger than the chosen cluster size.
+ *
+ * For v5 filesystems, scale the cluster size with the inode size to keep a
+ * constant ratio of inode per cluster buffer, but only if mkfs has set the
+ * inode alignment value appropriately for larger cluster sizes.
+ *
+ * Then compute the inode cluster alignment information.
+ */
+void
+xfs_ialloc_setup_geometry(
+	struct xfs_mount	*mp)
+{
+	struct xfs_sb		*sbp = &mp->m_sb;
+	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
+	uint64_t		icount;
+	uint			inodes;
+
+	/* Compute and fill in value of m_ino_geo.inobt_maxlevels. */
+	inodes = (1LL << XFS_INO_AGINO_BITS(mp)) >> XFS_INODES_PER_CHUNK_LOG;
+	igeo->inobt_maxlevels = xfs_btree_compute_maxlevels(igeo->inobt_mnr,
+			inodes);
+
+	/* Set the maximum inode count for this filesystem. */
+	if (sbp->sb_imax_pct) {
+		/*
+		 * Make sure the maximum inode count is a multiple
+		 * of the units we allocate inodes in.
+		 */
+		icount = sbp->sb_dblocks * sbp->sb_imax_pct;
+		do_div(icount, 100);
+		do_div(icount, igeo->ialloc_blks);
+		igeo->maxicount = XFS_FSB_TO_INO(mp,
+				icount * igeo->ialloc_blks);
+	} else {
+		igeo->maxicount = 0;
+	}
+
+	igeo->inode_cluster_size = XFS_INODE_BIG_CLUSTER_SIZE;
+	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+		int	new_size = igeo->inode_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))
+			igeo->inode_cluster_size = new_size;
+	}
+	igeo->blocks_per_cluster = xfs_icluster_size_fsb(mp);
+	igeo->inodes_per_cluster = XFS_FSB_TO_INO(mp,
+			igeo->blocks_per_cluster);
+	igeo->cluster_align = xfs_ialloc_cluster_alignment(mp);
+	igeo->cluster_align_inodes = XFS_FSB_TO_INO(mp,
+			igeo->cluster_align);
+
+	/* Set whether we're using inode alignment. */
+	if (xfs_sb_version_hasalign(&mp->m_sb) &&
+		mp->m_sb.sb_inoalignmt >= xfs_icluster_size_fsb(mp))
+		igeo->inoalign_mask = mp->m_sb.sb_inoalignmt - 1;
+	else
+		igeo->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 && igeo->inoalign_mask &&
+	    !(mp->m_dalign & igeo->inoalign_mask))
+		igeo->ialloc_align = mp->m_dalign;
+	else
+		igeo->ialloc_align = 0;
+}
diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
index e7d935e69b11..455f65a2f1dd 100644
--- a/fs/xfs/libxfs/xfs_ialloc.h
+++ b/fs/xfs/libxfs/xfs_ialloc.h
@@ -95,13 +95,6 @@  xfs_imap(
 	struct xfs_imap	*imap,		/* location map structure */
 	uint		flags);		/* flags for inode btree lookup */
 
-/*
- * Compute and fill in value of m_ino_geo.inobt_maxlevels.
- */
-void
-xfs_ialloc_compute_maxlevels(
-	struct xfs_mount *mp);		/* file system mount structure */
-
 /*
  * Log specified fields for the ag hdr (inode section)
  */
@@ -168,5 +161,6 @@  int xfs_inobt_insert_rec(struct xfs_btree_cur *cur, uint16_t holemask,
 		int *stat);
 
 int xfs_ialloc_cluster_alignment(struct xfs_mount *mp);
+void xfs_ialloc_setup_geometry(struct xfs_mount *mp);
 
 #endif	/* __XFS_IALLOC_H__ */
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 73e5cfc4d0ec..81d6535b24b4 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -429,32 +429,6 @@  xfs_update_alignment(xfs_mount_t *mp)
 	return 0;
 }
 
-/*
- * Set the maximum inode count for this filesystem
- */
-STATIC void
-xfs_set_maxicount(
-	struct xfs_mount	*mp)
-{
-	struct xfs_sb		*sbp = &(mp->m_sb);
-	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
-	uint64_t		icount;
-
-	if (sbp->sb_imax_pct) {
-		/*
-		 * Make sure the maximum inode count is a multiple
-		 * of the units we allocate inodes in.
-		 */
-		icount = sbp->sb_dblocks * sbp->sb_imax_pct;
-		do_div(icount, 100);
-		do_div(icount, igeo->ialloc_blks);
-		igeo->maxicount = XFS_FSB_TO_INO(mp,
-				icount * igeo->ialloc_blks);
-	} else {
-		igeo->maxicount = 0;
-	}
-}
-
 /*
  * Set the default minimum read and write sizes unless
  * already specified in a mount option.
@@ -511,29 +485,6 @@  xfs_set_low_space_thresholds(
 	}
 }
 
-
-/*
- * Set whether we're using inode alignment.
- */
-STATIC void
-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))
-		M_IGEO(mp)->inoalign_mask = mp->m_sb.sb_inoalignmt - 1;
-	else
-		M_IGEO(mp)->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 && M_IGEO(mp)->inoalign_mask &&
-	    !(mp->m_dalign & M_IGEO(mp)->inoalign_mask))
-		M_IGEO(mp)->ialloc_align = mp->m_dalign;
-	else
-		M_IGEO(mp)->ialloc_align = 0;
-}
-
 /*
  * Check that the data (and log if separate) is an ok size.
  */
@@ -752,12 +703,10 @@  xfs_mountfs(
 	xfs_alloc_compute_maxlevels(mp);
 	xfs_bmap_compute_maxlevels(mp, XFS_DATA_FORK);
 	xfs_bmap_compute_maxlevels(mp, XFS_ATTR_FORK);
-	xfs_ialloc_compute_maxlevels(mp);
+	xfs_ialloc_setup_geometry(mp);
 	xfs_rmapbt_compute_maxlevels(mp);
 	xfs_refcountbt_compute_maxlevels(mp);
 
-	xfs_set_maxicount(mp);
-
 	/* enable fail_at_unmount as default */
 	mp->m_fail_unmount = true;
 
@@ -790,31 +739,6 @@  xfs_mountfs(
 	/* set the low space thresholds for dynamic preallocation */
 	xfs_set_low_space_thresholds(mp);
 
-	/*
-	 * Set the inode cluster size.
-	 * This may still be overridden by the file system
-	 * block size if it is larger than the chosen cluster size.
-	 *
-	 * For v5 filesystems, scale the cluster size with the inode size to
-	 * keep a constant ratio of inode per cluster buffer, but only if mkfs
-	 * has set the inode alignment value appropriately for larger cluster
-	 * sizes.
-	 */
-	igeo->inode_cluster_size = XFS_INODE_BIG_CLUSTER_SIZE;
-	if (xfs_sb_version_hascrc(&mp->m_sb)) {
-		int	new_size = igeo->inode_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))
-			igeo->inode_cluster_size = new_size;
-	}
-	igeo->blocks_per_cluster = xfs_icluster_size_fsb(mp);
-	igeo->inodes_per_cluster = XFS_FSB_TO_INO(mp,
-			igeo->blocks_per_cluster);
-	igeo->cluster_align = xfs_ialloc_cluster_alignment(mp);
-	igeo->cluster_align_inodes = XFS_FSB_TO_INO(mp,
-			igeo->cluster_align);
-
 	/*
 	 * If enabled, sparse inode chunk alignment is expected to match the
 	 * cluster size. Full inode chunk alignment must match the chunk size,
@@ -831,11 +755,6 @@  xfs_mountfs(
 		goto out_remove_uuid;
 	}
 
-	/*
-	 * Set inode alignment fields
-	 */
-	xfs_set_inoalignment(mp);
-
 	/*
 	 * Check that the data (and log if separate) is an ok size.
 	 */