[Qemu-devel] [PATCH 0/2] virtio-blk: miscellaneous changes

[Qemu-devel] [PATCH 0/2] virtio-blk: miscellaneous changes

[Mark Kanda]

This series is for two minor virtio-blk changes. The first patch
makes the virtio-blk queue size user configurable. The second patch
rejects logical block size > physical block configurations (similar
to a recent change in virtio-scsi).

Mark Kanda (2):
  virtio-blk: make queue size configurable
  virtio-blk: reject configs with logical block size > physical block
    size

 hw/block/virtio-blk.c          | 14 +++++++++++++-
 include/hw/virtio/virtio-blk.h |  1 +
 2 files changed, 14 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] virtio-blk: make queue size configurable

[Mark Kanda]

Depending on the configuration, it can be beneficial to adjust the virtio-blk
queue size to something other than the current default of 128. Add a new
property to make the queue size configurable.

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Reviewed-by: Ameya More <ameya.more@oracle.com>
---
 hw/block/virtio-blk.c          | 7 ++++++-
 include/hw/virtio/virtio-blk.h | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 05d1440..002c56f 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -928,6 +928,10 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "num-queues property must be larger than 0");
         return;
     }
+    if (!is_power_of_2(conf->queue_size)) {
+        error_setg(errp, "queue-size property must be a power of 2");
+        return;
+    }
 
     blkconf_serial(&conf->conf, &conf->serial);
     blkconf_apply_backend_options(&conf->conf,
@@ -953,7 +957,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     s->sector_mask = (s->conf.conf.logical_block_size / BDRV_SECTOR_SIZE) - 1;
 
     for (i = 0; i < conf->num_queues; i++) {
-        virtio_add_queue(vdev, 128, virtio_blk_handle_output);
+        virtio_add_queue(vdev, conf->queue_size, virtio_blk_handle_output);
     }
     virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err);
     if (err != NULL) {
@@ -1012,6 +1016,7 @@ static Property virtio_blk_properties[] = {
     DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0,
                     true),
     DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1),
+    DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
     DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
                      IOThread *),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index d3c8a6f..5117431 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -39,6 +39,7 @@ struct VirtIOBlkConf
     uint32_t config_wce;
     uint32_t request_merging;
     uint16_t num_queues;
+    uint16_t queue_size;
 };
 
 struct VirtIOBlockDataPlane;
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] virtio-blk: reject configs with logical block size > physical block size

[Mark Kanda]

virtio-blk logical block size should never be larger than physical block
size because it doesn't make sense to have such configurations. QEMU doesn't
have a way to effectively express this condition; the best it can do is
report the physical block exponent as 0 - indicating the logical block size
equals the physical block size.

This is identical to commit 3da023b5827543ee4c022986ea2ad9d1274410b2
but applied to virtio-blk (instead of virtio-scsi).

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Ameya More <ameya.more@oracle.com>
---
 hw/block/virtio-blk.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 002c56f..acfca78 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -949,6 +949,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     }
     blkconf_blocksizes(&conf->conf);
 
+    if (conf->conf.logical_block_size >
+        conf->conf.physical_block_size) {
+        error_setg(errp,
+                   "logical_block_size > physical_block_size not supported");
+        return;
+    }
+
     virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
                 sizeof(struct virtio_blk_config));
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/2] virtio-blk: reject configs with logical block size > physical block size

[Martin K. Petersen]

Mark,

> virtio-blk logical block size should never be larger than physical block
> size because it doesn't make sense to have such configurations. QEMU doesn't
> have a way to effectively express this condition; the best it can do is
> report the physical block exponent as 0 - indicating the logical block size
> equals the physical block size.
>
> This is identical to commit 3da023b5827543ee4c022986ea2ad9d1274410b2
> but applied to virtio-blk (instead of virtio-scsi).

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [Qemu-devel] [PATCH 1/2] virtio-blk: make queue size configurable

[Stefan Hajnoczi]

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

On Wed, Dec 06, 2017 at 01:54:00PM -0600, Mark Kanda wrote:
> Depending on the configuration, it can be beneficial to adjust the virtio-blk
> queue size to something other than the current default of 128. Add a new
> property to make the queue size configurable.
> 
> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
> Reviewed-by: Ameya More <ameya.more@oracle.com>
> ---
>  hw/block/virtio-blk.c          | 7 ++++++-
>  include/hw/virtio/virtio-blk.h | 1 +
>  2 files changed, 7 insertions(+), 1 deletion(-)

Please see commit 9b02e1618cf26aa52cf786f215d757506dda14f8 ("virtio-net:
enable configurable tx queue size") for additional considerations.
QEMU's virtio implementation hardcodes the maximum queue size at 1024
and this limit must be checked.

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

Re: [Qemu-devel] [PATCH 2/2] virtio-blk: reject configs with logical block size > physical block size

[Stefan Hajnoczi]

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

On Wed, Dec 06, 2017 at 01:54:01PM -0600, Mark Kanda wrote:
> virtio-blk logical block size should never be larger than physical block
> size because it doesn't make sense to have such configurations. QEMU doesn't
> have a way to effectively express this condition; the best it can do is
> report the physical block exponent as 0 - indicating the logical block size
> equals the physical block size.
> 
> This is identical to commit 3da023b5827543ee4c022986ea2ad9d1274410b2
> but applied to virtio-blk (instead of virtio-scsi).
> 
> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Ameya More <ameya.more@oracle.com>
> ---
>  hw/block/virtio-blk.c | 7 +++++++
>  1 file changed, 7 insertions(+)

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

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

[Qemu-devel] [PATCH v2 0/2] virtio-blk: miscellaneous changes

[Mark Kanda]

v2: add check for maximum queue size [Stefan]

This series is for two minor virtio-blk changes. The first patch
makes the virtio-blk queue size user configurable. The second patch
rejects logical block size > physical block configurations (similar
to a recent change in virtio-scsi).

Mark Kanda (2):
  virtio-blk: make queue size configurable
  virtio-blk: reject configs with logical block size > physical block
    size

 hw/block/virtio-blk.c          | 17 ++++++++++++++++-
 include/hw/virtio/virtio-blk.h |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/2] virtio-blk: make queue size configurable

[Mark Kanda]

Depending on the configuration, it can be beneficial to adjust the virtio-blk
queue size to something other than the current default of 128. Add a new
property to make the queue size configurable.

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Reviewed-by: Ameya More <ameya.more@oracle.com>
---
 hw/block/virtio-blk.c          | 10 +++++++++-
 include/hw/virtio/virtio-blk.h |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 05d1440..fb59f57 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -928,6 +928,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "num-queues property must be larger than 0");
         return;
     }
+    if (!is_power_of_2(conf->queue_size) ||
+        conf->queue_size > VIRTQUEUE_MAX_SIZE) {
+        error_setg(errp, "invalid queue-size property (%" PRIu16 "), "
+                   "must be a power of 2 (max %d)",
+                   conf->queue_size, VIRTQUEUE_MAX_SIZE);
+        return;
+    }
 
     blkconf_serial(&conf->conf, &conf->serial);
     blkconf_apply_backend_options(&conf->conf,
@@ -953,7 +960,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     s->sector_mask = (s->conf.conf.logical_block_size / BDRV_SECTOR_SIZE) - 1;
 
     for (i = 0; i < conf->num_queues; i++) {
-        virtio_add_queue(vdev, 128, virtio_blk_handle_output);
+        virtio_add_queue(vdev, conf->queue_size, virtio_blk_handle_output);
     }
     virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err);
     if (err != NULL) {
@@ -1012,6 +1019,7 @@ static Property virtio_blk_properties[] = {
     DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0,
                     true),
     DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1),
+    DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
     DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
                      IOThread *),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index d3c8a6f..5117431 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -39,6 +39,7 @@ struct VirtIOBlkConf
     uint32_t config_wce;
     uint32_t request_merging;
     uint16_t num_queues;
+    uint16_t queue_size;
 };
 
 struct VirtIOBlockDataPlane;
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/2] virtio-blk: reject configs with logical block size > physical block size

[Mark Kanda]

virtio-blk logical block size should never be larger than physical block
size because it doesn't make sense to have such configurations. QEMU doesn't
have a way to effectively express this condition; the best it can do is
report the physical block exponent as 0 - indicating the logical block size
equals the physical block size.

This is identical to commit 3da023b5827543ee4c022986ea2ad9d1274410b2
but applied to virtio-blk (instead of virtio-scsi).

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Ameya More <ameya.more@oracle.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index fb59f57..36c7608 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -952,6 +952,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     }
     blkconf_blocksizes(&conf->conf);
 
+    if (conf->conf.logical_block_size >
+        conf->conf.physical_block_size) {
+        error_setg(errp,
+                   "logical_block_size > physical_block_size not supported");
+        return;
+    }
+
     virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
                 sizeof(struct virtio_blk_config));
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 0/2] virtio-blk: miscellaneous changes

[Stefan Hajnoczi]

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

On Fri, Dec 08, 2017 at 09:57:25AM -0600, Mark Kanda wrote:
> v2: add check for maximum queue size [Stefan]
> 
> This series is for two minor virtio-blk changes. The first patch
> makes the virtio-blk queue size user configurable. The second patch
> rejects logical block size > physical block configurations (similar
> to a recent change in virtio-scsi).
> 
> Mark Kanda (2):
>   virtio-blk: make queue size configurable
>   virtio-blk: reject configs with logical block size > physical block
>     size
> 
>  hw/block/virtio-blk.c          | 17 ++++++++++++++++-
>  include/hw/virtio/virtio-blk.h |  1 +
>  2 files changed, 17 insertions(+), 1 deletion(-)

Hi Mark,
Please resend as a top level email thread so the continuous integration
and patch management tools will detect your patch series.

From https://wiki.qemu.org/Contribute/SubmitAPatch:

  "Send each new revision as a new top-level thread, rather than burying
  it in-reply-to an earlier revision, as many reviewers are not looking
  inside deep threads for new patches."

Thanks,
Stefan

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

Re: [Qemu-devel] [PATCH v2 0/2] virtio-blk: miscellaneous changes

[Mark Kanda]

On 12/11/2017 4:30 AM, Stefan Hajnoczi wrote:
> Hi Mark,
> Please resend as a top level email thread so the continuous integration
> and patch management tools will detect your patch series.

Apologies. I've just resent the series.

Thanks,

-Mark

[Qemu-devel] [PATCH v2 2/2] virtio-blk: reject configs with logical block size > physical block size

[Mark Kanda]

virtio-blk logical block size should never be larger than physical block
size because it doesn't make sense to have such configurations. QEMU doesn't
have a way to effectively express this condition; the best it can do is
report the physical block exponent as 0 - indicating the logical block size
equals the physical block size.

This is identical to commit 3da023b5827543ee4c022986ea2ad9d1274410b2
but applied to virtio-blk (instead of virtio-scsi).

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Ameya More <ameya.more@oracle.com>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index fb59f57..36c7608 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -952,6 +952,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     }
     blkconf_blocksizes(&conf->conf);
 
+    if (conf->conf.logical_block_size >
+        conf->conf.physical_block_size) {
+        error_setg(errp,
+                   "logical_block_size > physical_block_size not supported");
+        return;
+    }
+
     virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
                 sizeof(struct virtio_blk_config));
 
-- 
1.8.3.1