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