Re: [PATCH] mkfs.xfs: introduce sunit/swidth validation helper

[Gao Xiang <hsiangkao@redhat.com>]

Hi Darrick,

On Mon, Aug 03, 2020 at 08:26:09AM -0700, Darrick J. Wong wrote:
> On Mon, Aug 03, 2020 at 08:50:18PM +0800, Gao Xiang wrote:
> > Currently stripe unit/width checking logic is all over xfsprogs.
> > So, refactor the same code snippet into a single validation helper
> > xfs_validate_stripe_factors(), including:
> >  - integer overflows of either value
> >  - sunit and swidth alignment wrt sector size
> >  - if either sunit or swidth are zero, both should be zero
> >  - swidth must be a multiple of sunit
> > 
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > ---
> > 
> > This patch follows Darrick's original suggestion [1], yet I'm
> > not sure if I'm doing the right thing or if something is still
> > missing (e.g the meaning of six(ish) places)... So post it
> > right now...
> > 
> > TBH, especially all these naming and the helper location (whether
> > in topology.c)...plus, click a dislike on calc_stripe_factors()
> > itself...
> > 
> > (Hopefully hear some advice about this... Thanks!)
> > 
> > [1] https://lore.kernel.org/r/20200515204802.GO6714@magnolia
> > 
> >  libfrog/topology.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++
> >  libfrog/topology.h | 15 ++++++++++++++
> >  mkfs/xfs_mkfs.c    | 48 ++++++++++++++++++++++----------------------
> >  3 files changed, 89 insertions(+), 24 deletions(-)
> > 
> > diff --git a/libfrog/topology.c b/libfrog/topology.c
> > index b1b470c9..cf56fb03 100644
> > --- a/libfrog/topology.c
> > +++ b/libfrog/topology.c
> > @@ -174,6 +174,41 @@ out:
> >  	return ret;
> >  }
> >  
> > +enum xfs_stripe_retcode
> > +xfs_validate_stripe_factors(
> 
> libfrog functions (and enums) should be prefixed with libfrog, not xfs.
> 
> LIBFROG_STRIPEVAL_{OK,SUNIT_MISALIGN, etc.}
> 
> > +	int	sectorsize,
> > +	int 	*sup,
> 
> Errant space between "int" and "*sup".

Ack. Sorry about that.

> 
> > +	int	*swp)
> 
> Strange that a validator function has out parameters...
> 
> Also, uh, .... full names, please.

see the reasons below...

> 
> 	int	*sunitp,
> 	int	*swidthp)
> 
> (I'm vaguely wondering why we use signed ints here vs. unsigned, but
> that isn't critical...)

That is because I saw many previous "sunit/swidth" usage in the codebase
by using "int" rather than "unsigned int". I don't have much tendency
of this. (either form is ok with me since signed int is also enough here.)

> 
> > +{
> > +	int sunit = *sup, swidth = *swp;
> > +
> > +	if (sectorsize) {
> > +		long long	big_swidth;
> > +
> > +		if (sunit % sectorsize)
> > +			return XFS_STRIPE_RET_SUNIT_MISALIGN;
> > +
> > +		sunit = (int)BTOBBT(sunit);
> 
> Hmm.  On input, *sup is in units of bytes, but on output it can be in
> units of 512b blocks?  That is very surprising...

Yeah, It seems a bit weird at first. But I have no better idea
how to fulfill/wrap up "- sunit and swidth alignment wrt sector
size" check from the original thread in the validator helper.

So I finally implemented the helper in a form which accepts
either:
 [1] (sectersize != 0) dsu (in bytes) / dsw (which is multiple of dsu)

Or
 [2] (sectersize == 0) dunit / dwidth (in 512b sector size)

In [1], dsu and dsw would be turned into dunit / dwidth finally...


Yeah, that's my premature thought about this tho... hope for better
idea about this :)

> 
> > +		big_swidth = (long long)sunit * swidth;
> > +
> > +		if (big_swidth > INT_MAX)
> > +			return XFS_STRIPE_RET_SWIDTH_OVERFLOW;
> > +		swidth = big_swidth;
> > +	}
> > +	if ((sunit && !swidth) || (!sunit && swidth))
> > +		return XFS_STRIPE_RET_PARTIAL_VALID;
> > +
> > +	if (sunit > swidth)
> > +		return XFS_STRIPE_RET_SUNIT_TOO_LARGE;
> > +
> > +	if (sunit && (swidth % sunit))
> > +		return XFS_STRIPE_RET_SWIDTH_MISALIGN;
> > +
> > +	*sup = sunit;
> 
> ...especially since in the !sectorsize case we don't change it at all.

Yeah...

> 
> > +	*swp = swidth;
> > +	return XFS_STRIPE_RET_OK;
> > +}
> > +
> >  static void blkid_get_topology(
> >  	const char	*device,
> >  	int		*sunit,
> > @@ -229,6 +264,21 @@ static void blkid_get_topology(
> >  	 */
> >  	*sunit = *sunit >> 9;
> >  	*swidth = *swidth >> 9;
> > +	switch (xfs_validate_stripe_factors(0, sunit, swidth)) {
> > +	case XFS_STRIPE_RET_OK:
> > +		break;
> > +	case XFS_STRIPE_RET_PARTIAL_VALID:
> > +		fprintf(stderr,
> > +_("%s: Volume reports stripe unit of %d bytes and stripe width of %d bytes, ignoring.\n"),
> > +				progname, BBTOB(*sunit), BBTOB(*swidth));
> 
> Needs a "/* fallthrough */" comment here.

Ack.

> 
> > +	default:
> 
> Why don't we warn about receiving garbage geometry that produces
> MISALIGN or OVERFLOW?

Okay, I could add them in the next version...
Yet I still suspect my broken English works... :)

> 
> > +		/*
> > +		 * if firmware is broken, just give up and set both to zero,
> > +		 * we can't trust information from this device.
> > +		 */
> > +		*sunit = 0;
> > +		*swidth = 0;
> > +	}
> >  
> >  	if (blkid_topology_get_alignment_offset(tp) != 0) {
> >  		fprintf(stderr,
> > diff --git a/libfrog/topology.h b/libfrog/topology.h
> > index 6fde868a..e8be26b2 100644
> > --- a/libfrog/topology.h
> > +++ b/libfrog/topology.h
> > @@ -36,4 +36,19 @@ extern int
> >  check_overwrite(
> >  	const char	*device);
> >  
> > +enum xfs_stripe_retcode {
> > +	XFS_STRIPE_RET_OK = 0,
> > +	XFS_STRIPE_RET_SUNIT_MISALIGN,
> > +	XFS_STRIPE_RET_SWIDTH_OVERFLOW,
> > +	XFS_STRIPE_RET_PARTIAL_VALID,
> > +	XFS_STRIPE_RET_SUNIT_TOO_LARGE,
> > +	XFS_STRIPE_RET_SWIDTH_MISALIGN,
> > +};
> > +
> > +enum xfs_stripe_retcode
> > +xfs_validate_stripe_factors(
> > +	int	sectorsize,
> > +	int 	*sup,
> 
> Errant space between "int" and "*sup".

Sorry, copy again.

> 
> > +	int	*swp);
> > +
> >  #endif	/* __LIBFROG_TOPOLOGY_H__ */
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 2e6cd280..a3d6032c 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -2255,7 +2255,6 @@ calc_stripe_factors(
> >  	struct cli_params	*cli,
> >  	struct fs_topology	*ft)
> >  {
> > -	long long int	big_dswidth;
> >  	int		dsunit = 0;
> >  	int		dswidth = 0;
> >  	int		lsunit = 0;
> > @@ -2263,6 +2262,7 @@ calc_stripe_factors(
> >  	int		dsw = 0;
> >  	int		lsu = 0;
> >  	bool		use_dev = false;
> > +	int		error;
> >  
> >  	if (cli_opt_set(&dopts, D_SUNIT))
> >  		dsunit = cli->dsunit;
> > @@ -2289,31 +2289,40 @@ _("both data su and data sw options must be specified\n"));
> >  			usage();
> >  		}
> >  
> > -		if (dsu % cfg->sectorsize) {
> > +		dsunit = dsu;
> > +		dswidth = dsw;
> > +		error = xfs_validate_stripe_factors(cfg->sectorsize, &dsunit, &dswidth);
> 
> I thought this function returned an enum?

okay, will update in the next version.

> 
> > +		switch(error) {
> > +		case XFS_STRIPE_RET_SUNIT_MISALIGN:
> >  			fprintf(stderr,
> >  _("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
> >  			usage();
> > -		}
> > -
> > -		dsunit  = (int)BTOBBT(dsu);
> > -		big_dswidth = (long long int)dsunit * dsw;
> > -		if (big_dswidth > INT_MAX) {
> > +			break;
> > +		case XFS_STRIPE_RET_SWIDTH_OVERFLOW:
> >  			fprintf(stderr,
> > -_("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"),
> > -				big_dswidth, dsunit);
> > +_("data stripe width (dsw %d) is too large of a multiple of the data stripe unit (%d)\n"),
> 
> Why change this message?

big_dswidth isn't defined here.
So I'm not sure if the original message can still be properly used here.
(I could leave it alone...)

> 
> > +				dsw, dsunit);
> >  			usage();
> > +			break;
> >  		}
> > -		dswidth = big_dswidth;
> > +	} else {
> > +		error = xfs_validate_stripe_factors(0, &dsunit, &dswidth);
> >  	}
> >  
> > -	if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
> > -	    (dsunit && (dswidth % dsunit != 0))) {
> > +	if (error == XFS_STRIPE_RET_PARTIAL_VALID ||
> > +	    error == XFS_STRIPE_RET_SWIDTH_MISALIGN) {
> >  		fprintf(stderr,
> >  _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
> >  			dswidth, dsunit);
> >  		usage();
> >  	}
> >  
> > +	if (error) {
> > +		fprintf(stderr,
> > +_("invalid data stripe unit (%d), width (%d)\n"), dsunit, dswidth);
> 
> Invalid how?  We know the exact reason, so we should say so.

Okay, let me think more about some cleaner way for these message.
I feel it could be a bit messy here.

Thanks，
Gao Xiang

> 
> --D
> 
> > +		usage();
> > +	}
> > +
> >  	/* If sunit & swidth were manually specified as 0, same as noalign */
> >  	if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) &&
> >  	    !dsunit && !dswidth)
> > @@ -2328,18 +2337,9 @@ _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
> >  
> >  	/* if no stripe config set, use the device default */
> >  	if (!dsunit) {
> > -		/* Ignore nonsense from device.  XXX add more validation */
> > -		if (ft->dsunit && ft->dswidth == 0) {
> > -			fprintf(stderr,
> > -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"),
> > -				progname, BBTOB(ft->dsunit));
> > -			ft->dsunit = 0;
> > -			ft->dswidth = 0;
> > -		} else {
> > -			dsunit = ft->dsunit;
> > -			dswidth = ft->dswidth;
> > -			use_dev = true;
> > -		}
> > +		dsunit = ft->dsunit;
> > +		dswidth = ft->dswidth;
> > +		use_dev = true;
> >  	} else {
> >  		/* check and warn if user-specified alignment is sub-optimal */
> >  		if (ft->dsunit && ft->dsunit != dsunit) {
> > -- 
> > 2.18.1
> > 
>