Re: [PATCH 3/3] xfs: refactor the geometry structure filling function

["Darrick J. Wong" <darrick.wong@oracle.com>]

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@oracle.com>
> > 
> > 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@oracle.com>
> > ---
> 
> 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@vger.kernel.org
> > 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html