Re: [PATCH] xfs: introduce xfs_validate_stripe_factors()

[Gao Xiang <hsiangkao@redhat.com>]

Hi Brian,

On Mon, Oct 12, 2020 at 09:05:24AM -0400, Brian Foster wrote:
> On Fri, Oct 09, 2020 at 01:05:46PM +0800, Gao Xiang wrote:
> > Introduce a common helper to consolidate stripe validation process.
> > Also make kernel code xfs_validate_sb_common() use it first.
> > 
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > ---

...

> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index 5aeafa59ed27..cb2a7aa0ad51 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> ...
> > @@ -1233,3 +1230,49 @@ xfs_sb_get_secondary(
> >  	*bpp = bp;
> >  	return 0;
> >  }
> > +
> > +/*
> > + * sunit, swidth, sectorsize(optional with 0) should be all in bytes,
> > + * so users won't be confused by values in error messages.
> > + */
> > +bool
> > +xfs_validate_stripe_factors(
> 
> xfs_validate_stripe_geometry() perhaps?

Thanks for the review!

Ok, I'm fine with the naming, since I had no better name
about it at that time :)

> 
> > +	struct xfs_mount	*mp,
> > +	__s64			sunit,
> > +	__s64			swidth,
> > +	int			sectorsize)
> > +{
> > +	if (sectorsize && sunit % sectorsize) {
> > +		xfs_notice(mp,
> > +"stripe unit (%lld) must be a multiple of the sector size (%d)",
> > +			   sunit, sectorsize);
> > +		return false;
> > +	}
> > +
> > +	if (sunit && !swidth) {
> > +		xfs_notice(mp,
> > +"invalid stripe unit (%lld) and stripe width of 0", sunit);
> > +		return false;
> > +	}
> > +
> > +	if (!sunit && swidth) {
> > +		xfs_notice(mp,
> > +"invalid stripe width (%lld) and stripe unit of 0", swidth);
> > +		return false;
> > +	}
> 
> Seems like these two could be combined into one check that prints
> something like:
> 
> 	invalid stripe width (%lld) and stripe unit (%lld)

Hmm, that was in response to Darrick's previous review... see,
https://lore.kernel.org/linux-xfs/20201007222942.GH6540@magnolia

so I'd like to know further direction of this...

> 
> > +
> > +	if (sunit > swidth) {
> > +		xfs_notice(mp,
> > +"stripe unit (%lld) is larger than the stripe width (%lld)", sunit, swidth);
> > +		return false;
> > +	}
> > +
> > +	if (sunit && (swidth % sunit)) {
> > +		xfs_notice(mp,
> > +"stripe width (%lld) must be a multiple of the stripe unit (%lld)",
> > +			   swidth, sunit);
> > +		return false;
> > +	}
> > +	return true;
> > +}
> > +
> 
> Trailing whitespace here.

That is trailing newline (I personally prefer that),
yeah, I will remove it in the next version.

Thanks,
Gao Xiang

> 
> Otherwise looks reasonable outside of those nits.
> 
> Brian