Re: [RFC 0/3] aio-posix: call ->poll_end() when removing AioHandler

[Hanna Czenczek <hreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3125 bytes --]

On 13.12.23 22:15, Stefan Hajnoczi wrote:
> Hanna and Fiona encountered a bug in aio_set_fd_handler(): there is no matching
> io_poll_end() call upon removing an AioHandler when io_poll_begin() was
> previously called. The missing io_poll_end() call leaves virtqueue
> notifications disabled and the virtqueue's ioeventfd will never become
> readable anymore.
>
> The details of how virtio-scsi devices using IOThreads can hang after
> hotplug/unplug are covered here:
> https://issues.redhat.com/browse/RHEL-3934
>
> Hanna is currently away over the December holidays. I'm sending these RFC
> patches in the meantime. They demonstrate running aio_set_fd_handler() in the
> AioContext home thread and adding the missing io_poll_end() call.

I agree with Paolo that if the handlers are removed, the caller probably 
isn’t interested in notifications: In our specific case, we remove the 
handlers because the device is to be drained, so we won’t poll the 
virtqueue anyway.  Therefore, if io_poll_end() is to be called to 
complete the start-end pair, it shouldn’t be done when the handlers are 
removed, but instead when they are reinstated.

I believe that’s quite infeasible to do in generic code: We’d have to 
basically remember that we haven’t called a specific io_poll_end 
callback yet, and so once it is reinstated while we’re not in a polling 
phase, we would need to call it then.  That in itself is already hairy, 
but in addition, the callback consists of a function pointer and an 
opaque pointer, and it seems impossible to reliably establish identity 
between two opaque pointers to know whether a newly instated io_poll_end 
callback is the same one as one that had been removed before (pointer 
identity may work, but also may not).

That’s why I think the responsibility should fall on the caller.  I 
believe both virtio_queue_aio_attach_host_notifier*() functions should 
enable vq notifications before instating the handler – if we’re in 
polling mode, io_poll_start() will then immediately be called and 
disable vq notifications again.  Having them enabled briefly shouldn’t 
hurt anything but performance (which I don’t think is terrible in the 
drain case).  For completeness’ sake, we may even have 
virtio_queue_aio_detach_host_notifier() disable vq notifications, 
because otherwise it’s unknown whether notifications are enabled or 
disabled after removing the notifiers.  That isn’t terrible, but I think 
(A) if any of the two, we want them to be disabled, because we won’t 
check for requests anyway, and (B) this gives us a clearer state 
machine, where removing the notifiers will always leave vq notifications 
disabled, so when they are reinstated (i.e. when calling 
virtio_queue_aio_attach_host_notifier*()), it’s clear that we must poll 
once to check for new requests.

I’ve attached the preliminary patch that I didn’t get to send (or test 
much) last year.  Not sure if it has the same CPU-usage-spike issue 
Fiona was seeing, the only functional difference is that I notify the vq 
after attaching the notifiers instead of before.

Hanna

[-- Attachment #2: 0001-Keep-notifications-disabled-during-drain.patch --]
[-- Type: text/x-patch, Size: 7354 bytes --]

From 451aae74fc19a6ea5cd6381247cd9202571651e8 Mon Sep 17 00:00:00 2001
From: Hanna Czenczek <hreitz@redhat.com>
Date: Wed, 6 Dec 2023 18:24:55 +0100
Subject: [PATCH] Keep notifications disabled during drain

Preliminary patch with a preliminary description: During drain, we do
not care about virtqueue notifications, which is why we remove the
handlers on it.  When removing those handlers, whether vq notifications
are enabled or not depends on whether we were in polling mode or not; if
not, they are enabled (by default); if so, they have been disabled by
the io_poll_start callback.

Ideally, we would rather have the vq notifications be disabled, because
we will not process requests during a drained section anyway.
Therefore, have virtio_queue_aio_detach_host_notifier() explicitly
disable them, and virtio_queue_aio_attach_host_notifier*() re-enable
them (if we are in a polling section, attaching them will invoke the
io_poll_start callback, which will re-disable them).

Because we will miss virtqueue updates in the drained section, we also
need to poll the virtqueue once in the drained_end functions after
attaching the notifiers.
---
 include/block/aio.h        |  7 ++++++-
 include/hw/virtio/virtio.h |  1 +
 hw/block/virtio-blk.c      | 10 ++++++++++
 hw/scsi/virtio-scsi.c      | 10 ++++++++++
 hw/virtio/virtio.c         | 30 +++++++++++++++++++++++++++++-
 5 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/include/block/aio.h b/include/block/aio.h
index f08b358077..795a375ff2 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -497,9 +497,14 @@ void aio_set_event_notifier(AioContext *ctx,
                             AioPollFn *io_poll,
                             EventNotifierHandler *io_poll_ready);
 
-/* Set polling begin/end callbacks for an event notifier that has already been
+/*
+ * Set polling begin/end callbacks for an event notifier that has already been
  * registered with aio_set_event_notifier.  Do nothing if the event notifier is
  * not registered.
+ *
+ * Note that if the io_poll_end() callback (or the entire notifier) is removed
+ * during polling, it will not be called, so an io_poll_begin() is not
+ * necessarily always followed by an io_poll_end().
  */
 void aio_set_event_notifier_poll(AioContext *ctx,
                                  EventNotifier *notifier,
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index c8f72850bc..64e66bea2d 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -335,6 +335,7 @@ void virtio_queue_update_rings(VirtIODevice *vdev, int n);
 void virtio_init_region_cache(VirtIODevice *vdev, int n);
 void virtio_queue_set_align(VirtIODevice *vdev, int n, int align);
 void virtio_queue_notify(VirtIODevice *vdev, int n);
+void virtio_queue_notify_vq(VirtQueue *vq);
 uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
 void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
 int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index a1f8e15522..68dad8cf48 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1538,6 +1538,16 @@ static void virtio_blk_drained_end(void *opaque)
     for (uint16_t i = 0; i < s->conf.num_queues; i++) {
         VirtQueue *vq = virtio_get_queue(vdev, i);
         virtio_queue_aio_attach_host_notifier(vq, ctx);
+
+        /*
+         * We will have ignored notifications about new requests from the guest
+         * during the drain, so "kick" the virt queue to process those requests
+         * now.
+         * Our .handle_output callback (virtio_blk_handle_output() ->
+         * virtio_blk_handle_vq()) acquires the AioContext, so this should be
+         * safe to call from any context.
+         */
+        virtio_queue_notify_vq(vq);
     }
 }
 
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 9c751bf296..9234bea7e8 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -1167,6 +1167,16 @@ static void virtio_scsi_drained_end(SCSIBus *bus)
     for (uint32_t i = 0; i < total_queues; i++) {
         VirtQueue *vq = virtio_get_queue(vdev, i);
         virtio_queue_aio_attach_host_notifier(vq, s->ctx);
+
+        /*
+         * We will have ignored notifications about new requests from the guest
+         * during the drain, so "kick" the virt queue to process those requests
+         * now.
+         * Our .handle_output callbacks (virtio_scsi_handle_{ctrl,cmd,vq}) use
+         * virtio_scsi_acquire(), so this should be safe to call from any
+         * context.
+         */
+        virtio_queue_notify_vq(vq);
     }
 }
 
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index e5105571cf..f515115069 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2255,7 +2255,7 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align)
     }
 }
 
-static void virtio_queue_notify_vq(VirtQueue *vq)
+void virtio_queue_notify_vq(VirtQueue *vq)
 {
     if (vq->vring.desc && vq->handle_output) {
         VirtIODevice *vdev = vq->vdev;
@@ -3556,6 +3556,17 @@ static void virtio_queue_host_notifier_aio_poll_end(EventNotifier *n)
 
 void virtio_queue_aio_attach_host_notifier(VirtQueue *vq, AioContext *ctx)
 {
+    /*
+     * virtio_queue_aio_detach_host_notifier() leaves notifications disabled.
+     * Re-enable them.  (And if detach has not been used before, notifications
+     * being enabled is still the default state while a notifier is attached;
+     * see virtio_queue_host_notifier_aio_poll_end(), which will always leave
+     * notifications enabled once the polling section is left.)
+     */
+    if (!virtio_queue_get_notification(vq)) {
+        virtio_queue_set_notification(vq, 1);
+    }
+
     aio_set_event_notifier(ctx, &vq->host_notifier,
                            virtio_queue_host_notifier_read,
                            virtio_queue_host_notifier_aio_poll,
@@ -3573,6 +3584,10 @@ void virtio_queue_aio_attach_host_notifier(VirtQueue *vq, AioContext *ctx)
  */
 void virtio_queue_aio_attach_host_notifier_no_poll(VirtQueue *vq, AioContext *ctx)
 {
+    if (!virtio_queue_get_notification(vq)) {
+        virtio_queue_set_notification(vq, 1);
+    }
+
     aio_set_event_notifier(ctx, &vq->host_notifier,
                            virtio_queue_host_notifier_read,
                            NULL, NULL);
@@ -3581,6 +3596,19 @@ void virtio_queue_aio_attach_host_notifier_no_poll(VirtQueue *vq, AioContext *ct
 void virtio_queue_aio_detach_host_notifier(VirtQueue *vq, AioContext *ctx)
 {
     aio_set_event_notifier(ctx, &vq->host_notifier, NULL, NULL, NULL);
+
+    /*
+     * aio_set_event_notifier_poll() does not guarantee whether io_poll_end()
+     * will run after io_poll_begin(), so by removing the notifier, we do not
+     * know whether virtio_queue_host_notifier_aio_poll_end() has run after a
+     * previous virtio_queue_host_notifier_aio_poll_begin(), i.e. whether
+     * notifications are enabled or disabled.  We just removed the notifier, so
+     * we may as well disable notifications.  The attach_host_notifier functions
+     * will re-enable them.
+     */
+    if (virtio_queue_get_notification(vq)) {
+        virtio_queue_set_notification(vq, 0);
+    }
 }
 
 void virtio_queue_host_notifier_read(EventNotifier *n)
-- 
2.43.0