On Fri, Oct 18, 2019 at 02:55:47PM +0300, Denis Plotnikov wrote: > From: "Denis V. Lunev" <d...@openvz.org> > > 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. > > Unfortunately the commit can not be applied as is. For the real cure we > need guest to be fixed to accomodate that queue length, which is done > only in the latest 4.14 kernel. Thus we are going to expose the property > and tweak it on machine type level. > > The problem with the old kernels is that they have > max_segments <= virtqueue_size restriction which cause the guest > crashing in the case of violation.
This isn't just in the guests: virtio spec also seems to imply this, or at least be vague on this point. So I think it'll need a feature bit. Doing that in a safe way will also allow being compatible with old guests. The only downside is it's a bit more work as we need to spec this out and add guest support. > To fix the case described above in the old kernels we can increase > virtqueue_size to 256 and max_segments to 254. The pitfall here is > that seabios allows the virtqueue_size-s < 128, however, the seabios > patch extending that value to 256 is pending. And the fix here is just to limit large vq size to virtio 1.0. In that mode it's fine I think: /* check if the queue is available */ if (vp->use_modern) { num = vp_read(&vp->common, virtio_pci_common_cfg, queue_size); if (num > MAX_QUEUE_NUM) { vp_write(&vp->common, virtio_pci_common_cfg, queue_size, MAX_QUEUE_NUM); num = vp_read(&vp->common, virtio_pci_common_cfg, queue_size); } } else { num = vp_read(&vp->legacy, virtio_pci_legacy, queue_num); } > CC: "Michael S. Tsirkin" <m...@redhat.com> > CC: Stefan Hajnoczi <stefa...@redhat.com> > CC: Kevin Wolf <kw...@redhat.com> > CC: Max Reitz <mre...@redhat.com> > CC: Gerd Hoffmann <kra...@redhat.com> > Signed-off-by: Denis V. Lunev <d...@openvz.org> > Signed-off-by: Denis Plotnikov <dplotni...@virtuozzo.com> > --- > hw/block/virtio-blk.c | 3 ++- > hw/scsi/vhost-scsi.c | 2 ++ > hw/scsi/virtio-scsi.c | 4 +++- > include/hw/virtio/virtio-blk.h | 1 + > include/hw/virtio/virtio-scsi.h | 1 + > 5 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 06e57a4d39..b2eaeeaf67 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -903,7 +903,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); > @@ -1240,6 +1240,7 @@ static Property virtio_blk_properties[] = { > conf.max_discard_sectors, BDRV_REQUEST_MAX_SECTORS), > DEFINE_PROP_UINT32("max-write-zeroes-sectors", VirtIOBlock, > conf.max_write_zeroes_sectors, > BDRV_REQUEST_MAX_SECTORS), > + DEFINE_PROP_UINT32("max_segments", VirtIOBlock, conf.max_segments, 126), > DEFINE_PROP_END_OF_LIST(), > }; > I'd worry that it's too easy to create a broken config with this parameter. > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c > index 61e2e57da9..fa3b377807 100644 > --- a/hw/scsi/vhost-scsi.c > +++ b/hw/scsi/vhost-scsi.c > @@ -242,6 +242,8 @@ static Property vhost_scsi_properties[] = { > DEFINE_PROP_BIT64("t10_pi", VHostSCSICommon, host_features, > VIRTIO_SCSI_F_T10_PI, > false), > + DEFINE_PROP_UINT32("max_segments", VirtIOSCSICommon, conf.max_segments, > + 126), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c > index 839f120256..8b070ddeed 100644 > --- a/hw/scsi/virtio-scsi.c > +++ b/hw/scsi/virtio-scsi.c > @@ -650,7 +650,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)); > @@ -948,6 +948,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, > + 126), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h > index cddcfbebe9..22da23a4a3 100644 > --- a/include/hw/virtio/virtio-blk.h > +++ b/include/hw/virtio/virtio-blk.h > @@ -40,6 +40,7 @@ struct VirtIOBlkConf > uint16_t queue_size; > uint32_t max_discard_sectors; > uint32_t max_write_zeroes_sectors; > + uint32_t max_segments; > }; > > struct VirtIOBlockDataPlane; > diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h > index 4c0bcdb788..1e5805eec4 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; > -- > 2.17.0