[Qemu-devel] [PATCH 1/3] hw/block: Introduce rotational qdev property

[Qemu-devel] [PATCH 1/3] hw/block: Introduce rotational qdev property

[Aurelien Jarno]

The Linux kernel uses different I/O scheduler depending if the block
device is a rotational device or not. Also it uses rotational devices
to add entropy to the random pool.

This patch add a rotational qdev property so that the block device can
be configured as a rotational device or a non-rotational device. Default
to true to not change the default behavior.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 blockdev.c               | 4 ++++
 include/hw/block/block.h | 4 +++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 4d8cdedd54..c914420641 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4037,6 +4037,10 @@ QemuOptsList qemu_common_drive_opts = {
             .name = BDRV_OPT_READ_ONLY,
             .type = QEMU_OPT_BOOL,
             .help = "open drive file as read-only",
+        },{
+            .name = "rotational",
+            .type = QEMU_OPT_BOOL,
+            .help = "rotational drive (off, on)",
         },
 
         THROTTLE_OPTS,
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index f3f6e8ef02..304a579eca 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -27,6 +27,7 @@ typedef struct BlockConf {
     uint32_t cyls, heads, secs;
     OnOffAuto wce;
     bool share_rw;
+    bool rotational;
     BlockdevOnError rerror;
     BlockdevOnError werror;
 } BlockConf;
@@ -56,7 +57,8 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
                        _conf.discard_granularity, -1), \
     DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, _conf.wce, \
                             ON_OFF_AUTO_AUTO), \
-    DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false)
+    DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false), \
+    DEFINE_PROP_BOOL("rotational", _state, _conf.rotational, true)
 
 #define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)      \
     DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \
-- 
2.11.0

[Qemu-devel] [PATCH 2/3] scsi-disk: export rotational qdev property

[Aurelien Jarno]

Export the rotational qdev property to the block device characteristics
VPD page.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 hw/scsi/scsi-disk.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index a53f058621..21b7e21a23 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -597,6 +597,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;
@@ -739,6 +740,19 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             outbuf[43] = max_io_sectors & 0xff;
             break;
         }
+        case 0xb1: /* block device characteristics */
+        {
+            buflen = 0x40;
+            memset(outbuf + 4, 0, buflen - 4);
+
+            /* medium rotation rate: 0 = not reported, 1 = non-rotating */
+            outbuf[4] = 0;
+            outbuf[5] = s->qdev.conf.rotational ? 0 : 1;
+
+            /* nominal form factor */
+            outbuf[7] = 0; /* not reported */
+            break;
+        }
         case 0xb2: /* thin provisioning */
         {
             buflen = 8;
-- 
2.11.0

Re: [Qemu-devel] [PATCH 2/3] scsi-disk: export rotational qdev property

[Philippe Mathieu-Daudé]

On 05/06/2017 09:43 AM, Aurelien Jarno wrote:
> Export the rotational qdev property to the block device characteristics
> VPD page.
>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/scsi/scsi-disk.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index a53f058621..21b7e21a23 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -597,6 +597,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;
> @@ -739,6 +740,19 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
>              outbuf[43] = max_io_sectors & 0xff;
>              break;
>          }
> +        case 0xb1: /* block device characteristics */
> +        {
> +            buflen = 0x40;
> +            memset(outbuf + 4, 0, buflen - 4);
> +
> +            /* medium rotation rate: 0 = not reported, 1 = non-rotating */
> +            outbuf[4] = 0;
> +            outbuf[5] = s->qdev.conf.rotational ? 0 : 1;
> +
> +            /* nominal form factor */
> +            outbuf[7] = 0; /* not reported */
> +            break;
> +        }
>          case 0xb2: /* thin provisioning */
>          {
>              buflen = 8;
>

[Qemu-devel] [PATCH 3/3] ide: export rotational qdev property

[Aurelien Jarno]

Export the rotational qdev property in the IDENTIFY request.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 hw/ide/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 0b48b64d3a..1aa76b0d90 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -191,6 +191,9 @@ static void ide_identify(IDEState *s)
     if (dev && dev->conf.discard_granularity) {
         put_le16(p + 169, 1); /* TRIM support */
     }
+    if (dev && !dev->conf.rotational) {
+        put_le16(p + 217, 1); /* non-rotating device */
+    }
 
     ide_identify_size(s);
     s->identify_set = 1;
-- 
2.11.0

Re: [Qemu-devel] [PATCH 3/3] ide: export rotational qdev property

[Philippe Mathieu-Daudé]

On 05/06/2017 09:43 AM, Aurelien Jarno wrote:
> Export the rotational qdev property in the IDENTIFY request.
>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/ide/core.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 0b48b64d3a..1aa76b0d90 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -191,6 +191,9 @@ static void ide_identify(IDEState *s)
>      if (dev && dev->conf.discard_granularity) {
>          put_le16(p + 169, 1); /* TRIM support */
>      }
> +    if (dev && !dev->conf.rotational) {
> +        put_le16(p + 217, 1); /* non-rotating device */
> +    }
>
>      ide_identify_size(s);
>      s->identify_set = 1;
>

Re: [Qemu-devel] [PATCH 3/3] ide: export rotational qdev property

[John Snow]

On 05/06/2017 08:43 AM, Aurelien Jarno wrote:
> Export the rotational qdev property in the IDENTIFY request.
> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  hw/ide/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 0b48b64d3a..1aa76b0d90 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -191,6 +191,9 @@ static void ide_identify(IDEState *s)
>      if (dev && dev->conf.discard_granularity) {
>          put_le16(p + 169, 1); /* TRIM support */
>      }
> +    if (dev && !dev->conf.rotational) {
> +        put_le16(p + 217, 1); /* non-rotating device */
> +    }
>  
>      ide_identify_size(s);
>      s->identify_set = 1;
> 

7.16.7.80 Word 217: Nominal media rotation rate
Word 217 is a copy of Nominal Media Rotation Rate (see A.11.5.4).

*flips page like it's a choose-your-own-adventure book*

OK, so 0x0001 just means "Non-rotational." I guess it probably does not
matter that our default is "Rate not reported."

Looks like there's no relevant bits for packet identify data either, so:

Acked-by: John Snow <jsnow@redhat.com>


A question about the series as a whole, though: if by default rotational
is set to false, we will newly be setting this bit in IDENTIFY data with
identical CLI invocations. Why is that OK?

Re: [Qemu-devel] [PATCH 1/3] hw/block: Introduce rotational qdev property

[Philippe Mathieu-Daudé]

On 05/06/2017 09:43 AM, Aurelien Jarno wrote:
> The Linux kernel uses different I/O scheduler depending if the block
> device is a rotational device or not. Also it uses rotational devices
> to add entropy to the random pool.
>
> This patch add a rotational qdev property so that the block device can
> be configured as a rotational device or a non-rotational device. Default
> to true to not change the default behavior.
>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  blockdev.c               | 4 ++++
>  include/hw/block/block.h | 4 +++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 4d8cdedd54..c914420641 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -4037,6 +4037,10 @@ QemuOptsList qemu_common_drive_opts = {
>              .name = BDRV_OPT_READ_ONLY,
>              .type = QEMU_OPT_BOOL,
>              .help = "open drive file as read-only",
> +        },{
> +            .name = "rotational",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "rotational drive (off, on)",
>          },
>
>          THROTTLE_OPTS,
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index f3f6e8ef02..304a579eca 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -27,6 +27,7 @@ typedef struct BlockConf {
>      uint32_t cyls, heads, secs;
>      OnOffAuto wce;
>      bool share_rw;
> +    bool rotational;
>      BlockdevOnError rerror;
>      BlockdevOnError werror;
>  } BlockConf;
> @@ -56,7 +57,8 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>                         _conf.discard_granularity, -1), \
>      DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, _conf.wce, \
>                              ON_OFF_AUTO_AUTO), \
> -    DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false)
> +    DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false), \
> +    DEFINE_PROP_BOOL("rotational", _state, _conf.rotational, true)
>
>  #define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)      \
>      DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \
>

Re: [Qemu-devel] [PATCH 1/3] hw/block: Introduce rotational qdev property

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]

On 05/06/2017 07:43 AM, Aurelien Jarno wrote:
> The Linux kernel uses different I/O scheduler depending if the block
> device is a rotational device or not. Also it uses rotational devices
> to add entropy to the random pool.
> 
> This patch add a rotational qdev property so that the block device can
> be configured as a rotational device or a non-rotational device. Default
> to true to not change the default behavior.
> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  blockdev.c               | 4 ++++
>  include/hw/block/block.h | 4 +++-
>  2 files changed, 7 insertions(+), 1 deletion(-)

Please remember to send a 0/3 cover letter when sending a series (you
can use 'git config format.coverletter auto' to make it easier).

The NBD server has the ability to expose to a client whether a given
exported volume is rotational or not; so there may be some additional
integration you should perform to make nbd-server properly export this
new property.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/3] hw/block: Introduce rotational qdev property

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

On Sat, May 06, 2017 at 02:43:12PM +0200, Aurelien Jarno wrote:
> The Linux kernel uses different I/O scheduler depending if the block
> device is a rotational device or not. Also it uses rotational devices
> to add entropy to the random pool.
> 
> This patch add a rotational qdev property so that the block device can
> be configured as a rotational device or a non-rotational device. Default
> to true to not change the default behavior.
> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  blockdev.c               | 4 ++++
>  include/hw/block/block.h | 4 +++-
>  2 files changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 1/3] hw/block: Introduce rotational qdev property

[Kevin Wolf]

Am 06.05.2017 um 14:43 hat Aurelien Jarno geschrieben:
> The Linux kernel uses different I/O scheduler depending if the block
> device is a rotational device or not. Also it uses rotational devices
> to add entropy to the random pool.
> 
> This patch add a rotational qdev property so that the block device can
> be configured as a rotational device or a non-rotational device. Default
> to true to not change the default behavior.
> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  blockdev.c               | 4 ++++
>  include/hw/block/block.h | 4 +++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 4d8cdedd54..c914420641 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -4037,6 +4037,10 @@ QemuOptsList qemu_common_drive_opts = {
>              .name = BDRV_OPT_READ_ONLY,
>              .type = QEMU_OPT_BOOL,
>              .help = "open drive file as read-only",
> +        },{
> +            .name = "rotational",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "rotational drive (off, on)",
>          },
>  

This hunk is wrong (and unnecessary). It makes the -drive parser parse
rotational=... options without doing anything with them, so instead of
correctly producing an error that this isn't a block driver option, it
would be silently ignored.

-drive has a few legacy options that affect qdev devices, but we don't
want to add anything new there; people can just use -device. So the fix
for your patch is simply dropping this hunk.

> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index f3f6e8ef02..304a579eca 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -27,6 +27,7 @@ typedef struct BlockConf {
>      uint32_t cyls, heads, secs;
>      OnOffAuto wce;
>      bool share_rw;
> +    bool rotational;
>      BlockdevOnError rerror;
>      BlockdevOnError werror;
>  } BlockConf;
> @@ -56,7 +57,8 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>                         _conf.discard_granularity, -1), \
>      DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, _conf.wce, \
>                              ON_OFF_AUTO_AUTO), \
> -    DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false)
> +    DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false), \
> +    DEFINE_PROP_BOOL("rotational", _state, _conf.rotational, true)
>  
>  #define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)      \
>      DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \

Also not sure about this one. It adds the property to all block devices,
but only IDE and SCSI will actually look at it. Maybe it would be better
to add this property only to the individual devices that actually
support it.

Kevin