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@xxxxxxxxxx>
>> ---
>>
>> 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;
>>
>> + /*
>> + * 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)) {
>
> 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?"
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.
-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
>>
>
