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