Reporting of rotation rate for disks

The Linux kernel will query the SCSI "Block device characteristics" VPD to determine the rotations per minute of the disk. If this has the value 1, it is taken to be an SSD and so Linux sets the 'rotational' flag to 0 for the I/O queue and will stop using that disk as a source of random entropy. Other operating systems may also take into account rotation rate when setting up default behaviour.

Mgmt apps should be able to set the rotation rate for virtualized block devices, based on characteristics of the host storage in use, so that the guest OS gets sensible behaviour out of the box. This patch thus adds a 'rotation-rate' parameter for 'scsi-hd' and 'scsi-block' device types. For the latter, this parameter will be ignored unless the host device has TYPE_DISK.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 hw/scsi/scsi-disk.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

     bool tray_locked;

The Linux kernel will query the SCSI "Block device characteristics" VPD to determine the rotations per minute of the disk. If this has the value 1, it is taken to be an SSD and so Linux sets the 'rotational' flag to 0 for the I/O queue and will stop using that disk as a source of random entropy. Other operating systems may also take into account rotation rate when setting up default behaviour.

Mgmt apps should be able to set the rotation rate for virtualized block devices, based on characteristics of the host storage in use, so that the guest OS gets sensible behaviour out of the box. This patch thus adds a 'rotation-rate' parameter for 'scsi-hd' and 'scsi-block' device types. For the latter, this parameter will be ignored unless the host device has TYPE_DISK.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 hw/scsi/scsi-disk.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

hw/scsi/scsi-disk.c +19 -0

index 6e841fb5ff..a518080e7d 100644
@@ -104,6 +104,14 @@ typedef struct SCSIDiskState
     char *product;
     bool tray_open;
     bool tray_locked;
+    /*
+     * 0x0000        - rotation rate not reported
+     * 0x0001        - non-rotating medium (SSD)
+     * 0x0002-0x0400 - reserved
+     * 0x0401-0xffe  - rotations per minute
+     * 0xffff        - reserved
+     */
+    uint16_t rotation_rate;
 } SCSIDiskState;
 
 static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed);

@@ -605,6 +613,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             outbuf[buflen++] = 0x83; // device identification
             if (s->qdev.type == TYPE_DISK) {
                 outbuf[buflen++] = 0xb0; // block limits
+                outbuf[buflen++] = 0xb1; /* block device characteristics */
                 outbuf[buflen++] = 0xb2; // thin provisioning
             }
             break;

@@ -747,6 +756,15 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             outbuf[43] = max_io_sectors & 0xff;
             break;
         }
+        case 0xb1: /* block device characteristics */
+        {
+            buflen = 8;
+            outbuf[4] = (s->rotation_rate >> 8) & 0xff;
+            outbuf[5] = s->rotation_rate & 0xff;
+            outbuf[6] = 0;
+            outbuf[7] = 0;
+            break;
+        }
         case 0xb2: /* thin provisioning */
         {
             buflen = 8;

@@ -2911,6 +2929,7 @@ static Property scsi_hd_properties[] = {
                        DEFAULT_MAX_UNMAP_SIZE),
     DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
                        DEFAULT_MAX_IO_SIZE),
+    DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
     DEFINE_BLOCK_CHS_PROPERTIES(SCSIDiskState, qdev.conf),
     DEFINE_PROP_END_OF_LIST(),
 };

Signature · 2 lines

-- 
2.13.5

Quoted 75 lines · * Daniel P. Berrange ([email protected]) wrote:

The Linux kernel will query the SCSI "Block device characteristics" VPD to determine the rotations per minute of the disk. If this has the value 1, it is taken to be an SSD and so Linux sets the 'rotational' flag to 0 for the I/O queue and will stop using that disk as a source of random entropy. Other operating systems may also take into account rotation rate when setting up default behaviour.

Mgmt apps should be able to set the rotation rate for virtualized block devices, based on characteristics of the host storage in use, so that the guest OS gets sensible behaviour out of the box. This patch thus adds a 'rotation-rate' parameter for 'scsi-hd' and 'scsi-block' device types. For the latter, this parameter will be ignored unless the host device has TYPE_DISK.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 hw/scsi/scsi-disk.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

hw/scsi/scsi-disk.c +19 -0

index 6e841fb5ff..a518080e7d 100644
@@ -104,6 +104,14 @@ typedef struct SCSIDiskState
     char *product;
     bool tray_open;
     bool tray_locked;
+    /*
+     * 0x0000        - rotation rate not reported
+     * 0x0001        - non-rotating medium (SSD)
+     * 0x0002-0x0400 - reserved
+     * 0x0401-0xffe  - rotations per minute
+     * 0xffff        - reserved
+     */
+    uint16_t rotation_rate;
 } SCSIDiskState;
 
 static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed);

@@ -605,6 +613,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             outbuf[buflen++] = 0x83; // device identification
             if (s->qdev.type == TYPE_DISK) {
                 outbuf[buflen++] = 0xb0; // block limits
+                outbuf[buflen++] = 0xb1; /* block device characteristics */
                 outbuf[buflen++] = 0xb2; // thin provisioning
             }
             break;

@@ -747,6 +756,15 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             outbuf[43] = max_io_sectors & 0xff;
             break;
         }
+        case 0xb1: /* block device characteristics */
+        {
+            buflen = 8;
+            outbuf[4] = (s->rotation_rate >> 8) & 0xff;
+            outbuf[5] = s->rotation_rate & 0xff;
+            outbuf[6] = 0;
+            outbuf[7] = 0;
+            break;
+        }
         case 0xb2: /* thin provisioning */
         {
             buflen = 8;

@@ -2911,6 +2929,7 @@ static Property scsi_hd_properties[] = {
                        DEFAULT_MAX_UNMAP_SIZE),
     DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
                        DEFAULT_MAX_IO_SIZE),
+    DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
     DEFINE_BLOCK_CHS_PROPERTIES(SCSIDiskState, qdev.conf),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -2982,6 +3001,7 @@ static const TypeInfo scsi_cd_info = {
 static Property scsi_block_properties[] = {
     DEFINE_BLOCK_ERROR_PROPERTIES(SCSIDiskState, qdev.conf),         \
     DEFINE_PROP_DRIVE("drive", SCSIDiskState, qdev.conf.blk),

hw/scsi/scsi-disk.c static Property scsi_hd_properties[] = { · line 2935

Quoted 3 lines

hw/scsi/scsi-disk.c +1 -0

+    DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
     DEFINE_PROP_END_OF_LIST(),
 };

Are you sure you want this as a property of SCSIDiskState etc rather than a property on the underlying drive?
Then if virtio devices etc wanted to pick this up later they could without needing extra parameters.

The Linux kernel will query the ATA IDENTITY DEVICE data, word 217 to determine the rotations per minute of the disk. If this has the value 1, it is taken to be an SSD and so Linux sets the 'rotational' flag to 0 for the I/O queue and will stop using that disk as a source of random entropy. Other operating systems may also take into account rotation rate when setting up default behaviour.

Mgmt apps should be able to set the rotation rate for virtualized block devices, based on characteristics of the host storage in use, so that the guest OS gets sensible behaviour out of the box. This patch thus adds a 'rotation-rate' parameter for 'ide-hd' device types.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 hw/ide/core.c             | 1 +

+++ b/include/hw/ide/internal.h

include/hw/ide/internal.h +3 -0

+     * 0xffff        - reserved
+     */
+    uint16_t rotation_rate;
 };
 
 /* These are used for the error_status field of IDEBus */

The Linux kernel will query the ATA IDENTITY DEVICE data, word 217 to determine the rotations per minute of the disk. If this has the value 1, it is taken to be an SSD and so Linux sets the 'rotational' flag to 0 for the I/O queue and will stop using that disk as a source of random entropy. Other operating systems may also take into account rotation rate when setting up default behaviour.

Mgmt apps should be able to set the rotation rate for virtualized block devices, based on characteristics of the host storage in use, so that the guest OS gets sensible behaviour out of the box. This patch thus adds a 'rotation-rate' parameter for 'ide-hd' device types.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 hw/ide/core.c             | 1 +
 hw/ide/qdev.c             | 1 +
 include/hw/ide/internal.h | 8 ++++++++
 3 files changed, 10 insertions(+)

hw/ide/core.c +1 -0

index 5f1cd3b91f..a04766aee7 100644
@@ -208,6 +208,7 @@ static void ide_identify(IDEState *s)
     if (dev && dev->conf.discard_granularity) {
         put_le16(p + 169, 1); /* TRIM support */
     }
+    put_le16(p + 217, dev->rotation_rate); /* Nominal media rotation rate */
 
     ide_identify_size(s);
     s->identify_set = 1;

hw/ide/qdev.c +1 -0

index d60ac25be0..a5181b4448 100644
@@ -299,6 +299,7 @@ static Property ide_hd_properties[] = {
     DEFINE_BLOCK_CHS_PROPERTIES(IDEDrive, dev.conf),
     DEFINE_PROP_BIOS_CHS_TRANS("bios-chs-trans",
                 IDEDrive, dev.chs_trans, BIOS_ATA_TRANSLATION_AUTO),
+    DEFINE_PROP_UINT16("rotation_rate", IDEDrive, dev.rotation_rate, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 

include/hw/ide/internal.h +1 -0

index e641012b48..31851b44d1 100644
@@ -508,6 +508,14 @@ struct IDEDevice {
     char *serial;
     char *model;
     uint64_t wwn;
+    /*

The Linux kernel will query the ATA IDENTITY DEVICE data, word 217 to determine the rotations per minute of the disk. If this has the value 1, it is taken to be an SSD and so Linux sets the 'rotational' flag to 0 for the I/O queue and will stop using that disk as a source of random entropy. Other operating systems may also take into account rotation rate when setting up default behaviour.

Mgmt apps should be able to set the rotation rate for virtualized block devices, based on characteristics of the host storage in use, so that the guest OS gets sensible behaviour out of the box. This patch thus adds a 'rotation-rate' parameter for 'ide-hd' device types.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 hw/ide/core.c             | 1 +
 hw/ide/qdev.c             | 1 +
 include/hw/ide/internal.h | 8 ++++++++
 3 files changed, 10 insertions(+)

hw/ide/core.c +0 -0

index 5f1cd3b91f..a04766aee7 100644

[ Cc: qemu-block ]

Am 04.10.2017 um 13:40 hat Daniel P. Berrange geschrieben:

Quoted 26 lines

The Linux kernel will query the ATA IDENTITY DEVICE data, word 217 to determine the rotations per minute of the disk. If this has the value 1, it is taken to be an SSD and so Linux sets the 'rotational' flag to 0 for the I/O queue and will stop using that disk as a source of random entropy. Other operating systems may also take into account rotation rate when setting up default behaviour.

Mgmt apps should be able to set the rotation rate for virtualized block devices, based on characteristics of the host storage in use, so that the guest OS gets sensible behaviour out of the box. This patch thus adds a 'rotation-rate' parameter for 'ide-hd' device types.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 hw/ide/core.c             | 1 +
 hw/ide/qdev.c             | 1 +
 include/hw/ide/internal.h | 8 ++++++++
 3 files changed, 10 insertions(+)

hw/ide/core.c +0 -0

index 5f1cd3b91f..a04766aee7 100644

hw/ide/core.c static void ide_identify(IDEState *s) · line 211

Quoted 4 lines

hw/ide/core.c +1 -0

@@ -208,6 +208,7 @@ static void ide_identify(IDEState *s)
     if (dev && dev->conf.discard_granularity) {
         put_le16(p + 169, 1); /* TRIM support */
     }
+    put_le16(p + 217, dev->rotation_rate); /* Nominal media rotation rate */

Coverity points out that all other dereferences of dev have a NULL check first. Are we sure that it is always non-NULL?

A follow-up patch is necessary either way. Either fix the missing NULL check here or remove useless NULL checks in the other places.

Quoted 37 lines · Kevin Wolf

[ Cc: qemu-block ]

Am 04.10.2017 um 13:40 hat Daniel P. Berrange geschrieben:

Quoted 26 lines

The Linux kernel will query the ATA IDENTITY DEVICE data, word 217 to determine the rotations per minute of the disk. If this has the value 1, it is taken to be an SSD and so Linux sets the 'rotational' flag to 0 for the I/O queue and will stop using that disk as a source of random entropy. Other operating systems may also take into account rotation rate when setting up default behaviour.

Mgmt apps should be able to set the rotation rate for virtualized block devices, based on characteristics of the host storage in use, so that the guest OS gets sensible behaviour out of the box. This patch thus adds a 'rotation-rate' parameter for 'ide-hd' device types.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 hw/ide/core.c             | 1 +
 hw/ide/qdev.c             | 1 +
 include/hw/ide/internal.h | 8 ++++++++
 3 files changed, 10 insertions(+)

hw/ide/core.c +0 -0

index 5f1cd3b91f..a04766aee7 100644

hw/ide/core.c static void ide_identify(IDEState *s) · line 211

Quoted 4 lines

hw/ide/core.c +1 -0

@@ -208,6 +208,7 @@ static void ide_identify(IDEState *s)
     if (dev && dev->conf.discard_granularity) {
         put_le16(p + 169, 1); /* TRIM support */
     }
+    put_le16(p + 217, dev->rotation_rate); /* Nominal media rotation rate */

Coverity points out that all other dereferences of dev have a NULL check first. Are we sure that it is always non-NULL?

Quoted 2 lines

A follow-up patch is necessary either way. Either fix the missing NULL check here or remove useless NULL checks in the other places.

'dev' comes from:

    IDEDevice *dev = s->unit ? s->bus->slave : s->bus->master;

IIUC, this is choosing either the first or the second unit on the IDE bus. Presumably this can be lead to dev==NULL, when the guest OS calls identify on a unit that doesn't have a drive attached. Soo the NULL checks looks like its required to me.

Regards,
Daniel

While looking at libvirt tagged questions on serverfault I saw someone ask how to indicate that a virtual disk is an SSD rather than rotating rust.

https://serverfault.com/questions/876467/how-to-add-virtual-storage-as-ssd-in-kvm

IIUC, IDE / SCSI don't really have an explicit concept of a disk being SSD vs HDD, but they do have a means of reporting the disk rotation rate and both specify that SSDs should report RPM==1.

Linux uses this to determine whether to set the 'rotational' property to 0 for the I/O queue, and also whether to allow the disk to contribute random entropy (only HDDs contribute).

This felt like something QEMU ought to allow the mgmt application to set based on the storage it is using to back the virtual block devices. So this series adds a 'rotation_rate' property to the SCSI and IDE disks, taking an RPM value per their respective specifications.

There is no mechanism to report this information to virtio-blk. We could perhaps argue that people should use virtio-scsi instead, because fixing virtio-blk would require enhancement to both QEMU and Linux virtio-blk drivers (and other guest OS drivers too)

I'm unclear if there is anything I should have done wrt the device migration vmstate when adding this extra field, or if it is safe to just expect the mgmt app to set the property correctly on src+dst ?