virtio-blk: miscellaneous changes

PATCH Superseded Mark Kanda 4 participants 11 messages Dec 6, 2017 – Dec 11, 2017

Share & tools

Mark Kanda

author

2017-12-06 19:52 UTC #0 lore raw

cover virtio-blk: miscellaneous changes

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(-)

Signature · 1 line

1.8.3.1

Mark Kanda

author

2017-12-06 19:52 UTC #1 lore raw

PATCH 1/2 virtio-blk: make queue size configurable

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 <[email protected]>

Reviewed-by Karl Heubaum <[email protected]>

Reviewed-by Martin K. Petersen <[email protected]>

Reviewed-by Ameya More <[email protected]>

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

hw/block/virtio-blk.c +6 -1

index 05d1440..002c56f 100644
@@ -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(),

include/hw/virtio/virtio-blk.h +1 -0

index d3c8a6f..5117431 100644
@@ -39,6 +39,7 @@ struct VirtIOBlkConf
     uint32_t config_wce;
     uint32_t request_merging;
     uint16_t num_queues;
+    uint16_t queue_size;
 };
 
 struct VirtIOBlockDataPlane;

Signature · 1 line

1.8.3.1

Stefan Hajnoczi

2017-12-08 10:13 UTC #2 lore raw

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

Quoted 4 lines · 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.

Quoted 8 lines

Signed-off-by Mark Kanda <[email protected]>
Reviewed-by Karl Heubaum <[email protected]>
Reviewed-by Martin K. Petersen <[email protected]>
Reviewed-by Ameya More <[email protected]>
---
 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.

Mark Kanda

author

2017-12-06 19:52 UTC #3 lore raw

PATCH 2/2 virtio-blk: reject configs with logical block size > physical block size

In reply to 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 <[email protected]>

Reviewed-by Konrad Rzeszutek Wilk <[email protected]>

Reviewed-by Ameya More <[email protected]>

---
 hw/block/virtio-blk.c | 7 +++++++
 1 file changed, 7 insertions(+)

hw/block/virtio-blk.c +7 -0

index 002c56f..acfca78 100644
@@ -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));

Signature · 1 line

1.8.3.1

Martin K. Petersen

Reviewed-by on v1 2/2

2017-12-06 20:41 UTC #4 lore raw

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

Mark,

Quoted 8 lines

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 <[email protected]>

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Stefan Hajnoczi

Reviewed-by on v1 2/2

2017-12-08 10:13 UTC #5 lore raw

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

In reply to Mark Kanda

Quoted 9 lines · 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).

Quoted 6 lines

Signed-off-by Mark Kanda <[email protected]>
Reviewed-by Konrad Rzeszutek Wilk <[email protected]>
Reviewed-by Ameya More <[email protected]>
---
 hw/block/virtio-blk.c | 7 +++++++
 1 file changed, 7 insertions(+)

Reviewed-by Stefan Hajnoczi <[email protected]>

Mark Kanda

author

2017-12-08 15:55 UTC #6 lore raw

v2 cover virtio-blk: miscellaneous changes

In reply to Mark Kanda

v2: add check for maximum queue size [Stefan]

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(-)

Signature · 1 line

1.8.3.1

Mark Kanda

author

2017-12-08 15:55 UTC #7 lore raw

PATCH v2 1/2 virtio-blk: make queue size configurable

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 <[email protected]>

Reviewed-by Karl Heubaum <[email protected]>

Reviewed-by Martin K. Petersen <[email protected]>

Reviewed-by Ameya More <[email protected]>

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

hw/block/virtio-blk.c +9 -1

index 05d1440..fb59f57 100644
@@ -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(),

include/hw/virtio/virtio-blk.h +1 -0

index d3c8a6f..5117431 100644
@@ -39,6 +39,7 @@ struct VirtIOBlkConf
     uint32_t config_wce;
     uint32_t request_merging;
     uint16_t num_queues;
+    uint16_t queue_size;
 };
 
 struct VirtIOBlockDataPlane;

Signature · 1 line

1.8.3.1

Mark Kanda

author

2017-12-08 15:55 UTC #8 lore raw

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

In reply to Mark Kanda

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

Signed-off-by Mark Kanda <[email protected]>

Reviewed-by Konrad Rzeszutek Wilk <[email protected]>

Reviewed-by Ameya More <[email protected]>

Reviewed-by Stefan Hajnoczi <[email protected]>

---
 hw/block/virtio-blk.c | 7 +++++++
 1 file changed, 7 insertions(+)

hw/block/virtio-blk.c +7 -0

index fb59f57..36c7608 100644
@@ -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));

Signature · 1 line

1.8.3.1

Stefan Hajnoczi

2017-12-11 10:30 UTC #9 lore raw

In reply to Mark Kanda

Quoted 7 lines · 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).

Quoted 8 lines

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

Mark Kanda

author

2017-12-11 15:15 UTC #10 lore raw

Quoted 3 lines · Stefan Hajnoczi

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