[PATCH 0/3] RFC: few random hacks to improve eventfd fallback path

[PATCH 0/3] RFC: few random hacks to improve eventfd fallback path

[Maxim Levitsky]

These few patches are the result of a random hacking I did to make the qemu
cope with eventfd allocation failure, when using an iothread,
as it happened in bz #1897550.

I am not 100% sure which patches in this series are worth to merge, or if
this can be fixed in a better way.

After this patch series applied, qemu still hangs while running reproducer for
this BZ due to ABBA lock inversion which needs some heavy rework to get rid of.
I explained all the (gory) details in the bugzilla.

This patch series was (lightly) tested with make check, iotests and with
the reproducer.

Best regards,
	Maxim Levitsky

Maxim Levitsky (3):
  scsi: virtio-scsi: don't process IO on fenced dataplane
  virtio-scsi: don't uninitialize queues that we didn't initialize
  event_notifier: handle initialization failure better

 hw/scsi/virtio-scsi-dataplane.c | 26 +++++++++++++++++++-------
 include/qemu/event_notifier.h   |  1 +
 util/event_notifier-posix.c     | 16 ++++++++++++++++
 3 files changed, 36 insertions(+), 7 deletions(-)

-- 
2.26.2

[PATCH 1/3] scsi: virtio-scsi: don't process IO on fenced dataplane

[Maxim Levitsky]

If virtio_scsi_dataplane_start fails, there is a small window when it drops the
aio lock (in aio_wait_bh_oneshot) and the dataplane's AIO handler can
still run during that window.

This is done after the dataplane was marked as fenced, thus we use this flag
to avoid it doing any IO.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 hw/scsi/virtio-scsi-dataplane.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index b995bab3a2..2824a25b65 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -56,8 +56,10 @@ static bool virtio_scsi_data_plane_handle_cmd(VirtIODevice *vdev,
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
     virtio_scsi_acquire(s);
-    assert(s->ctx && s->dataplane_started);
-    progress = virtio_scsi_handle_cmd_vq(s, vq);
+    if (!s->dataplane_fenced) {
+        assert(s->ctx && s->dataplane_started);
+        progress = virtio_scsi_handle_cmd_vq(s, vq);
+    }
     virtio_scsi_release(s);
     return progress;
 }
@@ -69,8 +71,10 @@ static bool virtio_scsi_data_plane_handle_ctrl(VirtIODevice *vdev,
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
     virtio_scsi_acquire(s);
-    assert(s->ctx && s->dataplane_started);
-    progress = virtio_scsi_handle_ctrl_vq(s, vq);
+    if (!s->dataplane_fenced) {
+        assert(s->ctx && s->dataplane_started);
+        progress = virtio_scsi_handle_ctrl_vq(s, vq);
+    }
     virtio_scsi_release(s);
     return progress;
 }
@@ -82,8 +86,10 @@ static bool virtio_scsi_data_plane_handle_event(VirtIODevice *vdev,
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
     virtio_scsi_acquire(s);
-    assert(s->ctx && s->dataplane_started);
-    progress = virtio_scsi_handle_event_vq(s, vq);
+    if (!s->dataplane_fenced) {
+        assert(s->ctx && s->dataplane_started);
+        progress = virtio_scsi_handle_event_vq(s, vq);
+    }
     virtio_scsi_release(s);
     return progress;
 }
-- 
2.26.2

[PATCH 2/3] virtio-scsi: don't uninitialize queues that we didn't initialize

[Maxim Levitsky]

Count number of queues that we initialized and only deinitialize these that we
initialized successfully.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 hw/scsi/virtio-scsi-dataplane.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 2824a25b65..cd1e72d3f8 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -132,6 +132,7 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
 {
     int i;
     int rc;
+    int vq_init_count = 0;
     BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
@@ -159,17 +160,22 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
     if (rc) {
         goto fail_vrings;
     }
+
+    vq_init_count++;
     rc = virtio_scsi_vring_init(s, vs->event_vq, 1,
                                 virtio_scsi_data_plane_handle_event);
     if (rc) {
         goto fail_vrings;
     }
+
+    vq_init_count++;
     for (i = 0; i < vs->conf.num_queues; i++) {
         rc = virtio_scsi_vring_init(s, vs->cmd_vqs[i], i + 2,
                                     virtio_scsi_data_plane_handle_cmd);
         if (rc) {
             goto fail_vrings;
         }
+        vq_init_count++;
     }
 
     s->dataplane_starting = false;
@@ -180,7 +186,7 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
 fail_vrings:
     aio_wait_bh_oneshot(s->ctx, virtio_scsi_dataplane_stop_bh, s);
     aio_context_release(s->ctx);
-    for (i = 0; i < vs->conf.num_queues + 2; i++) {
+    for (i = 0; i < vq_init_count; i++) {
         virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
         virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
     }
-- 
2.26.2

[PATCH 3/3] event_notifier: handle initialization failure better

[Maxim Levitsky]

Add 'initialized' field and use it to avoid touching event notifiers which are
either not initialized or if their initialization failed.

This is somewhat a hack, but it seems the less intrusive way to make
virtio code deal with event notifiers that failed initialization.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 include/qemu/event_notifier.h |  1 +
 util/event_notifier-posix.c   | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/qemu/event_notifier.h b/include/qemu/event_notifier.h
index 3380b662f3..b79add035d 100644
--- a/include/qemu/event_notifier.h
+++ b/include/qemu/event_notifier.h
@@ -24,6 +24,7 @@ struct EventNotifier {
 #else
     int rfd;
     int wfd;
+    bool initialized;
 #endif
 };
 
diff --git a/util/event_notifier-posix.c b/util/event_notifier-posix.c
index 00d93204f9..5b2110e861 100644
--- a/util/event_notifier-posix.c
+++ b/util/event_notifier-posix.c
@@ -29,6 +29,7 @@ void event_notifier_init_fd(EventNotifier *e, int fd)
 {
     e->rfd = fd;
     e->wfd = fd;
+    e->initialized = true;
 }
 #endif
 
@@ -68,6 +69,7 @@ int event_notifier_init(EventNotifier *e, int active)
     if (active) {
         event_notifier_set(e);
     }
+    e->initialized = true;
     return 0;
 
 fail:
@@ -78,12 +80,18 @@ fail:
 
 void event_notifier_cleanup(EventNotifier *e)
 {
+    if (!e->initialized) {
+        return;
+    }
+
     if (e->rfd != e->wfd) {
         close(e->rfd);
     }
+
     e->rfd = -1;
     close(e->wfd);
     e->wfd = -1;
+    e->initialized = false;
 }
 
 int event_notifier_get_fd(const EventNotifier *e)
@@ -96,6 +104,10 @@ int event_notifier_set(EventNotifier *e)
     static const uint64_t value = 1;
     ssize_t ret;
 
+    if (!e->initialized) {
+        return -1;
+    }
+
     do {
         ret = write(e->wfd, &value, sizeof(value));
     } while (ret < 0 && errno == EINTR);
@@ -113,6 +125,10 @@ int event_notifier_test_and_clear(EventNotifier *e)
     ssize_t len;
     char buffer[512];
 
+    if (!e->initialized) {
+        return 0;
+    }
+
     /* Drain the notify pipe.  For eventfd, only 8 bytes will be read.  */
     value = 0;
     do {
-- 
2.26.2

Re: [PATCH 0/3] RFC: few random hacks to improve eventfd fallback path

[Maxim Levitsky]

On Thu, 2020-12-17 at 17:00 +0200, Maxim Levitsky wrote:
> These few patches are the result of a random hacking I did to make the qemu
> cope with eventfd allocation failure, when using an iothread,
> as it happened in bz #1897550.
> 
> I am not 100% sure which patches in this series are worth to merge, or if
> this can be fixed in a better way.
> 
> After this patch series applied, qemu still hangs while running reproducer for
> this BZ due to ABBA lock inversion which needs some heavy rework to get rid of.
> I explained all the (gory) details in the bugzilla.
> 
> This patch series was (lightly) tested with make check, iotests and with
> the reproducer.
> 
> Best regards,
> 	Maxim Levitsky
> 
> Maxim Levitsky (3):
>   scsi: virtio-scsi: don't process IO on fenced dataplane
>   virtio-scsi: don't uninitialize queues that we didn't initialize
>   event_notifier: handle initialization failure better
> 
>  hw/scsi/virtio-scsi-dataplane.c | 26 +++++++++++++++++++-------
>  include/qemu/event_notifier.h   |  1 +
>  util/event_notifier-posix.c     | 16 ++++++++++++++++
>  3 files changed, 36 insertions(+), 7 deletions(-)
> 
> -- 
> 2.26.2
> 
> 
Any update on this?
Best regards,
	Maxim Levitsky

Re: [PATCH 0/3] RFC: few random hacks to improve eventfd fallback path

[Paolo Bonzini]

On 17/12/20 16:00, Maxim Levitsky wrote:
> These few patches are the result of a random hacking I did to make the qemu
> cope with eventfd allocation failure, when using an iothread,
> as it happened in bz #1897550.
> 
> I am not 100% sure which patches in this series are worth to merge, or if
> this can be fixed in a better way.
> 
> After this patch series applied, qemu still hangs while running reproducer for
> this BZ due to ABBA lock inversion which needs some heavy rework to get rid of.
> I explained all the (gory) details in the bugzilla.
> 
> This patch series was (lightly) tested with make check, iotests and with
> the reproducer.
> 
> Best regards,
> 	Maxim Levitsky
> 
> Maxim Levitsky (3):
>    scsi: virtio-scsi: don't process IO on fenced dataplane
>    virtio-scsi: don't uninitialize queues that we didn't initialize
>    event_notifier: handle initialization failure better
> 
>   hw/scsi/virtio-scsi-dataplane.c | 26 +++++++++++++++++++-------
>   include/qemu/event_notifier.h   |  1 +
>   util/event_notifier-posix.c     | 16 ++++++++++++++++
>   3 files changed, 36 insertions(+), 7 deletions(-)
> 

Queued, thanks.

Paolo