On Fri, Jan 05, 2018 at 08:08:35AM -0500, Brian Foster wrote:
> On Thu, Jan 04, 2018 at 07:39:51PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Refactor the geometry structure filling function to use the superblock
> > to fill the fields. While we're at it, make the function less indenty
> > and use some whitespace to make the function easier to read.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
>
> Nice cleanup...
>
> > fs/xfs/libxfs/xfs_sb.c | 167 ++++++++++++++++++++++++++++--------------------
> > fs/xfs/libxfs/xfs_sb.h | 5 +
> > fs/xfs/xfs_ioctl.c | 4 +
> > fs/xfs/xfs_ioctl32.c | 2 -
> > 4 files changed, 103 insertions(+), 75 deletions(-)
> >
> >
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index 139517a..70e5126 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -879,77 +879,104 @@ xfs_sync_sb(
> >
> > int
> > xfs_fs_geometry(
> > - xfs_mount_t *mp,
> > - xfs_fsop_geom_t *geo,
> > - int new_version)
> > + struct xfs_sb *sbp,
> > + struct xfs_fsop_geom *geo,
> > + int struct_version)
> > {
> ...
> > + geo->version = XFS_FSOP_GEOM_VERSION;
> > + geo->flags = XFS_FSOP_GEOM_FLAGS_NLINK |
> > + XFS_FSOP_GEOM_FLAGS_DIRV2;
> > +
> > +
>
> One whitespace line too many here..?
Yep, fixed.
> > + if (xfs_sb_version_hasattr(sbp))
> > + geo->flags |= XFS_FSOP_GEOM_FLAGS_ATTR;
> > +
>
> Obviously just a nit... but IMO the extra whitespace between each of
> these feature checks is a bit much. It actually reads cleaner to me to
> kill off the extra lines between them. Just my .02 though..
Yeah, I suppose since we proceed linearly through that section without
branching outside the block that it needn't the additional whitespace.
Fixed.
> ...
> > +
> > + geo->rtsectsize = sbp->sb_blocksize;
> > + geo->dirblocksize = 1 << (sbp->sb_blocklog + sbp->sb_dirblklog);
> > +
>
> Slightly unfortunate that we have the geo abstraction and can't use it
> for a simple reporting case. Is there a larger (userspace?) reason mp
> was dropped as a parameter or was that just a cleanup? If the latter,
> perhaps we could continue to pass mp and create a local sbp pointer. If
> the former, then oh well, not a big deal. :P
At the point that mkfs wants to call this function, it has a fully built
xfs_sb ready to go, but it hasn't called libxfs_init. Therefore,
mp->m_dir_geo hasn't yet been built, and I wanted to preserve the
behavior that mkfs can spit out the geometry even if libxfs
initialization fails for some other reason.
That said, there isn't any reason why I couldn't just make this a helper
in xfs_da_format.h and refactor all the opencoded instances:
/* Number of bytes in a directory block. */
static inline unsigned int xfs_dir2_dirblock_bytes(struct xfs_sb *sbp)
{
return 1 << (sbp->sb_blocklog + sbp->sb_dirblklog);
}
--D
> Brian
>
> > + if (struct_version < 3)
> > + return 0;
> > +
> > + if (xfs_sb_version_haslogv2(sbp))
> > + geo->flags |= XFS_FSOP_GEOM_FLAGS_LOGV2;
> > +
> > + geo->logsunit = sbp->sb_logsunit;
> >
> > - memset(geo, 0, sizeof(*geo));
> > -
> > - geo->blocksize = mp->m_sb.sb_blocksize;
> > - geo->rtextsize = mp->m_sb.sb_rextsize;
> > - geo->agblocks = mp->m_sb.sb_agblocks;
> > - geo->agcount = mp->m_sb.sb_agcount;
> > - geo->logblocks = mp->m_sb.sb_logblocks;
> > - geo->sectsize = mp->m_sb.sb_sectsize;
> > - geo->inodesize = mp->m_sb.sb_inodesize;
> > - geo->imaxpct = mp->m_sb.sb_imax_pct;
> > - geo->datablocks = mp->m_sb.sb_dblocks;
> > - geo->rtblocks = mp->m_sb.sb_rblocks;
> > - geo->rtextents = mp->m_sb.sb_rextents;
> > - geo->logstart = mp->m_sb.sb_logstart;
> > - ASSERT(sizeof(geo->uuid) == sizeof(mp->m_sb.sb_uuid));
> > - memcpy(geo->uuid, &mp->m_sb.sb_uuid, sizeof(mp->m_sb.sb_uuid));
> > - if (new_version >= 2) {
> > - geo->sunit = mp->m_sb.sb_unit;
> > - geo->swidth = mp->m_sb.sb_width;
> > - }
> > - if (new_version >= 3) {
> > - geo->version = XFS_FSOP_GEOM_VERSION;
> > - geo->flags = XFS_FSOP_GEOM_FLAGS_NLINK |
> > - XFS_FSOP_GEOM_FLAGS_DIRV2 |
> > - (xfs_sb_version_hasattr(&mp->m_sb) ?
> > - XFS_FSOP_GEOM_FLAGS_ATTR : 0) |
> > - (xfs_sb_version_hasquota(&mp->m_sb) ?
> > - XFS_FSOP_GEOM_FLAGS_QUOTA : 0) |
> > - (xfs_sb_version_hasalign(&mp->m_sb) ?
> > - XFS_FSOP_GEOM_FLAGS_IALIGN : 0) |
> > - (xfs_sb_version_hasdalign(&mp->m_sb) ?
> > - XFS_FSOP_GEOM_FLAGS_DALIGN : 0) |
> > - (xfs_sb_version_hasextflgbit(&mp->m_sb) ?
> > - XFS_FSOP_GEOM_FLAGS_EXTFLG : 0) |
> > - (xfs_sb_version_hassector(&mp->m_sb) ?
> > - XFS_FSOP_GEOM_FLAGS_SECTOR : 0) |
> > - (xfs_sb_version_hasasciici(&mp->m_sb) ?
> > - XFS_FSOP_GEOM_FLAGS_DIRV2CI : 0) |
> > - (xfs_sb_version_haslazysbcount(&mp->m_sb) ?
> > - XFS_FSOP_GEOM_FLAGS_LAZYSB : 0) |
> > - (xfs_sb_version_hasattr2(&mp->m_sb) ?
> > - XFS_FSOP_GEOM_FLAGS_ATTR2 : 0) |
> > - (xfs_sb_version_hasprojid32bit(&mp->m_sb) ?
> > - XFS_FSOP_GEOM_FLAGS_PROJID32 : 0) |
> > - (xfs_sb_version_hascrc(&mp->m_sb) ?
> > - XFS_FSOP_GEOM_FLAGS_V5SB : 0) |
> > - (xfs_sb_version_hasftype(&mp->m_sb) ?
> > - XFS_FSOP_GEOM_FLAGS_FTYPE : 0) |
> > - (xfs_sb_version_hasfinobt(&mp->m_sb) ?
> > - XFS_FSOP_GEOM_FLAGS_FINOBT : 0) |
> > - (xfs_sb_version_hassparseinodes(&mp->m_sb) ?
> > - XFS_FSOP_GEOM_FLAGS_SPINODES : 0) |
> > - (xfs_sb_version_hasrmapbt(&mp->m_sb) ?
> > - XFS_FSOP_GEOM_FLAGS_RMAPBT : 0) |
> > - (xfs_sb_version_hasreflink(&mp->m_sb) ?
> > - XFS_FSOP_GEOM_FLAGS_REFLINK : 0);
> > - geo->logsectsize = xfs_sb_version_hassector(&mp->m_sb) ?
> > - mp->m_sb.sb_logsectsize : BBSIZE;
> > - geo->rtsectsize = mp->m_sb.sb_blocksize;
> > - geo->dirblocksize = mp->m_dir_geo->blksize;
> > - }
> > - if (new_version >= 4) {
> > - geo->flags |=
> > - (xfs_sb_version_haslogv2(&mp->m_sb) ?
> > - XFS_FSOP_GEOM_FLAGS_LOGV2 : 0);
> > - geo->logsunit = mp->m_sb.sb_logsunit;
> > - }
> > return 0;
> > }
> > diff --git a/fs/xfs/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h
> > index a16632c..63dcd2a 100644
> > --- a/fs/xfs/libxfs/xfs_sb.h
> > +++ b/fs/xfs/libxfs/xfs_sb.h
> > @@ -34,7 +34,8 @@ extern void xfs_sb_from_disk(struct xfs_sb *to, struct xfs_dsb *from);
> > extern void xfs_sb_to_disk(struct xfs_dsb *to, struct xfs_sb *from);
> > extern void xfs_sb_quota_from_disk(struct xfs_sb *sbp);
> >
> > -extern int xfs_fs_geometry(struct xfs_mount *mp, struct xfs_fsop_geom *geo,
> > - int nversion);
> > +#define XFS_FS_GEOM_MAX_STRUCT_VER (4)
> > +extern int xfs_fs_geometry(struct xfs_sb *sbp, struct xfs_fsop_geom *geo,
> > + int struct_version);
> >
> > #endif /* __XFS_SB_H__ */
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 3015e17..89fb1eb 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -810,7 +810,7 @@ xfs_ioc_fsgeometry_v1(
> > xfs_fsop_geom_t fsgeo;
> > int error;
> >
> > - error = xfs_fs_geometry(mp, &fsgeo, 3);
> > + error = xfs_fs_geometry(&mp->m_sb, &fsgeo, 3);
> > if (error)
> > return error;
> >
> > @@ -832,7 +832,7 @@ xfs_ioc_fsgeometry(
> > xfs_fsop_geom_t fsgeo;
> > int error;
> >
> > - error = xfs_fs_geometry(mp, &fsgeo, 4);
> > + error = xfs_fs_geometry(&mp->m_sb, &fsgeo, 4);
> > if (error)
> > return error;
> >
> > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> > index 66cc3cd..10fbde3 100644
> > --- a/fs/xfs/xfs_ioctl32.c
> > +++ b/fs/xfs/xfs_ioctl32.c
> > @@ -67,7 +67,7 @@ xfs_compat_ioc_fsgeometry_v1(
> > xfs_fsop_geom_t fsgeo;
> > int error;
> >
> > - error = xfs_fs_geometry(mp, &fsgeo, 3);
> > + error = xfs_fs_geometry(&mp->m_sb, &fsgeo, 3);
> > if (error)
> > return error;
> > /* The 32-bit variant simply has some padding at the end */
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
