| Message ID | 20180906203719.209399-1-xueweiz@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | scsi: sd: Contribute to randomness when running rotational device | expand |
On Thu, 2018-09-06 at 13:37 -0700, Xuewei Zhang wrote: > Currently a scsi device won't contribute to kernel randomness when it > uses blk-mq. Since we commonly use scsi on rotational device with > blk-mq, it make sense to keep contributing to kernel randomness in these > cases. This is especially important for virtual machines. > > commit b5b6e8c8d3b4 ("scsi: virtio_scsi: fix IO hang caused by automatic > irq vector affinity") made all virtio-scsi device to use blk-mq, which > does not contribute to randomness today. So for a virtual machine only > having virtio-scsi disk (which is common), it will simple stop getting > randomness from its disks in today's implementation. > > With this patch, if the above VM has rotational virtio-scsi device, then > it can still benefit from the entropy generated from the disk. > > Reported-by: Xuewei Zhang <xueweiz@google.com> > Signed-off-by: Xuewei Zhang <xueweiz@google.com> > --- > drivers/scsi/sd.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index b79b366a94f7..5e4f10d28065 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -2959,6 +2959,9 @@ 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) { Although this patch looks fine to me, seeing this patch makes me wonder whether the default should be changed (QUEUE_FLAG_MQ_DEFAULT) instead of modifying the sd driver. Can anyone remind me why QUEUE_FLAG_MQ_DEFAULT does not include QUEUE_FLAG_ADD_RANDOM? Thanks, Bart.
On Thu, Sep 06, 2018 at 03:27:53PM -0700, Bart Van Assche wrote: > > Although this patch looks fine to me, seeing this patch makes me wonder > whether the default should be changed (QUEUE_FLAG_MQ_DEFAULT) instead of > modifying the sd driver. Can anyone remind me why QUEUE_FLAG_MQ_DEFAULT does > not include QUEUE_FLAG_ADD_RANDOM? There was a discussion about a number of *years* ago; blk-mq has been baking for a very long time. In the early days of block_mq, the overwhelming percentage of the users of blk-mq where those who were using PCIe attached flash. So when, I raised this question, the argument was that SSD users have no entropy. Which I agree with; but now that blk-mq is the default, and hard drives are using blk-mq, it's time for a patch like Xuewei's. Cheers, - Ted
On Thu, Sep 6, 2018 at 3:42 PM Theodore Y. Ts'o <tytso@mit.edu> wrote: > > On Thu, Sep 06, 2018 at 03:27:53PM -0700, Bart Van Assche wrote: > > > > Although this patch looks fine to me, seeing this patch makes me wonder > > whether the default should be changed (QUEUE_FLAG_MQ_DEFAULT) instead of > > modifying the sd driver. Can anyone remind me why QUEUE_FLAG_MQ_DEFAULT does > > not include QUEUE_FLAG_ADD_RANDOM? Besides Ted's point of "SSD users have no entropy", I think there are two more reasons: 1. setting QUEUE_FLAG_ADD_RANDOM has a more visible performance hit on SSD disks than rotational disks. 2. SSD disks provide less entropy than rotational disks. I actually experimented on Container-Optimized OS, running on Google Compute Engine with QUEUE_FLAG_ADD_RANDOM set. Turns out the VM will have ~800 bit of entropy provided on boot on rotational disk; and will only have ~70 bit of entropy if running on SSD (and remember there are ~50 bit contributed from other sources). (in the above experiment, both disks were virtualized disks) > > There was a discussion about a number of *years* ago; blk-mq has been > baking for a very long time. In the early days of block_mq, the > overwhelming percentage of the users of blk-mq where those who were > using PCIe attached flash. So when, I raised this question, the > argument was that SSD users have no entropy. Which I agree with; but > now that blk-mq is the default, and hard drives are using blk-mq, it's > time for a patch like Xuewei's. > > Cheers, > > - Ted
On 09/06/18 16:03, Xuewei Zhang wrote: > On Thu, Sep 6, 2018 at 3:42 PM Theodore Y. Ts'o <tytso@mit.edu> wrote: >> There was a discussion about a number of *years* ago; blk-mq has been >> baking for a very long time. In the early days of block_mq, the >> overwhelming percentage of the users of blk-mq where those who were >> using PCIe attached flash. So when, I raised this question, the >> argument was that SSD users have no entropy. Which I agree with; but >> now that blk-mq is the default, and hard drives are using blk-mq, it's >> time for a patch like Xuewei's. > > Besides Ted's point of "SSD users have no entropy", I think there are > two more reasons: > 1. setting QUEUE_FLAG_ADD_RANDOM has a more visible performance hit > on SSD disks than rotational disks. > 2. SSD disks provide less entropy than rotational disks. > I actually experimented on Container-Optimized OS, running on Google > Compute Engine with QUEUE_FLAG_ADD_RANDOM set. > Turns out the VM will have ~800 bit of entropy provided on boot on > rotational disk; > and will only have ~70 bit of entropy if running on SSD (and remember > there are ~50 bit contributed from other sources). > (in the above experiment, both disks were virtualized disks) All of the above makes sense to me and is much less risky than changing QUEUE_FLAG_MQ_DEFAULT. Hence: Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On Thu, Sep 06, 2018 at 01:37:19PM -0700, Xuewei Zhang wrote: > Currently a scsi device won't contribute to kernel randomness when it > uses blk-mq. Since we commonly use scsi on rotational device with > blk-mq, it make sense to keep contributing to kernel randomness in these > cases. This is especially important for virtual machines. > > commit b5b6e8c8d3b4 ("scsi: virtio_scsi: fix IO hang caused by automatic > irq vector affinity") made all virtio-scsi device to use blk-mq, which > does not contribute to randomness today. So for a virtual machine only > having virtio-scsi disk (which is common), it will simple stop getting > randomness from its disks in today's implementation. > > With this patch, if the above VM has rotational virtio-scsi device, then > it can still benefit from the entropy generated from the disk. > > Reported-by: Xuewei Zhang <xueweiz@google.com> > Signed-off-by: Xuewei Zhang <xueweiz@google.com> > --- > drivers/scsi/sd.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index b79b366a94f7..5e4f10d28065 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -2959,6 +2959,9 @@ 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) { > -- > 2.19.0.rc2.392.g5ba43deb5a-goog > Look reasonable, especially the disk randomness is added by SCSI itself. Reviewed-by: Ming Lei <ming.lei@redhat.com> Thanks, Ming
On Sun, Sep 9, 2018 at 4:52 AM, Ming Lei <ming.lei@redhat.com> wrote: > On Thu, Sep 06, 2018 at 01:37:19PM -0700, Xuewei Zhang wrote: >> Currently a scsi device won't contribute to kernel randomness when it >> uses blk-mq. Since we commonly use scsi on rotational device with >> blk-mq, it make sense to keep contributing to kernel randomness in these >> cases. This is especially important for virtual machines. >> >> commit b5b6e8c8d3b4 ("scsi: virtio_scsi: fix IO hang caused by automatic >> irq vector affinity") made all virtio-scsi device to use blk-mq, which >> does not contribute to randomness today. So for a virtual machine only >> having virtio-scsi disk (which is common), it will simple stop getting >> randomness from its disks in today's implementation. >> >> With this patch, if the above VM has rotational virtio-scsi device, then >> it can still benefit from the entropy generated from the disk. >> >> Reported-by: Xuewei Zhang <xueweiz@google.com> >> Signed-off-by: Xuewei Zhang <xueweiz@google.com> >> --- >> drivers/scsi/sd.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c >> index b79b366a94f7..5e4f10d28065 100644 >> --- a/drivers/scsi/sd.c >> +++ b/drivers/scsi/sd.c >> @@ -2959,6 +2959,9 @@ 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) { >> -- >> 2.19.0.rc2.392.g5ba43deb5a-goog >> > > Look reasonable, especially the disk randomness is added by SCSI itself. > > Reviewed-by: Ming Lei <ming.lei@redhat.com> > > Thanks, > Ming Also, see: https://bugzilla.redhat.com/show_bug.cgi?id=1572944 where we're having randomness starvation problems on FC28 running 4.18.5 due to lack of virtio-rng device in VM. (VM boot takes 9+ hours or 2 haswell VMs) I'd kindly request we get this not only into 4.19 but also stable trees. (along with Ted's other randomization fixes) Reviewed-by: Maciej Żenczykowski <maze@google.com>
Xuewei, > Currently a scsi device won't contribute to kernel randomness when it > uses blk-mq. Since we commonly use scsi on rotational device with > blk-mq, it make sense to keep contributing to kernel randomness in these > cases. This is especially important for virtual machines. Applied to 4.19/scsi-fixes, thank you!
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index b79b366a94f7..5e4f10d28065 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -2959,6 +2959,9 @@ 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) {
Currently a scsi device won't contribute to kernel randomness when it uses blk-mq. Since we commonly use scsi on rotational device with blk-mq, it make sense to keep contributing to kernel randomness in these cases. This is especially important for virtual machines. commit b5b6e8c8d3b4 ("scsi: virtio_scsi: fix IO hang caused by automatic irq vector affinity") made all virtio-scsi device to use blk-mq, which does not contribute to randomness today. So for a virtual machine only having virtio-scsi disk (which is common), it will simple stop getting randomness from its disks in today's implementation. With this patch, if the above VM has rotational virtio-scsi device, then it can still benefit from the entropy generated from the disk. Reported-by: Xuewei Zhang <xueweiz@google.com> Signed-off-by: Xuewei Zhang <xueweiz@google.com> --- drivers/scsi/sd.c | 3 +++ 1 file changed, 3 insertions(+)