[v2,1/1] virtio: fix IO request length in virtio SCSI/block
diff mbox

Message ID 1513260561-28130-1-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev Dec. 14, 2017, 2:09 p.m. UTC
Linux guests submit IO requests no longer than PAGE_SIZE * max_seg
field reported by SCSI controler. Thus typical sequential read with
1 MB size results in the following pattern of the IO from the guest:
  8,16   1    15754     2.766095122  2071  D   R 2095104 + 1008 [dd]
  8,16   1    15755     2.766108785  2071  D   R 2096112 + 1008 [dd]
  8,16   1    15756     2.766113486  2071  D   R 2097120 + 32 [dd]
  8,16   1    15757     2.767668961     0  C   R 2095104 + 1008 [0]
  8,16   1    15758     2.768534315     0  C   R 2096112 + 1008 [0]
  8,16   1    15759     2.768539782     0  C   R 2097120 + 32 [0]
The IO was generated by
  dd if=/dev/sda of=/dev/null bs=1024 iflag=direct

This effectively means that on rotational disks we will observe 3 IOPS
for each 2 MBs processed. This definitely negatively affects both
guest and host IO performance.

The cure is relatively simple - we should report lengthy scatter-gather
ability of the SCSI controller. Fortunately the situation here is very
good. VirtIO transport layer can accomodate 1024 items in one request
while we are using only 128. This situation is present since almost
very beginning. 2 items are dedicated for request metadata thus we
should publish VIRTQUEUE_MAX_SIZE - 2 as max_seg.

The following pattern is observed after the patch:
  8,16   1     9921     2.662721340  2063  D   R 2095104 + 1024 [dd]
  8,16   1     9922     2.662737585  2063  D   R 2096128 + 1024 [dd]
  8,16   1     9923     2.665188167     0  C   R 2095104 + 1024 [0]
  8,16   1     9924     2.665198777     0  C   R 2096128 + 1024 [0]
which is much better.

The dark side of this patch is that we are tweaking guest visible
parameter, though this should be relatively safe as above transport
layer support is present in QEMU/host Linux for a very long time.
The patch adds configurable property for VirtIO SCSI with a new default
and hardcode option for VirtBlock which does not provide good
configurable framework.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: "Michael S. Tsirkin" <mst@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
---
Changes from v1:
- added max_segments property for virtblock device

 include/hw/virtio/virtio-blk.h  | 1 +
 include/hw/virtio/virtio-scsi.h | 1 +
 hw/block/virtio-blk.c           | 4 +++-
 hw/scsi/vhost-scsi.c            | 2 ++
 hw/scsi/vhost-user-scsi.c       | 2 ++
 hw/scsi/virtio-scsi.c           | 4 +++-
 6 files changed, 12 insertions(+), 2 deletions(-)

Comments

Paolo Bonzini Dec. 14, 2017, 5:52 p.m. UTC | #1
On 14/12/2017 15:09, Denis V. Lunev wrote:
> Linux guests submit IO requests no longer than PAGE_SIZE * max_seg
> field reported by SCSI controler. Thus typical sequential read with
> 1 MB size results in the following pattern of the IO from the guest:
>   8,16   1    15754     2.766095122  2071  D   R 2095104 + 1008 [dd]
>   8,16   1    15755     2.766108785  2071  D   R 2096112 + 1008 [dd]
>   8,16   1    15756     2.766113486  2071  D   R 2097120 + 32 [dd]
>   8,16   1    15757     2.767668961     0  C   R 2095104 + 1008 [0]
>   8,16   1    15758     2.768534315     0  C   R 2096112 + 1008 [0]
>   8,16   1    15759     2.768539782     0  C   R 2097120 + 32 [0]
> The IO was generated by
>   dd if=/dev/sda of=/dev/null bs=1024 iflag=direct
> 
> This effectively means that on rotational disks we will observe 3 IOPS
> for each 2 MBs processed. This definitely negatively affects both
> guest and host IO performance.
> 
> The cure is relatively simple - we should report lengthy scatter-gather
> ability of the SCSI controller. Fortunately the situation here is very
> good. VirtIO transport layer can accomodate 1024 items in one request
> while we are using only 128. This situation is present since almost
> very beginning. 2 items are dedicated for request metadata thus we
> should publish VIRTQUEUE_MAX_SIZE - 2 as max_seg.
> 
> The following pattern is observed after the patch:
>   8,16   1     9921     2.662721340  2063  D   R 2095104 + 1024 [dd]
>   8,16   1     9922     2.662737585  2063  D   R 2096128 + 1024 [dd]
>   8,16   1     9923     2.665188167     0  C   R 2095104 + 1024 [0]
>   8,16   1     9924     2.665198777     0  C   R 2096128 + 1024 [0]
> which is much better.
> 
> The dark side of this patch is that we are tweaking guest visible
> parameter, though this should be relatively safe as above transport
> layer support is present in QEMU/host Linux for a very long time.
> The patch adds configurable property for VirtIO SCSI with a new default
> and hardcode option for VirtBlock which does not provide good
> configurable framework.

The patch is still missing compat properties (and 2.12 machine types).
If you don't want to add the machine types, feel free to wait until
someone else does it. :)

Paolo

> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: "Michael S. Tsirkin" <mst@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> ---
> Changes from v1:
> - added max_segments property for virtblock device
> 
>  include/hw/virtio/virtio-blk.h  | 1 +
>  include/hw/virtio/virtio-scsi.h | 1 +
>  hw/block/virtio-blk.c           | 4 +++-
>  hw/scsi/vhost-scsi.c            | 2 ++
>  hw/scsi/vhost-user-scsi.c       | 2 ++
>  hw/scsi/virtio-scsi.c           | 4 +++-
>  6 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index d3c8a6f..0aa83a3 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;
> +    uint32_t max_segments;
>  };
>  
>  struct VirtIOBlockDataPlane;
> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index 4c0bcdb..1e5805e 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -49,6 +49,7 @@ struct VirtIOSCSIConf {
>      uint32_t num_queues;
>      uint32_t virtqueue_size;
>      uint32_t max_sectors;
> +    uint32_t max_segments;
>      uint32_t cmd_per_lun;
>  #ifdef CONFIG_VHOST_SCSI
>      char *vhostfd;
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 05d1440..99da3b6 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -736,7 +736,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>      blk_get_geometry(s->blk, &capacity);
>      memset(&blkcfg, 0, sizeof(blkcfg));
>      virtio_stq_p(vdev, &blkcfg.capacity, capacity);
> -    virtio_stl_p(vdev, &blkcfg.seg_max, 128 - 2);
> +    virtio_stl_p(vdev, &blkcfg.seg_max, s->conf.max_segments);
>      virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls);
>      virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
>      virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
> @@ -1014,6 +1014,8 @@ static Property virtio_blk_properties[] = {
>      DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1),
>      DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
>                       IOThread *),
> +    DEFINE_PROP_UINT32("max_segments", VirtIOBlock, conf.max_segments,
> +                       VIRTQUEUE_MAX_SIZE - 2),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 9c1bea8..f93eac6 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -238,6 +238,8 @@ static Property vhost_scsi_properties[] = {
>      DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon, conf.max_sectors,
>                         0xFFFF),
>      DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSICommon, conf.cmd_per_lun, 128),
> +    DEFINE_PROP_UINT32("max_segments", VirtIOSCSICommon, conf.max_segments,
> +                       VIRTQUEUE_MAX_SIZE - 2),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index f7561e2..8b02ab1 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -146,6 +146,8 @@ static Property vhost_user_scsi_properties[] = {
>      DEFINE_PROP_BIT64("param_change", VHostUserSCSI, host_features,
>                                                       VIRTIO_SCSI_F_CHANGE,
>                                                       true),
> +    DEFINE_PROP_UINT32("max_segments", VirtIOSCSICommon, conf.max_segments,
> +                       VIRTQUEUE_MAX_SIZE - 2),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 3aa9971..5404dde 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -644,7 +644,7 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
>      VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
>  
>      virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues);
> -    virtio_stl_p(vdev, &scsiconf->seg_max, 128 - 2);
> +    virtio_stl_p(vdev, &scsiconf->seg_max, s->conf.max_segments);
>      virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors);
>      virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
>      virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
> @@ -929,6 +929,8 @@ static Property virtio_scsi_properties[] = {
>                                                  VIRTIO_SCSI_F_CHANGE, true),
>      DEFINE_PROP_LINK("iothread", VirtIOSCSI, parent_obj.conf.iothread,
>                       TYPE_IOTHREAD, IOThread *),
> +    DEFINE_PROP_UINT32("max_segments", VirtIOSCSI, parent_obj.conf.max_segments,
> +                       VIRTQUEUE_MAX_SIZE - 2),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
>
Denis V. Lunev Dec. 14, 2017, 6:13 p.m. UTC | #2
On 12/14/2017 08:52 PM, Paolo Bonzini wrote:
> On 14/12/2017 15:09, Denis V. Lunev wrote:
>> Linux guests submit IO requests no longer than PAGE_SIZE * max_seg
>> field reported by SCSI controler. Thus typical sequential read with
>> 1 MB size results in the following pattern of the IO from the guest:
>>   8,16   1    15754     2.766095122  2071  D   R 2095104 + 1008 [dd]
>>   8,16   1    15755     2.766108785  2071  D   R 2096112 + 1008 [dd]
>>   8,16   1    15756     2.766113486  2071  D   R 2097120 + 32 [dd]
>>   8,16   1    15757     2.767668961     0  C   R 2095104 + 1008 [0]
>>   8,16   1    15758     2.768534315     0  C   R 2096112 + 1008 [0]
>>   8,16   1    15759     2.768539782     0  C   R 2097120 + 32 [0]
>> The IO was generated by
>>   dd if=/dev/sda of=/dev/null bs=1024 iflag=direct
>>
>> This effectively means that on rotational disks we will observe 3 IOPS
>> for each 2 MBs processed. This definitely negatively affects both
>> guest and host IO performance.
>>
>> The cure is relatively simple - we should report lengthy scatter-gather
>> ability of the SCSI controller. Fortunately the situation here is very
>> good. VirtIO transport layer can accomodate 1024 items in one request
>> while we are using only 128. This situation is present since almost
>> very beginning. 2 items are dedicated for request metadata thus we
>> should publish VIRTQUEUE_MAX_SIZE - 2 as max_seg.
>>
>> The following pattern is observed after the patch:
>>   8,16   1     9921     2.662721340  2063  D   R 2095104 + 1024 [dd]
>>   8,16   1     9922     2.662737585  2063  D   R 2096128 + 1024 [dd]
>>   8,16   1     9923     2.665188167     0  C   R 2095104 + 1024 [0]
>>   8,16   1     9924     2.665198777     0  C   R 2096128 + 1024 [0]
>> which is much better.
>>
>> The dark side of this patch is that we are tweaking guest visible
>> parameter, though this should be relatively safe as above transport
>> layer support is present in QEMU/host Linux for a very long time.
>> The patch adds configurable property for VirtIO SCSI with a new default
>> and hardcode option for VirtBlock which does not provide good
>> configurable framework.
> The patch is still missing compat properties (and 2.12 machine types).
> If you don't want to add the machine types, feel free to wait until
> someone else does it. :)
>
> Paolo

sorry :( my fault. Will re-spin tomorrow.

Patch
diff mbox

diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index d3c8a6f..0aa83a3 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;
+    uint32_t max_segments;
 };
 
 struct VirtIOBlockDataPlane;
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 4c0bcdb..1e5805e 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -49,6 +49,7 @@  struct VirtIOSCSIConf {
     uint32_t num_queues;
     uint32_t virtqueue_size;
     uint32_t max_sectors;
+    uint32_t max_segments;
     uint32_t cmd_per_lun;
 #ifdef CONFIG_VHOST_SCSI
     char *vhostfd;
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 05d1440..99da3b6 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -736,7 +736,7 @@  static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
     blk_get_geometry(s->blk, &capacity);
     memset(&blkcfg, 0, sizeof(blkcfg));
     virtio_stq_p(vdev, &blkcfg.capacity, capacity);
-    virtio_stl_p(vdev, &blkcfg.seg_max, 128 - 2);
+    virtio_stl_p(vdev, &blkcfg.seg_max, s->conf.max_segments);
     virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls);
     virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
     virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
@@ -1014,6 +1014,8 @@  static Property virtio_blk_properties[] = {
     DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1),
     DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
                      IOThread *),
+    DEFINE_PROP_UINT32("max_segments", VirtIOBlock, conf.max_segments,
+                       VIRTQUEUE_MAX_SIZE - 2),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 9c1bea8..f93eac6 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -238,6 +238,8 @@  static Property vhost_scsi_properties[] = {
     DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon, conf.max_sectors,
                        0xFFFF),
     DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSICommon, conf.cmd_per_lun, 128),
+    DEFINE_PROP_UINT32("max_segments", VirtIOSCSICommon, conf.max_segments,
+                       VIRTQUEUE_MAX_SIZE - 2),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index f7561e2..8b02ab1 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -146,6 +146,8 @@  static Property vhost_user_scsi_properties[] = {
     DEFINE_PROP_BIT64("param_change", VHostUserSCSI, host_features,
                                                      VIRTIO_SCSI_F_CHANGE,
                                                      true),
+    DEFINE_PROP_UINT32("max_segments", VirtIOSCSICommon, conf.max_segments,
+                       VIRTQUEUE_MAX_SIZE - 2),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 3aa9971..5404dde 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -644,7 +644,7 @@  static void virtio_scsi_get_config(VirtIODevice *vdev,
     VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
 
     virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues);
-    virtio_stl_p(vdev, &scsiconf->seg_max, 128 - 2);
+    virtio_stl_p(vdev, &scsiconf->seg_max, s->conf.max_segments);
     virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors);
     virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
     virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
@@ -929,6 +929,8 @@  static Property virtio_scsi_properties[] = {
                                                 VIRTIO_SCSI_F_CHANGE, true),
     DEFINE_PROP_LINK("iothread", VirtIOSCSI, parent_obj.conf.iothread,
                      TYPE_IOTHREAD, IOThread *),
+    DEFINE_PROP_UINT32("max_segments", VirtIOSCSI, parent_obj.conf.max_segments,
+                       VIRTQUEUE_MAX_SIZE - 2),
     DEFINE_PROP_END_OF_LIST(),
 };