[Qemu-devel] [PATCH v1 1/2] scsi-disk: support reporting of rotation rate

Daniel P. Berrange posted 2 patches 8 years, 4 months ago
[Qemu-devel] [PATCH v1 1/2] scsi-disk: support reporting of rotation rate
Posted by Daniel P. Berrange 8 years, 4 months ago
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 <berrange@redhat.com>
---
 hw/scsi/scsi-disk.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 6e841fb5ff..a518080e7d 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -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),
+    DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.13.5


Re: [Qemu-devel] [PATCH v1 1/2] scsi-disk: support reporting of rotation rate
Posted by Eric Blake 8 years, 4 months ago
On 10/04/2017 06:40 AM, Daniel P. Berrange 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 <berrange@redhat.com>
> ---
>  hw/scsi/scsi-disk.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)

>      bool tray_locked;
> +    /*
> +     * 0x0000        - rotation rate not reported
> +     * 0x0001        - non-rotating medium (SSD)
> +     * 0x0002-0x0400 - reserved
> +     * 0x0401-0xffe  - rotations per minute

s/0xffe/0xfffe/

> +     * 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 */

This function is awkward - it is a non-local audit to see whether we are
 at risk of overflowing any buffers due to the new output.  But from
what I can see, I think you're safe.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v1 1/2] scsi-disk: support reporting of rotation rate
Posted by Dr. David Alan Gilbert 8 years, 4 months ago
* Daniel P. Berrange (berrange@redhat.com) 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 <berrange@redhat.com>
> ---
>  hw/scsi/scsi-disk.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 6e841fb5ff..a518080e7d 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -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),
> +    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.

Dave

>  
> -- 
> 2.13.5
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v1 1/2] scsi-disk: support reporting of rotation rate
Posted by Daniel P. Berrange 8 years, 4 months ago
On Wed, Oct 04, 2017 at 05:49:44PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berrange@redhat.com) 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 <berrange@redhat.com>
> > ---
> >  hw/scsi/scsi-disk.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> > index 6e841fb5ff..a518080e7d 100644
> > --- a/hw/scsi/scsi-disk.c
> > +++ b/hw/scsi/scsi-disk.c
> > @@ -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),
> > +    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.

That would make 'rotation_rate' available for everything, regardless
of whether its used/supported by the frontend device. I was trying to
restrict this to only the places where its appropriate, so only disks,
not cdroms for example.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|