Re: [PATCH] mkfs.xfs: sanity check stripe geometry from blkid

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, May 15, 2020 at 03:54:34PM -0500, Eric Sandeen wrote:
> On 5/15/20 3:48 PM, Darrick J. Wong wrote:
> > On Fri, May 15, 2020 at 02:14:17PM -0500, Eric Sandeen wrote:
> >> We validate commandline options for stripe unit and stripe width, and
> >> if a device returns nonsensical values via libblkid, the superbock write
> >> verifier will eventually catch it and fail (noisily and cryptically) but
> >> it seems a bit cleaner to just do a basic sanity check on the numbers
> >> as soon as we get them from blkid, and if they're bogus, ignore them from
> >> the start.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>
> >> diff --git a/libfrog/topology.c b/libfrog/topology.c
> >> index b1b470c9..38ed03b7 100644
> >> --- a/libfrog/topology.c
> >> +++ b/libfrog/topology.c
> >> @@ -213,6 +213,19 @@ static void blkid_get_topology(
> >>  	val = blkid_topology_get_optimal_io_size(tp);
> >>  	*swidth = val;
> >>  
> >> +        /*

Tabs not spaces

> >> +	 * Occasionally, firmware is broken and returns optimal < minimum,
> >> +	 * or optimal which is not a multiple of minimum.
> >> +	 * In that case just give up and set both to zero, we can't trust
> >> +	 * information from this device. Similar to xfs_validate_sb_common().
> >> +	 */
> >> +        if (*sunit) {
> >> +                if ((*sunit > *swidth) || (*swidth % *sunit != 0)) {

Why not combine these?

if (*sunit != 0 && (*sunit > *swidth || *swidth % *sunit != 0)) {

Aside from that the code looks fine I guess...

> > I feel like we're copypasting this sunit/swidth checking logic all over
> > xfsprogs 
> 
> That's because we are!
> 
> > and yet we're still losing the stripe unit validation whackamole
> > game.
> 
> Need moar hammers!
> 
> > In the end, we want to check more or less the same things for each pair
> > of stripe unit and stripe width:
> > 
> >  * 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
> > 
> > All four of these rules apply to the blkid_get_toplogy answers for the
> > data device, the log device, and the realtime device; and any mkfs CLI
> > overrides of those values.
> > 
> > IOWs, is there some way to refactor those four rules into a single
> > validation function and call that in the six(ish) places we need it?
> > Especially since you're the one who played the last round of whackamole,
> > back in May 2018. :)
> 
> So .... I would like to do that refactoring.  I'd also like to fix this
> with some expediency, TBH...
> 
> Refactoring is going to be a little more complicated, I fear, because sanity
> on "what came straight from blkid" is a little different from "what came from
> cmdline" and has slightly different checks than "how does it fit into the
> superblock we just read?"

Admittedly I wondered if "refactor all these checks" would fall apart
because each tool has its own slightly different reporting and logging
requirements.  You could make a checker function return an enum of what
it's mad about and each caller could either have a message catalogue or
just bail depending on the circumstances, but now I've probably
overengineered the corner case catching code.

> This (swidth-vs-sunit-is-borken) is common enough that I wanted to just kill
> it with fire, and um ... make it all better/cohesive at some later date.
> 
> I don't like arguing for expediency over beauty but well... here I am.

:(

--D

> -Eric
> 
> > --D
> > 
> >> +                        *sunit = 0;
> >> +                        *swidth = 0;
> >> +                }
> >> +        }
> >> +
> >>  	/*
> >>  	 * If the reported values are the same as the physical sector size
> >>  	 * do not bother to report anything.  It will only cause warnings
> >>
> > 
>