Re: [PATCH] xfs: introduce xfs_validate_stripe_factors()

[Brian Foster <bfoster@redhat.com>]

On Mon, Oct 12, 2020 at 09:55:36PM +0800, Gao Xiang wrote:
> 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...
> 

Oh, Ok. No problem, I don't feel strongly about it. It just looked like
a potential code reduction.

> > 
> > > +
> > > +	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.
> 

git (at least my configuration) tends to show this as a whitespace
error. I.e., it's highlighted in red and stands out similar to other
whitespace errors (such as tab after space, etc.). I thought that was a
fairly common config and thus something we tried to avoid, but could be
mistaken.

Brian

> Thanks,
> Gao Xiang
> 
> > 
> > Otherwise looks reasonable outside of those nits.
> > 
> > Brian
>