[PATCH 0/2] virtio: Keep notifications disabled during drain

[PATCH 0/2] virtio: Keep notifications disabled during drain

[Hanna Czenczek]

Hi,

When registering callbacks via aio_set_event_notifier_poll(), the
io_poll_end() callback is only invoked when polling actually ends.  If
the notifiers are removed while in a polling section, it is not called.
Therefore, io_poll_start() is not necessarily followed up by
io_poll_end().

It is not entirely clear whether this is good or bad behavior.  On one
hand, it may be unexpected to callers.  On the other, it may be
counterproductive to call io_poll_end() when the polling section has not
ended yet.

Right now, there is only one user of aio_set_event_notifier(), which is
virtio_queue_aio_attach_host_notifier().  It does not expect this
behavior, which leads to virtqueue notifiers remaining disabled if
virtio_queue_aio_detach_host_notifier() is called while polling.  That
can happen e.g. through virtio_scsi_drained_begin() or
virtio_blk_drained_begin() (through virtio_blk_data_plane_detach()).
In such a case, the virtqueue may not be processed for a while, letting
the guest driver hang.  This can be reproduced by repeatedly
hot-plugging and -unplugging a virtio-scsi device with a scsi-hd disk,
because the guest will try to enumerate the virtio-scsi device while
we’re attaching the scsi-hd disk, which causes a drain, which can cause
the virtio-scsi virtqueue to stall as described.

Stefan has suggested ensuring we always follow up io_poll_start() by
io_poll_end():

https://lists.nongnu.org/archive/html/qemu-block/2023-12/msg00163.html

I prefer changing the caller instead, because I don’t think we actually
want the virtqueue notifier to be force-enabled when removing our AIO
notifiers.  So I believe we actually only want to take care to
force-enable it when we re-attach the AIO notifiers, and to kick
virtqueue processing once, in case we missed any events while the AIO
notifiers were not attached.

That is done by patch 2.  We have already discussed a prior version of
it here:

https://lists.nongnu.org/archive/html/qemu-block/2024-01/msg00001.html

And compared to that, based on the discussion, there are some changes:
1. Used virtio_queue_notify() instead of virtio_queue_notify_vq(), as
   suggested by Paolo, because it’s thread-safe
2. Moved virtio_queue_notify() into
   virtio_queue_aio_attach_host_notifier*(), because we always want it
3. Dropped virtio_queue_set_notification(vq, 0) from
   virtio_queue_aio_detach_host_notifier(): Paolo wasn’t sure whether
   that was safe to do from any context.  We don’t really need to call
   it anyway, so I just dropped it.
4. Added patch 1:

Patch 1 fixes virtio_scsi_drained_end() so it won’t attach polling
notifiers for the event virtqueue.  That didn’t turn out to be an issue
so far, but with patch 2, Fiona saw the virtqueue processing queue
spinning in a loop as described in
38738f7dbbda90fbc161757b7f4be35b52205552 ("virtio-scsi: don't waste CPU
polling the event virtqueue").


Note that as of eaad0fe26050c227dc5dad63205835bac4912a51 ("scsi: only
access SCSIDevice->requests from one thread") there’s a different
problem when trying to reproduce the bug via hot-plugging and
-unplugging a virtio-scsi device, specifically, when unplugging, qemu
may crash with an assertion failure[1].  I don’t have a full fix for
that yet, but in case you need a work-around for the specific case of
virtio-scsi hot-plugging and -unplugging, you can use this patch:

https://czenczek.de/0001-DONTMERGE-Fix-crash-on-scsi-unplug.patch


[1] https://lists.nongnu.org/archive/html/qemu-block/2024-01/msg00317.html


Hanna Czenczek (2):
  virtio-scsi: Attach event vq notifier with no_poll
  virtio: Keep notifications disabled during drain

 include/block/aio.h   |  7 ++++++-
 hw/scsi/virtio-scsi.c |  7 ++++++-
 hw/virtio/virtio.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH 1/2] virtio-scsi: Attach event vq notifier with no_poll

[Hanna Czenczek]

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>
---
 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 690aceec45..9f02ceea09 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -1149,6 +1149,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;
@@ -1166,7 +1167,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);
+        }
     }
 }
 
-- 
2.43.0

[PATCH 2/2] virtio: Keep notifications disabled during drain

[Hanna Czenczek]

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>
---
 include/block/aio.h |  7 ++++++-
 hw/virtio/virtio.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/include/block/aio.h b/include/block/aio.h
index 5d0a114988..8378553eb9 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -480,9 +480,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/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7549094154..4166da9e97 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -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() 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,
@@ -3563,6 +3574,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);
 }
 
 /*
@@ -3573,14 +3591,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)
-- 
2.43.0

Re: [PATCH 1/2] virtio-scsi: Attach event vq notifier with no_poll

[Stefan Hajnoczi]

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

On Wed, Jan 24, 2024 at 06:38:29PM +0100, Hanna Czenczek wrote:
> 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>
> ---
>  hw/scsi/virtio-scsi.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Thank you!

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] virtio-scsi: Attach event vq notifier with no_poll

[Fiona Ebner]

Am 24.01.24 um 18:38 schrieb Hanna Czenczek:
> 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>

Tested-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

Re: [PATCH 2/2] virtio: Keep notifications disabled during drain

[Fiona Ebner]

Am 24.01.24 um 18:38 schrieb Hanna Czenczek:
> 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>

Tested-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

Re: [PATCH 2/2] virtio: Keep notifications disabled during drain

[Stefan Hajnoczi]

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

On Wed, Jan 24, 2024 at 06:38:30PM +0100, Hanna Czenczek wrote:
> 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>
> ---
>  include/block/aio.h |  7 ++++++-
>  hw/virtio/virtio.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/include/block/aio.h b/include/block/aio.h
> index 5d0a114988..8378553eb9 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -480,9 +480,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/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 7549094154..4166da9e97 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -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() 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,
> @@ -3563,6 +3574,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);

event_notifier_set(&vq->host_notifier) is easier to understand because
it doesn't contain a non-host_notifier code path that we must not take.

Is there a reason why you used virtio_queue_notify() instead?

Otherwise:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

>  }
>  
>  /*
> @@ -3573,14 +3591,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)
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] virtio: Keep notifications disabled during drain

[Stefan Hajnoczi]

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

On Wed, Jan 24, 2024 at 06:38:28PM +0100, Hanna Czenczek wrote:
> Hi,
> 
> When registering callbacks via aio_set_event_notifier_poll(), the
> io_poll_end() callback is only invoked when polling actually ends.  If
> the notifiers are removed while in a polling section, it is not called.
> Therefore, io_poll_start() is not necessarily followed up by
> io_poll_end().
> 
> It is not entirely clear whether this is good or bad behavior.  On one
> hand, it may be unexpected to callers.  On the other, it may be
> counterproductive to call io_poll_end() when the polling section has not
> ended yet.
> 
> Right now, there is only one user of aio_set_event_notifier(), which is
> virtio_queue_aio_attach_host_notifier().  It does not expect this
> behavior, which leads to virtqueue notifiers remaining disabled if
> virtio_queue_aio_detach_host_notifier() is called while polling.  That
> can happen e.g. through virtio_scsi_drained_begin() or
> virtio_blk_drained_begin() (through virtio_blk_data_plane_detach()).
> In such a case, the virtqueue may not be processed for a while, letting
> the guest driver hang.  This can be reproduced by repeatedly
> hot-plugging and -unplugging a virtio-scsi device with a scsi-hd disk,
> because the guest will try to enumerate the virtio-scsi device while
> we’re attaching the scsi-hd disk, which causes a drain, which can cause
> the virtio-scsi virtqueue to stall as described.
> 
> Stefan has suggested ensuring we always follow up io_poll_start() by
> io_poll_end():
> 
> https://lists.nongnu.org/archive/html/qemu-block/2023-12/msg00163.html
> 
> I prefer changing the caller instead, because I don’t think we actually
> want the virtqueue notifier to be force-enabled when removing our AIO
> notifiers.  So I believe we actually only want to take care to
> force-enable it when we re-attach the AIO notifiers, and to kick
> virtqueue processing once, in case we missed any events while the AIO
> notifiers were not attached.
> 
> That is done by patch 2.  We have already discussed a prior version of
> it here:
> 
> https://lists.nongnu.org/archive/html/qemu-block/2024-01/msg00001.html
> 
> And compared to that, based on the discussion, there are some changes:
> 1. Used virtio_queue_notify() instead of virtio_queue_notify_vq(), as
>    suggested by Paolo, because it’s thread-safe
> 2. Moved virtio_queue_notify() into
>    virtio_queue_aio_attach_host_notifier*(), because we always want it
> 3. Dropped virtio_queue_set_notification(vq, 0) from
>    virtio_queue_aio_detach_host_notifier(): Paolo wasn’t sure whether
>    that was safe to do from any context.  We don’t really need to call
>    it anyway, so I just dropped it.
> 4. Added patch 1:
> 
> Patch 1 fixes virtio_scsi_drained_end() so it won’t attach polling
> notifiers for the event virtqueue.  That didn’t turn out to be an issue
> so far, but with patch 2, Fiona saw the virtqueue processing queue
> spinning in a loop as described in
> 38738f7dbbda90fbc161757b7f4be35b52205552 ("virtio-scsi: don't waste CPU
> polling the event virtqueue").
> 
> 
> Note that as of eaad0fe26050c227dc5dad63205835bac4912a51 ("scsi: only
> access SCSIDevice->requests from one thread") there’s a different
> problem when trying to reproduce the bug via hot-plugging and
> -unplugging a virtio-scsi device, specifically, when unplugging, qemu
> may crash with an assertion failure[1].  I don’t have a full fix for
> that yet, but in case you need a work-around for the specific case of
> virtio-scsi hot-plugging and -unplugging, you can use this patch:
> 
> https://czenczek.de/0001-DONTMERGE-Fix-crash-on-scsi-unplug.patch
> 
> 
> [1] https://lists.nongnu.org/archive/html/qemu-block/2024-01/msg00317.html
> 
> 
> Hanna Czenczek (2):
>   virtio-scsi: Attach event vq notifier with no_poll
>   virtio: Keep notifications disabled during drain
> 
>  include/block/aio.h   |  7 ++++++-
>  hw/scsi/virtio-scsi.c |  7 ++++++-
>  hw/virtio/virtio.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 54 insertions(+), 2 deletions(-)

This patch series also fixes RHEL-7356.

Buglink: https://issues.redhat.com/browse/RHEL-7356.
Tested-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] virtio: Keep notifications disabled during drain

[Hanna Czenczek]

On 25.01.24 19:03, Stefan Hajnoczi wrote:
> On Wed, Jan 24, 2024 at 06:38:30PM +0100, Hanna Czenczek wrote:
>> 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>
>> ---
>>   include/block/aio.h |  7 ++++++-
>>   hw/virtio/virtio.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/block/aio.h b/include/block/aio.h
>> index 5d0a114988..8378553eb9 100644
>> --- a/include/block/aio.h
>> +++ b/include/block/aio.h
>> @@ -480,9 +480,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/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 7549094154..4166da9e97 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -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() 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,
>> @@ -3563,6 +3574,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);
> event_notifier_set(&vq->host_notifier) is easier to understand because
> it doesn't contain a non-host_notifier code path that we must not take.
>
> Is there a reason why you used virtio_queue_notify() instead?

Not a good one anyway!

virtio_queue_notify() is just what seemed obvious to me (i.e. to notify 
the virtqueue).  Before removal of the AioContext lock, calling 
handle_output seemed safe.  But, yes, there was the discussion on the 
RFC that it really isn’t.  I didn’t consider that means we must rely on 
virtio_queue_notify() calling event_notifier_set(), so we may as well 
call it explicitly here.

I’ll fix it, thanks for pointing it out!

> Otherwise:
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Thanks!

Hanna

Re: [PATCH 2/2] virtio: Keep notifications disabled during drain

[Hanna Czenczek]

On 25.01.24 19:18, Hanna Czenczek wrote:
> On 25.01.24 19:03, Stefan Hajnoczi wrote:
>> On Wed, Jan 24, 2024 at 06:38:30PM +0100, Hanna Czenczek wrote:

[...]

>>> @@ -3563,6 +3574,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);
>> event_notifier_set(&vq->host_notifier) is easier to understand because
>> it doesn't contain a non-host_notifier code path that we must not take.
>>
>> Is there a reason why you used virtio_queue_notify() instead?
>
> Not a good one anyway!
>
> virtio_queue_notify() is just what seemed obvious to me (i.e. to 
> notify the virtqueue).  Before removal of the AioContext lock, calling 
> handle_output seemed safe.  But, yes, there was the discussion on the 
> RFC that it really isn’t.  I didn’t consider that means we must rely 
> on virtio_queue_notify() calling event_notifier_set(), so we may as 
> well call it explicitly here.
>
> I’ll fix it, thanks for pointing it out!

(I think together with this change, I’ll also remove the 
event_notifier_set() call from virtio_blk_data_plane_start().  It’d 
obviously be a duplicate, and removing it shows why 
virtio_queue_aio_attach_host_notifier() should always kick the queue.)

Re: [PATCH 2/2] virtio: Keep notifications disabled during drain

[Stefan Hajnoczi]

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

On Thu, Jan 25, 2024 at 07:32:12PM +0100, Hanna Czenczek wrote:
> On 25.01.24 19:18, Hanna Czenczek wrote:
> > On 25.01.24 19:03, Stefan Hajnoczi wrote:
> > > On Wed, Jan 24, 2024 at 06:38:30PM +0100, Hanna Czenczek wrote:
> 
> [...]
> 
> > > > @@ -3563,6 +3574,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);
> > > event_notifier_set(&vq->host_notifier) is easier to understand because
> > > it doesn't contain a non-host_notifier code path that we must not take.
> > > 
> > > Is there a reason why you used virtio_queue_notify() instead?
> > 
> > Not a good one anyway!
> > 
> > virtio_queue_notify() is just what seemed obvious to me (i.e. to notify
> > the virtqueue).  Before removal of the AioContext lock, calling
> > handle_output seemed safe.  But, yes, there was the discussion on the
> > RFC that it really isn’t.  I didn’t consider that means we must rely on
> > virtio_queue_notify() calling event_notifier_set(), so we may as well
> > call it explicitly here.
> > 
> > I’ll fix it, thanks for pointing it out!
> 
> (I think together with this change, I’ll also remove the
> event_notifier_set() call from virtio_blk_data_plane_start().  It’d
> obviously be a duplicate, and removing it shows why
> virtio_queue_aio_attach_host_notifier() should always kick the queue.)

Yes, it can be removed from start().

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]