[pve-devel] [PATCH v2 qemu] add patches to work around stuck guest IO with iothread and VirtIO block/SCSI

[pve-devel] [PATCH v2 qemu] add patches to work around stuck guest IO with iothread and VirtIO block/SCSI

[Fiona Ebner]

This essentially repeats commit 6b7c181 ("add patch to work around
stuck guest IO with iothread and VirtIO block/SCSI") with an added
fix for the SCSI event virtqueue, which requires special handling.
This is to avoid the issue [3] that made the revert 2a49e66 ("Revert
"add patch to work around stuck guest IO with iothread and VirtIO
block/SCSI"") necessary the first time around.

When using iothread, after commits
1665d9326f ("virtio-blk: implement BlockDevOps->drained_begin()")
766aa2de0f ("virtio-scsi: implement BlockDevOps->drained_begin()")
it can happen that polling gets stuck when draining. This would cause
IO in the guest to get completely stuck.

A workaround for users is stopping and resuming the vCPUs because that
would also stop and resume the dataplanes which would kick the host
notifiers.

This can happen with block jobs like backup and drive mirror as well
as with hotplug [2].

Reports in the community forum that might be about this issue[0][1]
and there is also one in the enterprise support channel.

As a workaround in the code, just re-enable notifications and kick the
virt queue after draining. Draining is already costly and rare, so no
need to worry about a performance penalty here.

Take special care to attach the SCSI event virtqueue host notifier
with the _no_poll() variant like in virtio_scsi_dataplane_start().
This avoids the issue from the first attempted fix where the iothread
would suddenly loop with 100% CPU usage whenever some guest IO came in
[3]. This is necessary because of commit 38738f7dbb ("virtio-scsi:
don't waste CPU polling the event virtqueue"). See [4] for the
relevant discussion.

[0]: https://forum.proxmox.com/threads/137286/
[1]: https://forum.proxmox.com/threads/137536/
[2]: https://issues.redhat.com/browse/RHEL-3934
[3]: https://forum.proxmox.com/threads/138140/
[4]: https://lore.kernel.org/qemu-devel/bfc7b20c-2144-46e9-acbc-e726276c5a31@proxmox.com/

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v2:
    * Pick (functionally equivalent) upstream patches to reduce diff.

 ...ttach-event-vq-notifier-with-no_poll.patch |  62 +++++++++
 ...-notifications-disabled-during-drain.patch | 126 ++++++++++++++++++
 debian/patches/series                         |   2 +
 3 files changed, 190 insertions(+)
 create mode 100644 debian/patches/extra/0012-virtio-scsi-Attach-event-vq-notifier-with-no_poll.patch
 create mode 100644 debian/patches/extra/0013-virtio-Keep-notifications-disabled-during-drain.patch

diff --git a/debian/patches/extra/0012-virtio-scsi-Attach-event-vq-notifier-with-no_poll.patch b/debian/patches/extra/0012-virtio-scsi-Attach-event-vq-notifier-with-no_poll.patch
new file mode 100644
index 0000000..8f060c4
--- /dev/null
+++ b/debian/patches/extra/0012-virtio-scsi-Attach-event-vq-notifier-with-no_poll.patch
@@ -0,0 +1,62 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Hanna Czenczek <hreitz@redhat.com>
+Date: Wed, 24 Jan 2024 18:38:29 +0100
+Subject: [PATCH] virtio-scsi: Attach event vq notifier with no_poll
+
+As of commit 38738f7dbbda90fbc161757b7f4be35b52205552 ("virtio-scsi:
+don't waste CPU polling the event virtqueue"), we only attach an io_read
+notifier for the virtio-scsi event virtqueue instead, and no polling
+notifiers.  During operation, the event virtqueue is typically
+non-empty, but none of the buffers are intended to be used immediately.
+Instead, they only get used when certain events occur.  Therefore, it
+makes no sense to continuously poll it when non-empty, because it is
+supposed to be and stay non-empty.
+
+We do this by using virtio_queue_aio_attach_host_notifier_no_poll()
+instead of virtio_queue_aio_attach_host_notifier() for the event
+virtqueue.
+
+Commit 766aa2de0f29b657148e04599320d771c36fd126 ("virtio-scsi: implement
+BlockDevOps->drained_begin()") however has virtio_scsi_drained_end() use
+virtio_queue_aio_attach_host_notifier() for all virtqueues, including
+the event virtqueue.  This can lead to it being polled again, undoing
+the benefit of commit 38738f7dbbda90fbc161757b7f4be35b52205552.
+
+Fix it by using virtio_queue_aio_attach_host_notifier_no_poll() for the
+event virtqueue.
+
+Reported-by: Fiona Ebner <f.ebner@proxmox.com>
+Fixes: 766aa2de0f29b657148e04599320d771c36fd126
+       ("virtio-scsi: implement BlockDevOps->drained_begin()")
+Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
+(picked from https://lists.nongnu.org/archive/html/qemu-devel/2024-01/msg04826.html)
+Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
+---
+ hw/scsi/virtio-scsi.c | 7 ++++++-
+ 1 file changed, 6 insertions(+), 1 deletion(-)
+
+diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
+index 45b95ea070..ad24a882fd 100644
+--- a/hw/scsi/virtio-scsi.c
++++ b/hw/scsi/virtio-scsi.c
+@@ -1148,6 +1148,7 @@ static void virtio_scsi_drained_begin(SCSIBus *bus)
+ static void virtio_scsi_drained_end(SCSIBus *bus)
+ {
+     VirtIOSCSI *s = container_of(bus, VirtIOSCSI, bus);
++    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
+     VirtIODevice *vdev = VIRTIO_DEVICE(s);
+     uint32_t total_queues = VIRTIO_SCSI_VQ_NUM_FIXED +
+                             s->parent_obj.conf.num_queues;
+@@ -1165,7 +1166,11 @@ 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);
++        if (vq == vs->event_vq) {
++            virtio_queue_aio_attach_host_notifier_no_poll(vq, s->ctx);
++        } else {
++            virtio_queue_aio_attach_host_notifier(vq, s->ctx);
++        }
+     }
+ }
+ 
diff --git a/debian/patches/extra/0013-virtio-Keep-notifications-disabled-during-drain.patch b/debian/patches/extra/0013-virtio-Keep-notifications-disabled-during-drain.patch
new file mode 100644
index 0000000..6b37623
--- /dev/null
+++ b/debian/patches/extra/0013-virtio-Keep-notifications-disabled-during-drain.patch
@@ -0,0 +1,126 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Hanna Czenczek <hreitz@redhat.com>
+Date: Wed, 24 Jan 2024 18:38:30 +0100
+Subject: [PATCH] virtio: Keep notifications disabled during drain
+
+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.
+
+Because we do not care about those notifications after removing the
+handlers, this is fine.  However, we have to explicitly ensure they are
+enabled when re-attaching the handlers, so we will resume receiving
+notifications.  We do this in virtio_queue_aio_attach_host_notifier*().
+If such a function is called while we are in a polling section,
+attaching the notifiers will then invoke the io_poll_start callback,
+re-disabling notifications.
+
+Because we will always miss virtqueue updates in the drained section, we
+also need to poll the virtqueue once after attaching the notifiers.
+
+Buglink: https://issues.redhat.com/browse/RHEL-3934
+Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
+(picked from https://lists.nongnu.org/archive/html/qemu-devel/2024-01/msg04827.html)
+Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
+---
+ hw/virtio/virtio.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
+ include/block/aio.h |  7 ++++++-
+ 2 files changed, 48 insertions(+), 1 deletion(-)
+
+diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
+index 969c25f4cf..24c24839e3 100644
+--- a/hw/virtio/virtio.c
++++ b/hw/virtio/virtio.c
+@@ -3526,6 +3526,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() can leave 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,
+@@ -3533,6 +3544,13 @@ void virtio_queue_aio_attach_host_notifier(VirtQueue *vq, AioContext *ctx)
+     aio_set_event_notifier_poll(ctx, &vq->host_notifier,
+                                 virtio_queue_host_notifier_aio_poll_begin,
+                                 virtio_queue_host_notifier_aio_poll_end);
++
++    /*
++     * We will have ignored notifications about new requests from the guest
++     * during the drain, so "kick" the virt queue to process those requests
++     * now.
++     */
++    virtio_queue_notify(vq->vdev, vq->queue_index);
+ }
+ 
+ /*
+@@ -3543,14 +3561,38 @@ void virtio_queue_aio_attach_host_notifier(VirtQueue *vq, AioContext *ctx)
+  */
+ void virtio_queue_aio_attach_host_notifier_no_poll(VirtQueue *vq, AioContext *ctx)
+ {
++    /* See virtio_queue_aio_attach_host_notifier() */
++    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);
++
++    /*
++     * See virtio_queue_aio_attach_host_notifier().
++     * Note that this may be unnecessary for the type of virtqueues this
++     * function is used for.  Still, it will not hurt to have a quick look into
++     * whether we can/should process any of the virtqueue elements.
++     */
++    virtio_queue_notify(vq->vdev, vq->queue_index);
+ }
+ 
+ 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.  It does not really matter anyway;
++     * we just removed the notifier, so we do not care about notifications until
++     * we potentially re-attach it.  The attach_host_notifier functions will
++     * ensure that notifications are enabled again when they are needed.
++     */
+ }
+ 
+ void virtio_queue_host_notifier_read(EventNotifier *n)
+diff --git a/include/block/aio.h b/include/block/aio.h
+index 32042e8905..79efadfa48 100644
+--- a/include/block/aio.h
++++ b/include/block/aio.h
+@@ -498,9 +498,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/debian/patches/series b/debian/patches/series
index b3da8bb..ab4ccad 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -9,6 +9,8 @@ extra/0008-Revert-x86-acpi-workaround-Windows-not-handling-name.patch
 extra/0009-hw-ide-ahci-fix-legacy-software-reset.patch
 extra/0010-ui-vnc-clipboard-fix-inflate_buffer.patch
 extra/0011-block-Fix-AioContext-locking-in-qmp_block_resize.patch
+extra/0012-virtio-scsi-Attach-event-vq-notifier-with-no_poll.patch
+extra/0013-virtio-Keep-notifications-disabled-during-drain.patch
 bitmap-mirror/0001-drive-mirror-add-support-for-sync-bitmap-mode-never.patch
 bitmap-mirror/0002-drive-mirror-add-support-for-conditional-and-always-.patch
 bitmap-mirror/0003-mirror-add-check-for-bitmap-mode-without-bitmap.patch
-- 
2.39.2

[pve-devel] applied: [PATCH v2 qemu] add patches to work around stuck guest IO with iothread and VirtIO block/SCSI

[Thomas Lamprecht]

Am 25/01/2024 um 10:40 schrieb Fiona Ebner:
> This essentially repeats commit 6b7c181 ("add patch to work around
> stuck guest IO with iothread and VirtIO block/SCSI") with an added
> fix for the SCSI event virtqueue, which requires special handling.
> This is to avoid the issue [3] that made the revert 2a49e66 ("Revert
> "add patch to work around stuck guest IO with iothread and VirtIO
> block/SCSI"") necessary the first time around.
> 
> When using iothread, after commits
> 1665d9326f ("virtio-blk: implement BlockDevOps->drained_begin()")
> 766aa2de0f ("virtio-scsi: implement BlockDevOps->drained_begin()")
> it can happen that polling gets stuck when draining. This would cause
> IO in the guest to get completely stuck.
> 
> A workaround for users is stopping and resuming the vCPUs because that
> would also stop and resume the dataplanes which would kick the host
> notifiers.
> 
> This can happen with block jobs like backup and drive mirror as well
> as with hotplug [2].
> 
> Reports in the community forum that might be about this issue[0][1]
> and there is also one in the enterprise support channel.
> 
> As a workaround in the code, just re-enable notifications and kick the
> virt queue after draining. Draining is already costly and rare, so no
> need to worry about a performance penalty here.
> 
> Take special care to attach the SCSI event virtqueue host notifier
> with the _no_poll() variant like in virtio_scsi_dataplane_start().
> This avoids the issue from the first attempted fix where the iothread
> would suddenly loop with 100% CPU usage whenever some guest IO came in
> [3]. This is necessary because of commit 38738f7dbb ("virtio-scsi:
> don't waste CPU polling the event virtqueue"). See [4] for the
> relevant discussion.
> 
> [0]: https://forum.proxmox.com/threads/137286/
> [1]: https://forum.proxmox.com/threads/137536/
> [2]: https://issues.redhat.com/browse/RHEL-3934
> [3]: https://forum.proxmox.com/threads/138140/
> [4]: https://lore.kernel.org/qemu-devel/bfc7b20c-2144-46e9-acbc-e726276c5a31@proxmox.com/
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Changes in v2:
>     * Pick (functionally equivalent) upstream patches to reduce diff.
> 
>  ...ttach-event-vq-notifier-with-no_poll.patch |  62 +++++++++
>  ...-notifications-disabled-during-drain.patch | 126 ++++++++++++++++++
>  debian/patches/series                         |   2 +
>  3 files changed, 190 insertions(+)
>  create mode 100644 debian/patches/extra/0012-virtio-scsi-Attach-event-vq-notifier-with-no_poll.patch
>  create mode 100644 debian/patches/extra/0013-virtio-Keep-notifications-disabled-during-drain.patch
> 
>

basically applied this, thanks.

Basically, as I went for the v2 [0], as the series file needed some adaption
 due to recent v8.1.5 patch I just made a new commit but kept your message and
added an Originally-by Trailer, hope that's all right with you.

[0]: https://lore.kernel.org/qemu-devel/20240202153158.788922-1-hreitz@redhat.com/
     But still only the first two patches, the clean-up did not applied at 8.1.5 
     and I did not bother checking that out closely.

Re: [pve-devel] applied: [PATCH v2 qemu] add patches to work around stuck guest IO with iothread and VirtIO block/SCSI

[Fiona Ebner]

Am 02.02.24 um 19:42 schrieb Thomas Lamprecht:
> 
> basically applied this, thanks.
> 
> Basically, as I went for the v2 [0], as the series file needed some adaption
>  due to recent v8.1.5 patch I just made a new commit but kept your message and
> added an Originally-by Trailer, hope that's all right with you.
> 

Yes, sure. Wanted to send a new version anyways :)

While the diff to v1 is pretty small, I did a quick test again and found
no obvious issue, while it still fixes my reproducer.

> [0]: https://lore.kernel.org/qemu-devel/20240202153158.788922-1-hreitz@redhat.com/
>      But still only the first two patches, the clean-up did not applied at 8.1.5 
>      and I did not bother checking that out closely.

Yes, there were some changes in the meantime and while a backport might
be removing the event_notifier_set() call from
virtio_blk_data_plane_start(), with the current code, there also is a
smp_wmb() and aio_context_acquire(s->ctx) in between
event_notifier_set() and virtio_queue_aio_attach_host_notifier(), so not
sure if that's okay in all scenarios. I don't think the backport is
necessary, because setting the event notifier twice will just kick the
virt queue twice.