[Qemu-devel] [PATCH 0/3] vhost-user reconnect

[Qemu-devel] [PATCH 0/3] vhost-user reconnect

[Yury Kotov]

We are using QEMU (2.12.0) with SPDK (18.04.1) over vhost-user to emulate block
devices. One of our cases it to restart SPDK without restarting VM (in case
of some updates or smth like it). We tried to use the 'reconnect' option for
the '-chardev' device:
  -object memory-backend-file,id=mem0,size=1G,mem-path=/dev/hugepages,share=on \
  -numa node,memdev=mem0 \
  -chardev socket,id=spdk_vhost_blk1,path=/var/tmp/vhost.1,reconnect=10 \
  -device vhost-user-blk-pci,chardev=spdk_vhost_blk1,num-queues=4

After this, vhost-user-blk initialization fails with an error below:
  qemu-system-x86_64: -device ...: Failed to set msg fds.
  qemu-system-x86_64: -device ...: vhost-user-blk: vhost initialization failed:
                                   Operation not permitted

We got the same error with the latest QEMU (c542a9f9794ec8e0bc3f).

We made some investigations and found out that there are several issues:

1. Reconnect option postpones the first connection till machine init done event.
   But we need this connection during vhost blk device initialization which
   happens before the machine init done handling.

2. If the connection is forced, then the reconnection will be successful
   after SPDK restart. The problem is that virtual queue will not start.
   The reason for it is that virtual queue initialization commands
   should be resent:
   * VHOST_USER_SET_FEATURES
   * VHOST_USER_SET_MEM_TABLE
   * VHOST_USER_SET_VRING_NUM
   * VHOST_USER_SET_VRING_BASE
   * VHOST_USER_SET_VRING_ADDR
   * VHOST_USER_SET_VRING_KICK
   * VHOST_USER_SET_VRING_CALL

The patch set resolves both of these issues.

Test case:

1. Start fio process (inside VM):
     fio --name test --ioengine=libaio --iodepth=64 --bs=4096 \
         --rw=randrw --direct=1 --sync=1 --verify=md5 \
         --size=64M --filename=/dev/vda --loops=100

2. Restart SPDK many times.
   We are expecting that during SPDK restart fio will pause and fio should
   continue to work after restart completion.

3. fio process completed successfully without any error.

Yury Kotov (3):
  chardev: prevent extra connection attempt in tcp_chr_machine_done_hook
  vhost: refactor vhost_dev_start and vhost_virtqueue_start
  vhost-user: add reconnect support for vhost-user

 chardev/char-socket.c     |   5 +-
 hw/virtio/vhost-user.c    |  65 ++++++++++++--
 hw/virtio/vhost.c         | 223 +++++++++++++++++++++++++++++++---------------
 include/hw/virtio/vhost.h |   2 +
 4 files changed, 215 insertions(+), 80 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] chardev: prevent extra connection attempt in tcp_chr_machine_done_hook

[Yury Kotov]

Usually chardev connects to specified address during the initialization.
But if reconnect_time is specified then connection will be postponed until
machine done event.

Thus if reconnect is specified and some device forces connection during
initialization, tcp_chr_machine_done_hook will do useless connection attempt.

So add a check to prevent it.

Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
Signed-off-by: Evgeny Yakovlev <wrfsh@yandex-team.ru>
---
 chardev/char-socket.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index efbad6e..116dcc4 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1165,7 +1165,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
 
-    if (s->reconnect_time) {
+    /* It's possible that connection was established during the device
+     * initialization. So check if the socket is already connected to
+     * prevent extra connection attempt. */
+    if (!s->connected && s->reconnect_time) {
         tcp_chr_connect_async(chr);
     }
 
-- 
2.7.4

[Qemu-devel] [PATCH 2/3] vhost: refactor vhost_dev_start and vhost_virtqueue_start

[Yury Kotov]

vhost_dev_start and vhost_virtqueue_start do two things:
1. Initialize its own structs vhost_dev and vhost_virtqueue,
2. Sync with vhost backend.

It's ok, but we want to do sync part separately.
This refactoring is needed for the next patch, which adds reconnect
support for vhost-user.

So,
1. Move initialization part of vhost_dev_start which syncs with backend
   to the separate function: vhost_dev_sync_backend.
2. Divide vhost_virtqueue_start into two functions:
   * vhost_virtqueue_setup: prepares vhost_virtqueue to work with
     corresponding VirtQueue,
   * vhost_virtqueue_sync_backend: syncs vhost_virtqueue and backend.

Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
Signed-off-by: Evgeny Yakovlev <wrfsh@yandex-team.ru>
---
 hw/virtio/vhost.c         | 192 ++++++++++++++++++++++++++++------------------
 include/hw/virtio/vhost.h |   1 +
 2 files changed, 119 insertions(+), 74 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index d4cb589..6fcfb87 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -941,15 +941,14 @@ out:
     return ret;
 }
 
-static int vhost_virtqueue_start(struct vhost_dev *dev,
-                                struct VirtIODevice *vdev,
-                                struct vhost_virtqueue *vq,
-                                unsigned idx)
+static int vhost_virtqueue_sync_backend(struct vhost_dev *dev,
+                                        struct VirtIODevice *vdev,
+                                        struct vhost_virtqueue *vq,
+                                        unsigned idx)
 {
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
     VirtioBusState *vbus = VIRTIO_BUS(qbus);
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
-    hwaddr s, l, a;
     int r;
     int vhost_vq_index = dev->vhost_ops->vhost_get_vq_index(dev, idx);
     struct vhost_vring_file file = {
@@ -960,13 +959,7 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
     };
     struct VirtQueue *vvq = virtio_get_queue(vdev, idx);
 
-    a = virtio_queue_get_desc_addr(vdev, idx);
-    if (a == 0) {
-        /* Queue might not be ready for start */
-        return 0;
-    }
-
-    vq->num = state.num = virtio_queue_get_num(vdev, idx);
+    state.num = virtio_queue_get_num(vdev, idx);
     r = dev->vhost_ops->vhost_set_vring_num(dev, &state);
     if (r) {
         VHOST_OPS_DEBUG("vhost_set_vring_num failed");
@@ -989,32 +982,10 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
         }
     }
 
-    vq->desc_size = s = l = virtio_queue_get_desc_size(vdev, idx);
-    vq->desc_phys = a;
-    vq->desc = vhost_memory_map(dev, a, &l, 0);
-    if (!vq->desc || l != s) {
-        r = -ENOMEM;
-        goto fail_alloc_desc;
-    }
-    vq->avail_size = s = l = virtio_queue_get_avail_size(vdev, idx);
-    vq->avail_phys = a = virtio_queue_get_avail_addr(vdev, idx);
-    vq->avail = vhost_memory_map(dev, a, &l, 0);
-    if (!vq->avail || l != s) {
-        r = -ENOMEM;
-        goto fail_alloc_avail;
-    }
-    vq->used_size = s = l = virtio_queue_get_used_size(vdev, idx);
-    vq->used_phys = a = virtio_queue_get_used_addr(vdev, idx);
-    vq->used = vhost_memory_map(dev, a, &l, 1);
-    if (!vq->used || l != s) {
-        r = -ENOMEM;
-        goto fail_alloc_used;
-    }
-
     r = vhost_virtqueue_set_addr(dev, vq, vhost_vq_index, dev->log_enabled);
     if (r < 0) {
         r = -errno;
-        goto fail_alloc;
+        goto fail;
     }
 
     file.fd = event_notifier_get_fd(virtio_queue_get_host_notifier(vvq));
@@ -1022,7 +993,7 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
     if (r) {
         VHOST_OPS_DEBUG("vhost_set_vring_kick failed");
         r = -errno;
-        goto fail_kick;
+        goto fail;
     }
 
     /* Clear and discard previous events if any. */
@@ -1042,15 +1013,56 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
         file.fd = -1;
         r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
         if (r) {
-            goto fail_vector;
+            goto fail;
         }
     }
 
     return 0;
 
-fail_vector:
-fail_kick:
-fail_alloc:
+fail:
+    return r;
+}
+
+static int vhost_virtqueue_setup(struct vhost_dev *dev,
+                                 struct VirtIODevice *vdev,
+                                 struct vhost_virtqueue *vq,
+                                 unsigned idx)
+{
+    hwaddr s, l, a;
+    int r;
+
+    a = virtio_queue_get_desc_addr(vdev, idx);
+    if (a == 0) {
+        /* Queue might not be ready for start */
+        return 0;
+    }
+
+    vq->num = virtio_queue_get_num(vdev, idx);
+
+    vq->desc_size = s = l = virtio_queue_get_desc_size(vdev, idx);
+    vq->desc_phys = a;
+    vq->desc = vhost_memory_map(dev, a, &l, 0);
+    if (!vq->desc || l != s) {
+        r = -ENOMEM;
+        goto fail_alloc_desc;
+    }
+    vq->avail_size = s = l = virtio_queue_get_avail_size(vdev, idx);
+    vq->avail_phys = a = virtio_queue_get_avail_addr(vdev, idx);
+    vq->avail = vhost_memory_map(dev, a, &l, 0);
+    if (!vq->avail || l != s) {
+        r = -ENOMEM;
+        goto fail_alloc_avail;
+    }
+    vq->used_size = s = l = virtio_queue_get_used_size(vdev, idx);
+    vq->used_phys = a = virtio_queue_get_used_addr(vdev, idx);
+    vq->used = vhost_memory_map(dev, a, &l, 1);
+    if (!vq->used || l != s) {
+        r = -ENOMEM;
+        goto fail_alloc_used;
+    }
+
+    return 0;
+
     vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),
                        0, 0);
 fail_alloc_used:
@@ -1158,6 +1170,7 @@ static int vhost_virtqueue_init(struct vhost_dev *dev,
         return r;
     }
 
+    dev->vqs[n].masked = true;
     file.fd = event_notifier_get_fd(&vq->masked_notifier);
     r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
     if (r) {
@@ -1417,6 +1430,7 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
     } else {
         file.fd = event_notifier_get_fd(virtio_queue_get_guest_notifier(vvq));
     }
+    hdev->vqs[index].masked = mask;
 
     file.index = hdev->vhost_ops->vhost_get_vq_index(hdev, n);
     r = hdev->vhost_ops->vhost_set_vring_call(hdev, &file);
@@ -1483,56 +1497,41 @@ void vhost_dev_set_config_notifier(struct vhost_dev *hdev,
     hdev->config_ops = ops;
 }
 
-/* Host notifiers must be enabled at this point. */
-int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
+static int vhost_dev_sync_backend(struct vhost_dev *hdev)
 {
     int i, r;
-
-    /* should only be called after backend is connected */
     assert(hdev->vhost_ops);
-
-    hdev->started = true;
-    hdev->vdev = vdev;
+    assert(hdev->vdev);
 
     r = vhost_dev_set_features(hdev, hdev->log_enabled);
     if (r < 0) {
-        goto fail_features;
-    }
-
-    if (vhost_dev_has_iommu(hdev)) {
-        memory_listener_register(&hdev->iommu_listener, vdev->dma_as);
+        goto fail;
     }
 
     r = hdev->vhost_ops->vhost_set_mem_table(hdev, hdev->mem);
     if (r < 0) {
-        VHOST_OPS_DEBUG("vhost_set_mem_table failed");
         r = -errno;
-        goto fail_mem;
+        goto fail;
     }
+
     for (i = 0; i < hdev->nvqs; ++i) {
-        r = vhost_virtqueue_start(hdev,
-                                  vdev,
-                                  hdev->vqs + i,
-                                  hdev->vq_index + i);
+        r = vhost_virtqueue_sync_backend(hdev,
+                                         hdev->vdev,
+                                         hdev->vqs + i,
+                                         hdev->vq_index + i);
         if (r < 0) {
-            goto fail_vq;
+            goto fail;
         }
     }
 
     if (hdev->log_enabled) {
         uint64_t log_base;
-
-        hdev->log_size = vhost_get_log_size(hdev);
-        hdev->log = vhost_log_get(hdev->log_size,
-                                  vhost_dev_log_is_shared(hdev));
-        log_base = (uintptr_t)hdev->log->log;
-        r = hdev->vhost_ops->vhost_set_log_base(hdev,
-                                                hdev->log_size ? log_base : 0,
-                                                hdev->log);
+        assert(hdev->log);
+        log_base = hdev->log_size ? (uintptr_t)hdev->log->log : 0;
+        r = hdev->vhost_ops->vhost_set_log_base(hdev, log_base, hdev->log);
         if (r < 0) {
-            VHOST_OPS_DEBUG("vhost_set_log_base failed");
             r = -errno;
-            goto fail_log;
+            goto fail;
         }
     }
 
@@ -1546,20 +1545,65 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
             vhost_device_iotlb_miss(hdev, vq->used_phys, true);
         }
     }
+
     return 0;
-fail_log:
+
+fail:
+    return r;
+}
+
+/* Host notifiers must be enabled at this point. */
+int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
+{
+    int i, r;
+
+    /* should only be called after backend is connected */
+    assert(hdev->vhost_ops);
+
+    hdev->started = true;
+    hdev->vdev = vdev;
+
+    if (vhost_dev_has_iommu(hdev)) {
+        memory_listener_register(&hdev->iommu_listener, vdev->dma_as);
+    }
+
+    for (i = 0; i < hdev->nvqs; ++i) {
+        r = vhost_virtqueue_setup(hdev,
+                                  vdev,
+                                  hdev->vqs + i,
+                                  hdev->vq_index + i);
+        if (r < 0) {
+            goto fail_vq;
+        }
+    }
+
+    if (hdev->log_enabled) {
+        hdev->log_size = vhost_get_log_size(hdev);
+        hdev->log = vhost_log_get(hdev->log_size,
+                                  vhost_dev_log_is_shared(hdev));
+    }
+
+    r = vhost_dev_sync_backend(hdev);
+    if (r < 0) {
+        goto fail_sync;
+    }
+
+    return 0;
+
+fail_sync:
     vhost_log_put(hdev, false);
+
 fail_vq:
+    if (vhost_dev_has_iommu(hdev)) {
+        memory_listener_unregister(&hdev->iommu_listener);
+    }
+
     while (--i >= 0) {
         vhost_virtqueue_stop(hdev,
                              vdev,
                              hdev->vqs + i,
                              hdev->vq_index + i);
     }
-    i = hdev->nvqs;
-
-fail_mem:
-fail_features:
 
     hdev->started = false;
     return r;
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index a7f449f..a43db26 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -20,6 +20,7 @@ struct vhost_virtqueue {
     unsigned avail_size;
     unsigned long long used_phys;
     unsigned used_size;
+    bool masked;
     EventNotifier masked_notifier;
     struct vhost_dev *dev;
 };
-- 
2.7.4

[Qemu-devel] [PATCH 3/3] vhost-user: add reconnect support for vhost-user

[Yury Kotov]

Now, vhost device will stop if backend had restarted.
Even if we specify 'reconnect' parameter for chardev and connection will be
restored, vhost device will not be resumed.

To resume device we should sync with backend again after reconnect.

Add vhost_dev_reconnect extern function to vhost and add reconnect handler
to vhost-user which uses vhost_dev_reconnect to retry handshake with
vhost-user backend.

Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
Signed-off-by: Evgeny Yakovlev <wrfsh@yandex-team.ru>
---
 hw/virtio/vhost-user.c    | 65 +++++++++++++++++++++++++++++++++++++++++++----
 hw/virtio/vhost.c         | 31 ++++++++++++++++++++++
 include/hw/virtio/vhost.h |  1 +
 3 files changed, 92 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index b041343..5c7e113 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1121,6 +1121,17 @@ out:
     return ret;
 }
 
+static void vhost_close_slave_channel(struct vhost_dev *dev)
+{
+    struct vhost_user *u = dev->opaque;
+
+    if (u->slave_fd >= 0) {
+        qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL);
+        close(u->slave_fd);
+        u->slave_fd = -1;
+    }
+}
+
 /*
  * Called back from the postcopy fault thread when a fault is received on our
  * ufd.
@@ -1334,6 +1345,41 @@ static int vhost_user_postcopy_notifier(NotifierWithReturn *notifier,
     return 0;
 }
 
+static void vhost_user_reconnect_handler(void *opaque, int event)
+{
+    struct vhost_user *u = opaque;
+    struct vhost_dev *dev = u->dev;
+    int err;
+
+    if (!dev->started || event != CHR_EVENT_OPENED) {
+        return;
+    }
+
+    if (virtio_has_feature(dev->features, VHOST_USER_F_PROTOCOL_FEATURES)) {
+        err = vhost_user_set_protocol_features(dev, dev->protocol_features);
+        if (err < 0) {
+            goto fail;
+        }
+    }
+
+    vhost_close_slave_channel(dev);
+    err = vhost_setup_slave_channel(dev);
+    if (err < 0) {
+        goto fail;
+    }
+
+    err = vhost_dev_reconnect(dev);
+    if (err < 0) {
+        goto fail;
+    }
+
+    return;
+
+fail:
+    error_report("Failed to reconnect to backend: %d", err);
+    qemu_chr_fe_disconnect(u->user->chr);
+}
+
 static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
 {
     uint64_t features, protocol_features;
@@ -1348,6 +1394,19 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
     u->dev = dev;
     dev->opaque = u;
 
+    /* We expect the socket is already connected, but Chardev with reconnect
+     * option postpones connect till machine init done event. If this is the
+     * case, then the connect will be forced. */
+    if (!qemu_chr_fe_backend_open(u->user->chr) &&
+        qemu_chr_fe_wait_connected(u->user->chr, NULL) < 0) {
+        return -1;
+    }
+
+    /* Set reconnection handler. */
+    qemu_chr_fe_set_handlers(u->user->chr, NULL, NULL,
+                             vhost_user_reconnect_handler,
+                             NULL, u, NULL, false);
+
     err = vhost_user_get_features(dev, &features);
     if (err < 0) {
         return err;
@@ -1430,11 +1489,7 @@ static int vhost_user_backend_cleanup(struct vhost_dev *dev)
         postcopy_remove_notifier(&u->postcopy_notifier);
         u->postcopy_notifier.notify = NULL;
     }
-    if (u->slave_fd >= 0) {
-        qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL);
-        close(u->slave_fd);
-        u->slave_fd = -1;
-    }
+    vhost_close_slave_channel(dev);
     g_free(u->region_rb);
     u->region_rb = NULL;
     g_free(u->region_rb_offset);
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 6fcfb87..dbd496b 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1633,6 +1633,37 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
     hdev->vdev = NULL;
 }
 
+int vhost_dev_reconnect(struct vhost_dev *hdev)
+{
+    int i, r;
+
+    assert(hdev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
+    assert(hdev->started);
+    assert(hdev->vhost_ops);
+    assert(hdev->vdev);
+
+    for (i = 0; i < hdev->nvqs; ++i) {
+        /* Sync internal last avail idx to the device used idx. */
+        virtio_queue_restore_last_avail_idx(hdev->vdev, hdev->vq_index + i);
+    }
+
+    r = vhost_dev_sync_backend(hdev);
+    if (r < 0) {
+        goto fail;
+    }
+
+    /* Sync previous mask values */
+    for (i = 0; i < hdev->nvqs; ++i) {
+        unsigned idx = hdev->vq_index + i;
+        vhost_virtqueue_mask(hdev, hdev->vdev, idx, hdev->vqs[idx].masked);
+    }
+
+    return 0;
+
+fail:
+    return r;
+}
+
 int vhost_net_set_backend(struct vhost_dev *hdev,
                           struct vhost_vring_file *file)
 {
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index a43db26..c3d375a 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -91,6 +91,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
 void vhost_dev_cleanup(struct vhost_dev *hdev);
 int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
 void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
+int vhost_dev_reconnect(struct vhost_dev *hdev);
 int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
 void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/3] vhost-user reconnect

[Marc-André Lureau]

On Thu, Aug 16, 2018 at 5:32 PM, Yury Kotov <yury-kotov@yandex-team.ru> wrote:
> We are using QEMU (2.12.0) with SPDK (18.04.1) over vhost-user to emulate block
> devices. One of our cases it to restart SPDK without restarting VM (in case
> of some updates or smth like it). We tried to use the 'reconnect' option for
> the '-chardev' device:
>   -object memory-backend-file,id=mem0,size=1G,mem-path=/dev/hugepages,share=on \
>   -numa node,memdev=mem0 \
>   -chardev socket,id=spdk_vhost_blk1,path=/var/tmp/vhost.1,reconnect=10 \
>   -device vhost-user-blk-pci,chardev=spdk_vhost_blk1,num-queues=4
>
> After this, vhost-user-blk initialization fails with an error below:
>   qemu-system-x86_64: -device ...: Failed to set msg fds.
>   qemu-system-x86_64: -device ...: vhost-user-blk: vhost initialization failed:
>                                    Operation not permitted
>
> We got the same error with the latest QEMU (c542a9f9794ec8e0bc3f).
>
> We made some investigations and found out that there are several issues:
>
> 1. Reconnect option postpones the first connection till machine init done event.
>    But we need this connection during vhost blk device initialization which
>    happens before the machine init done handling.
>
> 2. If the connection is forced, then the reconnection will be successful
>    after SPDK restart. The problem is that virtual queue will not start.
>    The reason for it is that virtual queue initialization commands
>    should be resent:
>    * VHOST_USER_SET_FEATURES
>    * VHOST_USER_SET_MEM_TABLE
>    * VHOST_USER_SET_VRING_NUM
>    * VHOST_USER_SET_VRING_BASE
>    * VHOST_USER_SET_VRING_ADDR
>    * VHOST_USER_SET_VRING_KICK
>    * VHOST_USER_SET_VRING_CALL
>
> The patch set resolves both of these issues.
>
> Test case:
>
> 1. Start fio process (inside VM):
>      fio --name test --ioengine=libaio --iodepth=64 --bs=4096 \
>          --rw=randrw --direct=1 --sync=1 --verify=md5 \
>          --size=64M --filename=/dev/vda --loops=100
>
> 2. Restart SPDK many times.
>    We are expecting that during SPDK restart fio will pause and fio should
>    continue to work after restart completion.
>
> 3. fio process completed successfully without any error.

Can you write a test case in vhost-user-test.c ? (perhaps under
QTEST_VHOST_USER_FIXME scope...)

>
> Yury Kotov (3):
>   chardev: prevent extra connection attempt in tcp_chr_machine_done_hook
>   vhost: refactor vhost_dev_start and vhost_virtqueue_start
>   vhost-user: add reconnect support for vhost-user
>
>  chardev/char-socket.c     |   5 +-
>  hw/virtio/vhost-user.c    |  65 ++++++++++++--
>  hw/virtio/vhost.c         | 223 +++++++++++++++++++++++++++++++---------------
>  include/hw/virtio/vhost.h |   2 +
>  4 files changed, 215 insertions(+), 80 deletions(-)
>
> --
> 2.7.4
>

Re: [Qemu-devel] [PATCH 1/3] chardev: prevent extra connection attempt in tcp_chr_machine_done_hook

[Marc-André Lureau]

Hi

On Thu, Aug 16, 2018 at 5:32 PM, Yury Kotov <yury-kotov@yandex-team.ru> wrote:
> Usually chardev connects to specified address during the initialization.
> But if reconnect_time is specified then connection will be postponed until
> machine done event.
>
> Thus if reconnect is specified and some device forces connection during
> initialization, tcp_chr_machine_done_hook will do useless connection attempt.
>
> So add a check to prevent it.
>
> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
> Signed-off-by: Evgeny Yakovlev <wrfsh@yandex-team.ru>

It would be better with a test case in tests/test-char.c, but lgtm,

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  chardev/char-socket.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index efbad6e..116dcc4 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1165,7 +1165,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
>
> -    if (s->reconnect_time) {
> +    /* It's possible that connection was established during the device
> +     * initialization. So check if the socket is already connected to
> +     * prevent extra connection attempt. */
> +    if (!s->connected && s->reconnect_time) {
>          tcp_chr_connect_async(chr);
>      }
>
> --
> 2.7.4
>

Re: [Qemu-devel] [PATCH 0/3] vhost-user reconnect

[Marc-André Lureau]

Hi

On Thu, Aug 16, 2018 at 5:32 PM, Yury Kotov <yury-kotov@yandex-team.ru> wrote:
> We are using QEMU (2.12.0) with SPDK (18.04.1) over vhost-user to emulate block
> devices. One of our cases it to restart SPDK without restarting VM (in case
> of some updates or smth like it). We tried to use the 'reconnect' option for
> the '-chardev' device:
>   -object memory-backend-file,id=mem0,size=1G,mem-path=/dev/hugepages,share=on \
>   -numa node,memdev=mem0 \
>   -chardev socket,id=spdk_vhost_blk1,path=/var/tmp/vhost.1,reconnect=10 \
>   -device vhost-user-blk-pci,chardev=spdk_vhost_blk1,num-queues=4
>
> After this, vhost-user-blk initialization fails with an error below:
>   qemu-system-x86_64: -device ...: Failed to set msg fds.
>   qemu-system-x86_64: -device ...: vhost-user-blk: vhost initialization failed:
>                                    Operation not permitted
>
> We got the same error with the latest QEMU (c542a9f9794ec8e0bc3f).

Why not setup qemu socket chardev in server mode? This is the only
vhost-user reconnect setup that is supported atm (see
vhost-user-test.c). You avoid having a reconnect loop that way.

>
> We made some investigations and found out that there are several issues:
>
> 1. Reconnect option postpones the first connection till machine init done event.
>    But we need this connection during vhost blk device initialization which
>    happens before the machine init done handling.
>
> 2. If the connection is forced, then the reconnection will be successful
>    after SPDK restart. The problem is that virtual queue will not start.
>    The reason for it is that virtual queue initialization commands
>    should be resent:
>    * VHOST_USER_SET_FEATURES
>    * VHOST_USER_SET_MEM_TABLE
>    * VHOST_USER_SET_VRING_NUM
>    * VHOST_USER_SET_VRING_BASE
>    * VHOST_USER_SET_VRING_ADDR
>    * VHOST_USER_SET_VRING_KICK
>    * VHOST_USER_SET_VRING_CALL
>
> The patch set resolves both of these issues.
>
> Test case:
>
> 1. Start fio process (inside VM):
>      fio --name test --ioengine=libaio --iodepth=64 --bs=4096 \
>          --rw=randrw --direct=1 --sync=1 --verify=md5 \
>          --size=64M --filename=/dev/vda --loops=100
>
> 2. Restart SPDK many times.
>    We are expecting that during SPDK restart fio will pause and fio should
>    continue to work after restart completion.
>
> 3. fio process completed successfully without any error.
>
> Yury Kotov (3):
>   chardev: prevent extra connection attempt in tcp_chr_machine_done_hook
>   vhost: refactor vhost_dev_start and vhost_virtqueue_start
>   vhost-user: add reconnect support for vhost-user
>
>  chardev/char-socket.c     |   5 +-
>  hw/virtio/vhost-user.c    |  65 ++++++++++++--
>  hw/virtio/vhost.c         | 223 +++++++++++++++++++++++++++++++---------------
>  include/hw/virtio/vhost.h |   2 +
>  4 files changed, 215 insertions(+), 80 deletions(-)
>
> --
> 2.7.4
>

Re: [Qemu-devel] [PATCH 0/3] vhost-user reconnect

[Yury Kotov]

16.08.2018, 18:36, "Marc-André Lureau" <marcandre.lureau@redhat.com>:
> On Thu, Aug 16, 2018 at 5:32 PM, Yury Kotov <yury-kotov@yandex-team.ru> wrote:
>>  We are using QEMU (2.12.0) with SPDK (18.04.1) over vhost-user to emulate block
>>  devices. One of our cases it to restart SPDK without restarting VM (in case
>>  of some updates or smth like it). We tried to use the 'reconnect' option for
>>  the '-chardev' device:
>>    -object memory-backend-file,id=mem0,size=1G,mem-path=/dev/hugepages,share=on \
>>    -numa node,memdev=mem0 \
>>    -chardev socket,id=spdk_vhost_blk1,path=/var/tmp/vhost.1,reconnect=10 \
>>    -device vhost-user-blk-pci,chardev=spdk_vhost_blk1,num-queues=4
>>
>>  After this, vhost-user-blk initialization fails with an error below:
>>    qemu-system-x86_64: -device ...: Failed to set msg fds.
>>    qemu-system-x86_64: -device ...: vhost-user-blk: vhost initialization failed:
>>                                     Operation not permitted
>>
>>  We got the same error with the latest QEMU (c542a9f9794ec8e0bc3f).
>>
>>  We made some investigations and found out that there are several issues:
>>
>>  1. Reconnect option postpones the first connection till machine init done event.
>>     But we need this connection during vhost blk device initialization which
>>     happens before the machine init done handling.
>>
>>  2. If the connection is forced, then the reconnection will be successful
>>     after SPDK restart. The problem is that virtual queue will not start.
>>     The reason for it is that virtual queue initialization commands
>>     should be resent:
>>     * VHOST_USER_SET_FEATURES
>>     * VHOST_USER_SET_MEM_TABLE
>>     * VHOST_USER_SET_VRING_NUM
>>     * VHOST_USER_SET_VRING_BASE
>>     * VHOST_USER_SET_VRING_ADDR
>>     * VHOST_USER_SET_VRING_KICK
>>     * VHOST_USER_SET_VRING_CALL
>>
>>  The patch set resolves both of these issues.
>>
>>  Test case:
>>
>>  1. Start fio process (inside VM):
>>       fio --name test --ioengine=libaio --iodepth=64 --bs=4096 \
>>           --rw=randrw --direct=1 --sync=1 --verify=md5 \
>>           --size=64M --filename=/dev/vda --loops=100
>>
>>  2. Restart SPDK many times.
>>     We are expecting that during SPDK restart fio will pause and fio should
>>     continue to work after restart completion.
>>
>>  3. fio process completed successfully without any error.
>
> Can you write a test case in vhost-user-test.c ? (perhaps under
> QTEST_VHOST_USER_FIXME scope...)
>

This is a great idea and we were definitely going to do that during coming couple of weeks. We thought that we could make a follow up commit with necessary tests added a bit later though, since currently we need to figure out the state of vhost-user tests in general, before we can try to add any new stuff, and that will take some time. So far we have stress-tested these fixes manually.

Do you suggest we wait with this series as well until we have all tests ready? Or do we proceed now and make a follow up series with vhost user tests later like we suggested?

>>  Yury Kotov (3):
>>    chardev: prevent extra connection attempt in tcp_chr_machine_done_hook
>>    vhost: refactor vhost_dev_start and vhost_virtqueue_start
>>    vhost-user: add reconnect support for vhost-user
>>
>>   chardev/char-socket.c | 5 +-
>>   hw/virtio/vhost-user.c | 65 ++++++++++++--
>>   hw/virtio/vhost.c | 223 +++++++++++++++++++++++++++++++---------------
>>   include/hw/virtio/vhost.h | 2 +
>>   4 files changed, 215 insertions(+), 80 deletions(-)
>>
>>  --
>>  2.7.4

Re: [Qemu-devel] [PATCH 0/3] vhost-user reconnect

[Yury Kotov]

16.08.2018, 19:12, "Marc-André Lureau" <marcandre.lureau@redhat.com>:
> Hi
>
> On Thu, Aug 16, 2018 at 5:32 PM, Yury Kotov <yury-kotov@yandex-team.ru> wrote:
>>  We are using QEMU (2.12.0) with SPDK (18.04.1) over vhost-user to emulate block
>>  devices. One of our cases it to restart SPDK without restarting VM (in case
>>  of some updates or smth like it). We tried to use the 'reconnect' option for
>>  the '-chardev' device:
>>    -object memory-backend-file,id=mem0,size=1G,mem-path=/dev/hugepages,share=on \
>>    -numa node,memdev=mem0 \
>>    -chardev socket,id=spdk_vhost_blk1,path=/var/tmp/vhost.1,reconnect=10 \
>>    -device vhost-user-blk-pci,chardev=spdk_vhost_blk1,num-queues=4
>>
>>  After this, vhost-user-blk initialization fails with an error below:
>>    qemu-system-x86_64: -device ...: Failed to set msg fds.
>>    qemu-system-x86_64: -device ...: vhost-user-blk: vhost initialization failed:
>>                                     Operation not permitted
>>
>>  We got the same error with the latest QEMU (c542a9f9794ec8e0bc3f).
>
> Why not setup qemu socket chardev in server mode? This is the only
> vhost-user reconnect setup that is supported atm (see
> vhost-user-test.c). You avoid having a reconnect loop that way.
>

Yes, it will work. But client mode also should work and we want to support them both.

>>  We made some investigations and found out that there are several issues:
>>
>>  1. Reconnect option postpones the first connection till machine init done event.
>>     But we need this connection during vhost blk device initialization which
>>     happens before the machine init done handling.
>>
>>  2. If the connection is forced, then the reconnection will be successful
>>     after SPDK restart. The problem is that virtual queue will not start.
>>     The reason for it is that virtual queue initialization commands
>>     should be resent:
>>     * VHOST_USER_SET_FEATURES
>>     * VHOST_USER_SET_MEM_TABLE
>>     * VHOST_USER_SET_VRING_NUM
>>     * VHOST_USER_SET_VRING_BASE
>>     * VHOST_USER_SET_VRING_ADDR
>>     * VHOST_USER_SET_VRING_KICK
>>     * VHOST_USER_SET_VRING_CALL
>>
>>  The patch set resolves both of these issues.
>>
>>  Test case:
>>
>>  1. Start fio process (inside VM):
>>       fio --name test --ioengine=libaio --iodepth=64 --bs=4096 \
>>           --rw=randrw --direct=1 --sync=1 --verify=md5 \
>>           --size=64M --filename=/dev/vda --loops=100
>>
>>  2. Restart SPDK many times.
>>     We are expecting that during SPDK restart fio will pause and fio should
>>     continue to work after restart completion.
>>
>>  3. fio process completed successfully without any error.
>>
>>  Yury Kotov (3):
>>    chardev: prevent extra connection attempt in tcp_chr_machine_done_hook
>>    vhost: refactor vhost_dev_start and vhost_virtqueue_start
>>    vhost-user: add reconnect support for vhost-user
>>
>>   chardev/char-socket.c | 5 +-
>>   hw/virtio/vhost-user.c | 65 ++++++++++++--
>>   hw/virtio/vhost.c | 223 +++++++++++++++++++++++++++++++---------------
>>   include/hw/virtio/vhost.h | 2 +
>>   4 files changed, 215 insertions(+), 80 deletions(-)
>>
>>  --
>>  2.7.4

Re: [Qemu-devel] [PATCH 0/3] vhost-user reconnect

[Marc-André Lureau]

Hi

On Mon, Aug 20, 2018 at 2:51 PM, Yury Kotov <yury-kotov@yandex-team.ru> wrote:
> 16.08.2018, 18:36, "Marc-André Lureau" <marcandre.lureau@redhat.com>:
>> On Thu, Aug 16, 2018 at 5:32 PM, Yury Kotov <yury-kotov@yandex-team.ru> wrote:
>>>  We are using QEMU (2.12.0) with SPDK (18.04.1) over vhost-user to emulate block
>>>  devices. One of our cases it to restart SPDK without restarting VM (in case
>>>  of some updates or smth like it). We tried to use the 'reconnect' option for
>>>  the '-chardev' device:
>>>    -object memory-backend-file,id=mem0,size=1G,mem-path=/dev/hugepages,share=on \
>>>    -numa node,memdev=mem0 \
>>>    -chardev socket,id=spdk_vhost_blk1,path=/var/tmp/vhost.1,reconnect=10 \
>>>    -device vhost-user-blk-pci,chardev=spdk_vhost_blk1,num-queues=4
>>>
>>>  After this, vhost-user-blk initialization fails with an error below:
>>>    qemu-system-x86_64: -device ...: Failed to set msg fds.
>>>    qemu-system-x86_64: -device ...: vhost-user-blk: vhost initialization failed:
>>>                                     Operation not permitted
>>>
>>>  We got the same error with the latest QEMU (c542a9f9794ec8e0bc3f).
>>>
>>>  We made some investigations and found out that there are several issues:
>>>
>>>  1. Reconnect option postpones the first connection till machine init done event.
>>>     But we need this connection during vhost blk device initialization which
>>>     happens before the machine init done handling.
>>>
>>>  2. If the connection is forced, then the reconnection will be successful
>>>     after SPDK restart. The problem is that virtual queue will not start.
>>>     The reason for it is that virtual queue initialization commands
>>>     should be resent:
>>>     * VHOST_USER_SET_FEATURES
>>>     * VHOST_USER_SET_MEM_TABLE
>>>     * VHOST_USER_SET_VRING_NUM
>>>     * VHOST_USER_SET_VRING_BASE
>>>     * VHOST_USER_SET_VRING_ADDR
>>>     * VHOST_USER_SET_VRING_KICK
>>>     * VHOST_USER_SET_VRING_CALL
>>>
>>>  The patch set resolves both of these issues.
>>>
>>>  Test case:
>>>
>>>  1. Start fio process (inside VM):
>>>       fio --name test --ioengine=libaio --iodepth=64 --bs=4096 \
>>>           --rw=randrw --direct=1 --sync=1 --verify=md5 \
>>>           --size=64M --filename=/dev/vda --loops=100
>>>
>>>  2. Restart SPDK many times.
>>>     We are expecting that during SPDK restart fio will pause and fio should
>>>     continue to work after restart completion.
>>>
>>>  3. fio process completed successfully without any error.
>>
>> Can you write a test case in vhost-user-test.c ? (perhaps under
>> QTEST_VHOST_USER_FIXME scope...)
>>
>
> This is a great idea and we were definitely going to do that during coming couple of weeks. We thought that we could make a follow up commit with necessary tests added a bit later though, since currently we need to figure out the state of vhost-user tests in general, before we can try to add any new stuff, and that will take some time. So far we have stress-tested these fixes manually.

Yes, some vhost-user tests are disabled by default (sadly for travis
CI reason - not a really bug), and it's easy to introduce regressions.

I sent a related series "[PATCH 0/4] Fix socket chardev regression" to
make it work again.

> Do you suggest we wait with this series as well until we have all tests ready? Or do we proceed now and make a follow up series with vhost user tests later like we suggested?

I would rather have the tests with the series.

>
>>>  Yury Kotov (3):
>>>    chardev: prevent extra connection attempt in tcp_chr_machine_done_hook
>>>    vhost: refactor vhost_dev_start and vhost_virtqueue_start
>>>    vhost-user: add reconnect support for vhost-user
>>>
>>>   chardev/char-socket.c | 5 +-
>>>   hw/virtio/vhost-user.c | 65 ++++++++++++--
>>>   hw/virtio/vhost.c | 223 +++++++++++++++++++++++++++++++---------------
>>>   include/hw/virtio/vhost.h | 2 +
>>>   4 files changed, 215 insertions(+), 80 deletions(-)
>>>
>>>  --
>>>  2.7.4

Re: [Qemu-devel] [PATCH 0/3] vhost-user reconnect

[Yury Kotov]

20.08.2018, 16:11, "Marc-André Lureau" <marcandre.lureau@redhat.com>:
> Hi
>
> On Mon, Aug 20, 2018 at 2:51 PM, Yury Kotov <yury-kotov@yandex-team.ru> wrote:
>>  16.08.2018, 18:36, "Marc-André Lureau" <marcandre.lureau@redhat.com>:
>>>  On Thu, Aug 16, 2018 at 5:32 PM, Yury Kotov <yury-kotov@yandex-team.ru> wrote:
>>>>   We are using QEMU (2.12.0) with SPDK (18.04.1) over vhost-user to emulate block
>>>>   devices. One of our cases it to restart SPDK without restarting VM (in case
>>>>   of some updates or smth like it). We tried to use the 'reconnect' option for
>>>>   the '-chardev' device:
>>>>     -object memory-backend-file,id=mem0,size=1G,mem-path=/dev/hugepages,share=on \
>>>>     -numa node,memdev=mem0 \
>>>>     -chardev socket,id=spdk_vhost_blk1,path=/var/tmp/vhost.1,reconnect=10 \
>>>>     -device vhost-user-blk-pci,chardev=spdk_vhost_blk1,num-queues=4
>>>>
>>>>   After this, vhost-user-blk initialization fails with an error below:
>>>>     qemu-system-x86_64: -device ...: Failed to set msg fds.
>>>>     qemu-system-x86_64: -device ...: vhost-user-blk: vhost initialization failed:
>>>>                                      Operation not permitted
>>>>
>>>>   We got the same error with the latest QEMU (c542a9f9794ec8e0bc3f).
>>>>
>>>>   We made some investigations and found out that there are several issues:
>>>>
>>>>   1. Reconnect option postpones the first connection till machine init done event.
>>>>      But we need this connection during vhost blk device initialization which
>>>>      happens before the machine init done handling.
>>>>
>>>>   2. If the connection is forced, then the reconnection will be successful
>>>>      after SPDK restart. The problem is that virtual queue will not start.
>>>>      The reason for it is that virtual queue initialization commands
>>>>      should be resent:
>>>>      * VHOST_USER_SET_FEATURES
>>>>      * VHOST_USER_SET_MEM_TABLE
>>>>      * VHOST_USER_SET_VRING_NUM
>>>>      * VHOST_USER_SET_VRING_BASE
>>>>      * VHOST_USER_SET_VRING_ADDR
>>>>      * VHOST_USER_SET_VRING_KICK
>>>>      * VHOST_USER_SET_VRING_CALL
>>>>
>>>>   The patch set resolves both of these issues.
>>>>
>>>>   Test case:
>>>>
>>>>   1. Start fio process (inside VM):
>>>>        fio --name test --ioengine=libaio --iodepth=64 --bs=4096 \
>>>>            --rw=randrw --direct=1 --sync=1 --verify=md5 \
>>>>            --size=64M --filename=/dev/vda --loops=100
>>>>
>>>>   2. Restart SPDK many times.
>>>>      We are expecting that during SPDK restart fio will pause and fio should
>>>>      continue to work after restart completion.
>>>>
>>>>   3. fio process completed successfully without any error.
>>>
>>>  Can you write a test case in vhost-user-test.c ? (perhaps under
>>>  QTEST_VHOST_USER_FIXME scope...)
>>
>>  This is a great idea and we were definitely going to do that during coming couple of weeks. We thought that we could make a follow up commit with necessary tests added a bit later though, since currently we need to figure out the state of vhost-user tests in general, before we can try to add any new stuff, and that will take some time. So far we have stress-tested these fixes manually.
>
> Yes, some vhost-user tests are disabled by default (sadly for travis
> CI reason - not a really bug), and it's easy to introduce regressions.
>
> I sent a related series "[PATCH 0/4] Fix socket chardev regression" to
> make it work again.
>
>>  Do you suggest we wait with this series as well until we have all tests ready? Or do we proceed now and make a follow up series with vhost user tests later like we suggested?
>
> I would rather have the tests with the series.
>

Sounds good. We will resend v2 with tests. However while we do that, we would be
grateful for more comments on current implementation as well, since it at least
passes our internal functional tests.

>>>>   Yury Kotov (3):
>>>>     chardev: prevent extra connection attempt in tcp_chr_machine_done_hook
>>>>     vhost: refactor vhost_dev_start and vhost_virtqueue_start
>>>>     vhost-user: add reconnect support for vhost-user
>>>>
>>>>    chardev/char-socket.c | 5 +-
>>>>    hw/virtio/vhost-user.c | 65 ++++++++++++--
>>>>    hw/virtio/vhost.c | 223 +++++++++++++++++++++++++++++++---------------
>>>>    include/hw/virtio/vhost.h | 2 +
>>>>    4 files changed, 215 insertions(+), 80 deletions(-)
>>>>
>>>>   --
>>>>   2.7.4