Re: [5.0-rc5 regression] "scsi: kill off the legacy IO path" causes 5 minute delay during boot on Sun Blade 2500

[Jens Axboe <axboe@kernel.dk>]

On 2/12/19 8:24 AM, James Bottomley wrote:
> On Mon, 2019-02-11 at 19:50 -0700, Jens Axboe wrote:
>> On 2/11/19 7:13 PM, James Bottomley wrote:
>>> On Mon, 2019-02-11 at 09:31 -0700, Jens Axboe wrote:
>>>> On 2/11/19 9:28 AM, James Bottomley wrote:
>>>>> On Mon, 2019-02-11 at 08:46 -0700, Jens Axboe wrote:
>>>>>> On 2/11/19 8:42 AM, James Bottomley wrote:
>>>>>>> On Mon, 2019-02-11 at 08:28 -0700, Jens Axboe wrote:
>>>>>>>> On 2/11/19 8:25 AM, James Bottomley wrote:
>>>>>>>>> On Sun, 2019-02-10 at 09:35 -0700, Jens Axboe wrote:
>>>>>>>>>> On 2/10/19 9:25 AM, James Bottomley wrote:
>>>>>
>>>>> [...]
>>>>>>>>>>> That check wasn't changed by the code removal.
>>>>>>>>>>
>>>>>>>>>> As I said above, for sd. This isn't true for non-
>>>>>>>>>> disks.
>>>>>>>>>
>>>>>>>>> Yes, but the behaviour above doesn't change across a
>>>>>>>>> switch
>>>>>>>>> to MQ, so I don't quite understand how it bisects back
>>>>>>>>> to
>>>>>>>>> that change.  If we're not gathering entropy for the
>>>>>>>>> device
>>>>>>>>> now, we wouldn't have been before the switch, so the
>>>>>>>>> entropy characteristics shouldn't have changed.
>>>>>>>>
>>>>>>>> But it does, as I also wrote in that first email. The
>>>>>>>> legacy
>>>>>>>> queue flags had QUEUE_FLAG_ADD_RANDOM set by default, the
>>>>>>>> MQ
>>>>>>>> ones do not. Hence any non-sd device would previously
>>>>>>>> ALWAYS
>>>>>>>> have ADD_RANDOM set, now none of them do. Also see the
>>>>>>>> patch
>>>>>>>> I sent.
>>>>>>>
>>>>>>> So your theory is that the disk in question never gets to
>>>>>>> the
>>>>>>> rotational check?  because the check will clear the flag if
>>>>>>> it's non-rotational and set it if it's not, so the default
>>>>>>> state of the flag shouldn't matter.
>>>>>>
>>>>>> No, my point is about non-disks, devices that aren't driven
>>>>>> by
>>>>>> sd. The behavior for sd hasn't changed, as it sets/clears it
>>>>>> unconditionally. 
>>>>>
>>>>> I agree, but I don't think any of them were significant entropy
>>>>> contributors before: things like nvme have always been outside
>>>>> of
>>>>> this and sr and st don't really contribute much to the seek
>>>>> load
>>>>> during boot because they're probed but not used by the boot
>>>>> sequence, so I can't see how they would cause this
>>>>> behaviour.  I
>>>>> suppose it could be target probing, but even that seems
>>>>> unlikely
>>>>> because it should be dwarfed by the number of root disk reads
>>>>> during boot.
>>>>>
>>>>> For the rng to take an additional 5 minutes to initialize, we
>>>>> must
>>>>> have lost a significant entropy source somewhere.
>>>>
>>>> I agree it's not a significant amount of entropy, but even just
>>>> one
>>>> bit could mean a long stall if that put us over the edge of just
>>>> not
>>>> having enough for whatever is blocking on /dev/random. Mikael's
>>>> boot
>>>> did have a CDROM, it's not impossible that the handful of
>>>> commands we
>>>> end up doing to that device would have contributed enough entropy
>>>> to
>>>> get the boot done without stalling for minutes.
>>>>
>>>> One way to know for sure, and that's if Mikael tests the patch.
>>>
>>> I think I've got the root cause.  I have one system in my test bed
>>> exhibiting this behaviour.  It turns out the disk in it has no
>>> characteristics VPD page.  The 0xB1 VPD was a SBC-3 addition, so
>>> that's
>>> not surprising.  However, the characteristics check bails before
>>> setting the flags, so it takes the default flag which has flipped.
>>>
>>> We can either fix this by setting the QUEUE_FLAG_ADD_RANDOM if
>>> there's
>>> no 0xB1 page or by setting the default as Jens proposed.
>>
>> I'd recommend just doing my patch, since that'll be the same behavior
>> that SCSI had before.
> 
> I've got the history now, it's this patch
> 
> Author: Xuewei Zhang <xueweiz@google.com>
> Date:   Thu Sep 6 13:37:19 2018 -0700
> 
>     scsi: sd: Contribute to randomness when running rotational device
> 
> It added the else branch to the if (rot == 1).  It's the position of
> that else branch which is wrong because not all disks have a SBC-3
> characteristics VPD page, so they're the ones under MQ which stop
> contributing entropy.  Whichever patch we go with will need a fixes:
> for this.

Ah, makes sense. I'd say we're _probably_ fine just fixing that then,
or at least it should be two separate patches.

-- 
Jens Axboe