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

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

[James Bottomley]

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.

James

---

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d0a980915801..1f3a1474042e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2961,6 +2961,10 @@ static void sd_read_block_characteristics(struct scsi_disk *sdkp)
 
 	buffer = kmalloc(vpd_len, GFP_KERNEL);
 
+	/* set to rotational in case no device characteristic page exists */
+	blk_queue_flag_clear(QUEUE_FLAG_NONROT, q);
+	blk_queue_flag_set(QUEUE_FLAG_ADD_RANDOM, q);
+
 	if (!buffer ||
 	    /* Block Device Characteristics VPD */
 	    scsi_get_vpd_page(sdkp->device, 0xb1, buffer, vpd_len))
@@ -2971,9 +2975,6 @@ static void sd_read_block_characteristics(struct scsi_disk *sdkp)
 	if (rot == 1) {
 		blk_queue_flag_set(QUEUE_FLAG_NONROT, q);
 		blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, q);
-	} else {
-		blk_queue_flag_clear(QUEUE_FLAG_NONROT, q);
-		blk_queue_flag_set(QUEUE_FLAG_ADD_RANDOM, q);
 	}
 
 	if (sdkp->device->type == TYPE_ZBC) {

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

[Jens Axboe]

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.

-- 
Jens Axboe

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

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-
> owner@vger.kernel.org> On Behalf Of Jens Axboe
> Sent: Monday, February 11, 2019 8:50 PM
> To: James Bottomley <James.Bottomley@HansenPartnership.com>; Mikael
> Pettersson <mikpelinux@gmail.com>
> Cc: Linux SPARC Kernel Mailing List <sparclinux@vger.kernel.org>;
> linux-block@vger.kernel.org; linux-kernel@vger.kernel.org; linux-scsi
> <linux-scsi@vger.kernel.org>
> Subject: Re: [5.0-rc5 regression] "scsi: kill off the legacy IO path"
> causes 5 minute delay during boot on Sun Blade 2500
...

> > 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.

When blk-mq and scsi-mq were introduced to boost SAS SSDs over
1 million IOPS, this was purposely kept off, because it generated
a noticeable 3-12 us latency blip whenever the "fast_mix" code ran
out of bits. If Jens' patch changes the default to be enabled for
all scsi-mq users, that'll be a change for the newer legacy
scsi-mq users (except for users that don't trust the default and
always set the value in sysfs).

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

[James Bottomley]

On Tue, 2019-02-12 at 03:37 +0000, Elliott, Robert (Persistent Memory)
wrote:
> > -----Original Message-----
> > From: linux-kernel-owner@vger.kernel.org <linux-kernel-
> > owner@vger.kernel.org> On Behalf Of Jens Axboe
> > Sent: Monday, February 11, 2019 8:50 PM
> > To: James Bottomley <James.Bottomley@HansenPartnership.com>; Mikael
> > Pettersson <mikpelinux@gmail.com>
> > Cc: Linux SPARC Kernel Mailing List <sparclinux@vger.kernel.org>;
> > linux-block@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> > scsi
> > <linux-scsi@vger.kernel.org>
> > Subject: Re: [5.0-rc5 regression] "scsi: kill off the legacy IO
> > path"
> > causes 5 minute delay during boot on Sun Blade 2500
> 
> ...
> 
> > > 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.
> 
> When blk-mq and scsi-mq were introduced to boost SAS SSDs over
> 1 million IOPS, this was purposely kept off, because it generated
> a noticeable 3-12 us latency blip whenever the "fast_mix" code ran
> out of bits. If Jens' patch changes the default to be enabled for
> all scsi-mq users, that'll be a change for the newer legacy
> scsi-mq users (except for users that don't trust the default and
> always set the value in sysfs).

All high IOPs devices have a device characteristics page, so even if we
 default this to ON, it will get turned off for these devices which
should be the desired result regardless of the default setting. 

James

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

[James Bottomley]

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.

James

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

[Jens Axboe]

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