diff mbox series

scsi: sd: fix entropy gathering for most rotational disks

Message ID 1549987525.3173.17.camel@HansenPartnership.com (mailing list archive)
State Mainlined
Commit e4a056987c86f402f1286e050b1dee3f4ce7c7eb
Headers show
Series scsi: sd: fix entropy gathering for most rotational disks | expand

Commit Message

James Bottomley Feb. 12, 2019, 4:05 p.m. UTC
The problem is that the default for MQ is not to gather entropy,
whereas the default for the legacy queue was always to gather it.  The
original attempt to fix entropy gathering for rotational disks under MQ
added an else branch in sd_read_block_characteristics(). 
Unfortunately, the entire check isn't reached if the device has no
characteristics VPD page.  Since this page was only introduced in SBC-3 
and its optional anyway, most less expensive rotational disks don't
have one, meaning they all stopped gathering entropy when we made MQ
the default.  In a wholly unrelated change, openssl and openssh won't
function until the random number generator is initialised, meaning lots
of people have been seeing large delays before they could log into
systems with default MQ kernels due to this lack of entropy, because it
now can take tens of minutes to initialise the kernel random number
generator.

The fix is to set the non-rotational and add-randomness flags
unconditionally early on in the disk initialization path, so they can
be reset only if the device actually reports being non-rotational via
the VPD page.

Reported-by: Mikael Pettersson <mikpelinux@gmail.com>
Fixes: 83e32a591077 ("scsi: sd: Contribute to randomness when running rotational device")
Cc: stable@vger.kernel.org
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---
I updated this slightly over the original proposal so we set the flags
even if the device doesn't have any VPD pages, so it should work for
very old disks.

Comments

Jens Axboe Feb. 12, 2019, 4:06 p.m. UTC | #1
On 2/12/19 9:05 AM, James Bottomley wrote:
> The problem is that the default for MQ is not to gather entropy,
> whereas the default for the legacy queue was always to gather it.  The
> original attempt to fix entropy gathering for rotational disks under MQ
> added an else branch in sd_read_block_characteristics(). 
> Unfortunately, the entire check isn't reached if the device has no
> characteristics VPD page.  Since this page was only introduced in SBC-3 
> and its optional anyway, most less expensive rotational disks don't
> have one, meaning they all stopped gathering entropy when we made MQ
> the default.  In a wholly unrelated change, openssl and openssh won't
> function until the random number generator is initialised, meaning lots
> of people have been seeing large delays before they could log into
> systems with default MQ kernels due to this lack of entropy, because it
> now can take tens of minutes to initialise the kernel random number
> generator.
> 
> The fix is to set the non-rotational and add-randomness flags
> unconditionally early on in the disk initialization path, so they can
> be reset only if the device actually reports being non-rotational via
> the VPD page.

Reviewed-by: Jens Axboe <axboe@kernel.dk>
Martin K. Petersen Feb. 12, 2019, 4:59 p.m. UTC | #2
Jens,

>> The fix is to set the non-rotational and add-randomness flags
>> unconditionally early on in the disk initialization path, so they can
>> be reset only if the device actually reports being non-rotational via
>> the VPD page.
>
> Reviewed-by: Jens Axboe <axboe@kernel.dk>

Yeah, this looks good to me. I'll queue it up for fixes.
Xuewei Zhang Feb. 12, 2019, 8 p.m. UTC | #3
Thanks for the fix James!

On Tue, Feb 12, 2019 at 8:59 AM Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>
>
> Jens,
>
> >> The fix is to set the non-rotational and add-randomness flags
> >> unconditionally early on in the disk initialization path, so they can
> >> be reset only if the device actually reports being non-rotational via
> >> the VPD page.
> >
> > Reviewed-by: Jens Axboe <axboe@kernel.dk>

Reviewed-by: Xuewei Zhang <xueweiz@google.com>

>
> Yeah, this looks good to me. I'll queue it up for fixes.
>
> --
> Martin K. Petersen      Oracle Linux Engineering
diff mbox series

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d0a980915801..3b8093c48eba 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2971,9 +2971,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) {
@@ -3110,6 +3107,15 @@  static int sd_revalidate_disk(struct gendisk *disk)
 	if (sdkp->media_present) {
 		sd_read_capacity(sdkp, buffer);
 
+		/*
+		 * set the default to rotational.  All non-rotational devices
+		 * support the block characteristics VPD page, which will
+		 * cause this to be updated correctly and any device which
+		 * doesn't support it should be treated as rotational.
+		 */
+		blk_queue_flag_clear(QUEUE_FLAG_NONROT, q);
+		blk_queue_flag_set(QUEUE_FLAG_ADD_RANDOM, q);
+
 		if (scsi_device_supports_vpd(sdp)) {
 			sd_read_block_provisioning(sdkp);
 			sd_read_block_limits(sdkp);