Re: [PATCH] mkfs: fix divide-by-zero in align_ag_geometry

[Jeff Mahoney <jeffm@suse.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2365 bytes --]

On 6/21/18 9:34 PM, Eric Sandeen wrote:
> On 6/21/18 6:16 PM, Eric Sandeen wrote:
>>> Ah, so it came from the hardware? In that case, we probably
>>> shouldn't zero sunit when blkid reports this whacky case. i.e. I
>>> think we should set swidth = sunit so that we retain allocation
>>> alignment to the minimum IO size the device specified.
>> Hrmph.
>>
>> yeah, I'd really like to see 
>>
>> # blockdev --getiomin --getioopt --getss --getpbsz
>>
>> for all the devices in the stack, I guess...
> 
> Ok, so Jeff shared the lsblk output with me; the underlying
> device and the mpath device are both:
> 
> MIN-IO OPT-IO PHY-SEC LOG-SEC
> 8192      0     512     512
> 
> so logical/physical 512, with minimum io 8192 and optimal io 0.
> On further inspection Jeff says this may be because an optimal
> size is reported as greater than the maximum size, so we get 0
> due to the inconsistency.
> 
> Ok, well, I guess I see your point that we should probably set
> sunit=swidth=8192 here.  I was thinking that conflicted with
> how we process it on the commandline but no, it doesn't; if you're
> going to manually specify you can't set one to 0 and not the other,
> but we have to make the best of what the hardware tells us.
> 
> So we could do:
> 
>         /*
>          * Some odd hardware sets minimum IO but not optimal; assume
>          * minimal is the smallest preferred IO size, and set optimal
> 	 * to the same value, i.e. a stripe of 1 disk.
>          */
>         if (*sunit && *swidth == 0)
>                 *swidth = *sunit;
> 
> But it's clearly buggy hardware.  How are we to know which values 
> are accurate and which are not?  It still may be better to just set
> to no stripe geometry if we've gotten nonsense from libblockdev,
> if the device wants better performance it needs to have rational
> firmware... trying to 2nd guess bad values is just a crapshoot.
> I guess we could issue a warning ...

Getting back to this after some time off.  I agree we should warn, but
if we warn, we need to do it when we have more context.  Otherwise we'll
warn even when the user specifies stripe parameters, which just looks
sloppy.  I have an updated patch that puts it in calc_stripe_factors and
only issues the warning if we will consume those values.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]