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

From: Eric Sandeen <sandeen@redhat.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs <linux-xfs@vger.kernel.org>
Subject: Re: [PATCH] mkfs.xfs: sanity check stripe geometry from blkid
Date: Fri, 15 May 2020 15:54:34 -0500	[thread overview]
Message-ID: <49e3d73f-1df1-e4d3-2451-db76f7084731@redhat.com> (raw)
In-Reply-To: <20200515204802.GO6714@magnolia>

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;
>>  
>> +        /*
>> +	 * 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
>>
> 