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@xxxxxxxxxx>
> > > ---
>
> ...
>
> > > 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
>
