On Mon, Dec 23, 2019 at 09:44:46AM +0100, Paolo Bonzini wrote:
> On 23/12/19 09:28, Yang Zhong wrote:
> > In the guest kernel driver, like virtio_blk.c and virtio_scsi.c,
> > there are some definitions like below:
> >
> > num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs)
> >
> > If the queue number is bigger than vcpu number, the VM will be
> > stuck in the guest driver because the qemu and guest driver have
> > different queue number. So, this check can avoid this issues.
>
> Can you explain how the bug happens? This would be an issue in the
> kernel driver, QEMU shouldn't care about how the kernel driver chooses
> to steer requests to virtqueues.
>
Paolo, Merry Christmas!
This issue is easy to reproduce, when i enabled multiple queue with SPDK,
and if the num-queues > cpu number in the qemu command, the guest kernel
will stuck there and after guest kernel do "echo 0 >
/proc/sys/kernel/hung_task_timeout_secs"
, the guest kernel will crash.
My qemu command:
./qemu-system-x86_64 \
-machine q35,accel=kvm \
-cpu host -m 1024,maxmem=20G,slots=2 -smp 2 \
-chardev socket,id=char0,path=/var/tmp/vhost.1 \
-device
vhost-user-blk-pci,chardev=char0,num-queues=4,bootindex=2,config-wce=true \
......
My debug patch as below:
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 7ffd719d89de..b0a2fb503c00 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -511,11 +511,12 @@ static int init_vq(struct virtio_blk *vblk)
err = virtio_cread_feature(vdev, VIRTIO_BLK_F_MQ,
struct virtio_blk_config, num_queues,
&num_vqs);
+printk("---0------blk_probe and init_vq, the num_queues=%d---\n", num_vqs);
if (err)
num_vqs = 1;
-
+printk("---1------blk_probe and init_vq, the num_queues=%d---\n", num_vqs);
num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs);
-
+printk("---2------blk_probe and init_vq, the num_queues=%d---\n", num_vqs);
vblk->vqs = kmalloc_array(num_vqs, sizeof(*vblk->vqs), GFP_KERNEL);
The debug log:
[ 0.225780] Linux agpgart interface v0.103
[ 0.226908] loop: module loaded
[ 0.227079] ---0------blk_probe and init_vq, the num_queues=4---
[ 0.227192] ---1------blk_probe and init_vq, the num_queues=4---
[ 0.227260] ---2------blk_probe and init_vq, the num_queues=2---
[ 0.227813] virtio_blk virtio1: [vda] 1048576 512-byte logical blocks (537
MB/512 MiB)
In this time, the queue number in the front-end block driver is 2, but
the queue number in qemu side is still 4. So the guest virtio_blk
driver will failed to create vq with backend. There is no "set back"
mechnism for block driver to inform backend this new queue number.
So, i added this check in qemu side.
Since the current virtio-blk and vhost-user-blk device always
defaultly use 1 queue, it's hard to find this issue.
I checked the guest kernel driver, virtio-scsi and virtio-blk all
have same check in their driver probe:
num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs);
It's possible the guest driver has different queue number with qemu
side.
I also want to fix this issue from guest driver side, but currently there
is no better solution to fix this issue.
By the way, i did not try scsi with this corner case, and only check
driver and qemu code to find same issue. thanks!
Yang
> Paolo
>
> > Signed-off-by: Yang Zhong <yang.zh...@intel.com>
> > ---
> > hw/block/vhost-user-blk.c | 11 +++++++++++
> > hw/block/virtio-blk.c | 11 ++++++++++-
> > hw/scsi/virtio-scsi.c | 12 ++++++++++++
> > 3 files changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > index 63da9bb619..250e72abe4 100644
> > --- a/hw/block/vhost-user-blk.c
> > +++ b/hw/block/vhost-user-blk.c
> > @@ -23,6 +23,8 @@
> > #include "qom/object.h"
> > #include "hw/qdev-core.h"
> > #include "hw/qdev-properties.h"
> > +#include "qemu/option.h"
> > +#include "qemu/config-file.h"
> > #include "hw/virtio/vhost.h"
> > #include "hw/virtio/vhost-user-blk.h"
> > #include "hw/virtio/virtio.h"
> > @@ -391,6 +393,7 @@ static void vhost_user_blk_device_realize(DeviceState
> > *dev, Error **errp)
> > VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > VHostUserBlk *s = VHOST_USER_BLK(vdev);
> > Error *err = NULL;
> > + unsigned cpus;
> > int i, ret;
> >
> > if (!s->chardev.chr) {
> > @@ -403,6 +406,14 @@ static void vhost_user_blk_device_realize(DeviceState
> > *dev, Error **errp)
> > return;
> > }
> >
> > + cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"),
> > NULL),
> > + "cpus", 0);
> > + if (s->num_queues > cpus ) {
> > + error_setg(errp, "vhost-user-blk: the queue number should be equal
> > "
> > + "or less than vcpu number");
> > + return;
> > + }
> > +
> > if (!s->queue_size) {
> > error_setg(errp, "vhost-user-blk: queue size must be non-zero");
> > return;
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index d62e6377c2..b2f4d01148 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -18,6 +18,8 @@
> > #include "qemu/error-report.h"
> > #include "qemu/main-loop.h"
> > #include "trace.h"
> > +#include "qemu/option.h"
> > +#include "qemu/config-file.h"
> > #include "hw/block/block.h"
> > #include "hw/qdev-properties.h"
> > #include "sysemu/blockdev.h"
> > @@ -1119,7 +1121,7 @@ static void virtio_blk_device_realize(DeviceState
> > *dev, Error **errp)
> > VirtIOBlock *s = VIRTIO_BLK(dev);
> > VirtIOBlkConf *conf = &s->conf;
> > Error *err = NULL;
> > - unsigned i;
> > + unsigned i,cpus;
> >
> > if (!conf->conf.blk) {
> > error_setg(errp, "drive property not set");
> > @@ -1133,6 +1135,13 @@ static void virtio_blk_device_realize(DeviceState
> > *dev, Error **errp)
> > error_setg(errp, "num-queues property must be larger than 0");
> > return;
> > }
> > + cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"),
> > NULL),
> > + "cpus", 0);
> > + if (conf->num_queues > cpus ) {
> > + error_setg(errp, "virtio-blk: the queue number should be equal "
> > + "or less than vcpu number");
> > + return;
> > + }
> > if (!is_power_of_2(conf->queue_size) ||
> > conf->queue_size > VIRTQUEUE_MAX_SIZE) {
> > error_setg(errp, "invalid queue-size property (%" PRIu16 "), "
> > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> > index e8b2b64d09..8e3e44f6b9 100644
> > --- a/hw/scsi/virtio-scsi.c
> > +++ b/hw/scsi/virtio-scsi.c
> > @@ -21,6 +21,8 @@
> > #include "qemu/error-report.h"
> > #include "qemu/iov.h"
> > #include "qemu/module.h"
> > +#include "qemu/option.h"
> > +#include "qemu/config-file.h"
> > #include "sysemu/block-backend.h"
> > #include "hw/qdev-properties.h"
> > #include "hw/scsi/scsi.h"
> > @@ -880,6 +882,7 @@ void virtio_scsi_common_realize(DeviceState *dev,
> > {
> > VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(dev);
> > + unsigned cpus;
> > int i;
> >
> > virtio_init(vdev, "virtio-scsi", VIRTIO_ID_SCSI,
> > @@ -893,6 +896,15 @@ void virtio_scsi_common_realize(DeviceState *dev,
> > virtio_cleanup(vdev);
> > return;
> > }
> > +
> > + cpus = qemu_opt_get_number(qemu_opts_find(qemu_find_opts("smp-opts"),
> > NULL),
> > + "cpus", 0);
> > + if (s->conf.num_queues > cpus ) {
> > + error_setg(errp, "virtio-scsi: the queue number should be equal "
> > + "or less than vcpu number");
> > + return;
> > + }
> > +
> > s->cmd_vqs = g_new0(VirtQueue *, s->conf.num_queues);
> > s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
> > s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;
> >
