[Qemu-devel] [PATCH 0/4] virtio: Merge two host notifier handling paths

[Qemu-devel] [PATCH 0/4] virtio: Merge two host notifier handling paths

[Fam Zheng]

This series is based on top of Cornelia's

    [PATCH 0/6] virtio: refactor host notifiers

The benifit is we don't use event_notifier_set_handler even in non-dataplane
now, which in turn makes virtio-blk and virtio-scsi follow block layer aio
context semantics. Specifically, I/O requests must come from
blk_get_aio_context(blk) events, rather than iohandler_get_aio_context(), so
that bdrv_drained_begin/end will work as expected.

Patch 4 reverts the hack (ab27c3b5e7) we added for 2.6. Lately, commit
b880481579 added another pair of bdrv_drained_begin/end so the crash cannot
happen even without ab27c3b5e7, but in order to avoid leaking requests, patch
two is still a must.



Fam Zheng (4):
  virtio: Add typedef for handle_output
  virtio: Always use aio path to set host handler
  virtio: Drop the unused virtio_queue_set_host_notifier_fd_handler code
  Revert "mirror: Workaround for unexpected iohandler events during
    completion"

 block/mirror.c                  |  9 ---------
 hw/block/dataplane/virtio-blk.c |  6 +++---
 hw/scsi/virtio-scsi-dataplane.c |  9 +++++----
 hw/virtio/virtio-bus.c          | 13 +++++++++----
 hw/virtio/virtio.c              | 43 ++++++++++++-----------------------------
 include/hw/virtio/virtio.h      | 13 ++++++-------
 6 files changed, 35 insertions(+), 58 deletions(-)

-- 
2.8.3

[Qemu-devel] [PATCH 1/4] virtio: Add typedef for handle_output

[Fam Zheng]

The function pointer signature has been repeated a few times, using a
typedef may make coding easier.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/virtio/virtio.c         | 9 ++++-----
 include/hw/virtio/virtio.h | 5 +++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7ed06ea..e1e93c7 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -95,8 +95,8 @@ struct VirtQueue
     int inuse;
 
     uint16_t vector;
-    void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq);
-    void (*handle_aio_output)(VirtIODevice *vdev, VirtQueue *vq);
+    VirtQueueHandleOutput handle_output;
+    VirtQueueHandleOutput handle_aio_output;
     VirtIODevice *vdev;
     EventNotifier guest_notifier;
     EventNotifier host_notifier;
@@ -1131,7 +1131,7 @@ void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector)
 }
 
 VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
-                            void (*handle_output)(VirtIODevice *, VirtQueue *))
+                            VirtQueueHandleOutput handle_output)
 {
     int i;
 
@@ -1794,8 +1794,7 @@ static void virtio_queue_host_notifier_aio_read(EventNotifier *n)
 }
 
 void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
-                                                void (*handle_output)(VirtIODevice *,
-                                                                      VirtQueue *))
+                                                VirtQueueHandleOutput handle_output)
 {
     if (handle_output) {
         vq->handle_aio_output = handle_output;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 96b581d..faec22a 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -138,9 +138,10 @@ void virtio_cleanup(VirtIODevice *vdev);
 /* Set the child bus name. */
 void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);
 
+typedef void (*VirtQueueHandleOutput)(VirtIODevice *, VirtQueue *);
+
 VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
-                            void (*handle_output)(VirtIODevice *,
-                                                  VirtQueue *));
+                            VirtQueueHandleOutput handle_output);
 
 void virtio_del_queue(VirtIODevice *vdev, int n);
 
-- 
2.8.3

[Qemu-devel] [PATCH 2/4] virtio: Always use aio path to set host handler

[Fam Zheng]

Apart from the interface difference, the aio version works the same as
the non-aio one. The event notifier versus aio fd handler makes no
diffeerence, except the former led to an ugly patch in commit
ab27c3b5e7, which won't be necessary any more.

As the first step to unify them, all callers are switched to this
renamed aio iterface, and function comment is added.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/dataplane/virtio-blk.c |  6 +++---
 hw/scsi/virtio-scsi-dataplane.c |  9 +++++----
 hw/virtio/virtio-bus.c          | 13 +++++++++----
 hw/virtio/virtio.c              | 12 +++++++++---
 include/hw/virtio/virtio.h      |  6 +++---
 5 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 2041b04..61d65bb 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -174,8 +174,8 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
 
     /* Get this show started by hooking up our callbacks */
     aio_context_acquire(s->ctx);
-    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx,
-                                               virtio_blk_data_plane_handle_output);
+    virtio_queue_set_host_notifier_handler(s->vq, s->ctx, true,
+                                           virtio_blk_data_plane_handle_output);
     aio_context_release(s->ctx);
     return;
 
@@ -210,7 +210,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
     aio_context_acquire(s->ctx);
 
     /* Stop notifications for new requests from guest */
-    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, NULL);
+    virtio_queue_set_host_notifier_handler(s->vq, s->ctx, false, NULL);
 
     /* Drain and switch bs back to the QEMU main loop */
     blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context());
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 18ced31..ffabb87 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -80,7 +80,7 @@ static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n,
         return rc;
     }
 
-    virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, fn);
+    virtio_queue_set_host_notifier_handler(vq, s->ctx, true, fn);
     return 0;
 }
 
@@ -97,10 +97,11 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s)
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
     int i;
 
-    virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, NULL);
-    virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, NULL);
+    virtio_queue_set_host_notifier_handler(vs->ctrl_vq, s->ctx, false, NULL);
+    virtio_queue_set_host_notifier_handler(vs->event_vq, s->ctx, false, NULL);
     for (i = 0; i < vs->conf.num_queues; i++) {
-        virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, NULL);
+        virtio_queue_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, false,
+                                               NULL);
     }
 }
 
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index f34b4fc..0f81096 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -166,16 +166,20 @@ static int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
             error_report("%s: unable to init event notifier: %d", __func__, r);
             return r;
         }
-        virtio_queue_set_host_notifier_fd_handler(vq, true, true);
+        virtio_queue_set_host_notifier_handler(vq, qemu_get_aio_context(),
+                                               true, NULL);
+
         r = k->ioeventfd_assign(proxy, notifier, n, assign);
         if (r < 0) {
             error_report("%s: unable to assign ioeventfd: %d", __func__, r);
-            virtio_queue_set_host_notifier_fd_handler(vq, false, false);
+            virtio_queue_set_host_notifier_handler(vq, qemu_get_aio_context(),
+                                                   false, NULL);
             event_notifier_cleanup(notifier);
             return r;
         }
     } else {
-        virtio_queue_set_host_notifier_fd_handler(vq, false, false);
+        virtio_queue_set_host_notifier_handler(vq, qemu_get_aio_context(),
+                                               false, NULL);
         k->ioeventfd_assign(proxy, notifier, n, assign);
         event_notifier_cleanup(notifier);
     }
@@ -269,7 +273,8 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
      * ioeventfd and we may end up with a notification where
      * we don't expect one.
      */
-    virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
+    virtio_queue_set_host_notifier_handler(vq, qemu_get_aio_context(),
+                                           false, NULL);
     if (!assign) {
         /* Use generic ioeventfd handler again. */
         k->ioeventfd_set_disabled(proxy, false);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index e1e93c7..99cd0c0 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1793,10 +1793,16 @@ static void virtio_queue_host_notifier_aio_read(EventNotifier *n)
     }
 }
 
-void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
-                                                VirtQueueHandleOutput handle_output)
+/* If assign == true, set the host notifier handler to @handle_output, or use
+ * the default vq handler if it is NULL, in the aio context @ctx.
+ * If assign == false, unregister the handler of host notifier in @ctx, and do
+ * a last host notify if there are notifications pending. */
+void virtio_queue_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
+                                            bool assign,
+                                            VirtQueueHandleOutput handle_output)
 {
-    if (handle_output) {
+    if (assign) {
+        handle_output = handle_output ?: vq->handle_output;
         vq->handle_aio_output = handle_output;
         aio_set_event_notifier(ctx, &vq->host_notifier, true,
                                virtio_queue_host_notifier_aio_read);
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index faec22a..9a40df7 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -250,9 +250,9 @@ void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
 EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
 void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                bool set_handler);
-void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
-                                                void (*fn)(VirtIODevice *,
-                                                           VirtQueue *));
+void virtio_queue_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
+                                            bool assign,
+                                            VirtQueueHandleOutput handle_output);
 void virtio_irq(VirtQueue *vq);
 VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector);
 VirtQueue *virtio_vector_next_queue(VirtQueue *vq);
-- 
2.8.3

Re: [Qemu-devel] [PATCH 2/4] virtio: Always use aio path to set host handler

[Paolo Bonzini]

On 24/06/2016 07:12, Fam Zheng wrote:
> Apart from the interface difference, the aio version works the same as
> the non-aio one. The event notifier versus aio fd handler makes no
> diffeerence, except the former led to an ugly patch in commit
> ab27c3b5e7, which won't be necessary any more.
> 
> As the first step to unify them, all callers are switched to this
> renamed aio iterface, and function comment is added.

I think this is too aggressive, and I'm not sure it's correct.
bdrv_drain wants to look at block devices only, while this would also
process NICs too for example.  I don't know the effect.

Can we do this only for virtio-blk and virtio-scsi?

Paolo

Re: [Qemu-devel] [PATCH 2/4] virtio: Always use aio path to set host handler

[Fam Zheng]

On Fri, 06/24 08:39, Paolo Bonzini wrote:
> 
> 
> On 24/06/2016 07:12, Fam Zheng wrote:
> > Apart from the interface difference, the aio version works the same as
> > the non-aio one. The event notifier versus aio fd handler makes no
> > diffeerence, except the former led to an ugly patch in commit
> > ab27c3b5e7, which won't be necessary any more.
> > 
> > As the first step to unify them, all callers are switched to this
> > renamed aio iterface, and function comment is added.
> 
> I think this is too aggressive, and I'm not sure it's correct.
> bdrv_drain wants to look at block devices only, while this would also
> process NICs too for example.  I don't know the effect.
> 
> Can we do this only for virtio-blk and virtio-scsi?

OK, I'll see. :)

Fam

[Qemu-devel] [PATCH 3/4] virtio: Drop the unused virtio_queue_set_host_notifier_fd_handler code

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/virtio/virtio.c         | 24 ------------------------
 include/hw/virtio/virtio.h |  2 --
 2 files changed, 26 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 99cd0c0..7a375c1 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1815,30 +1815,6 @@ void virtio_queue_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
     }
 }
 
-static void virtio_queue_host_notifier_read(EventNotifier *n)
-{
-    VirtQueue *vq = container_of(n, VirtQueue, host_notifier);
-    if (event_notifier_test_and_clear(n)) {
-        virtio_queue_notify_vq(vq);
-    }
-}
-
-void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
-                                               bool set_handler)
-{
-    if (assign && set_handler) {
-        event_notifier_set_handler(&vq->host_notifier, true,
-                                   virtio_queue_host_notifier_read);
-    } else {
-        event_notifier_set_handler(&vq->host_notifier, true, NULL);
-    }
-    if (!assign) {
-        /* Test and clear notifier before after disabling event,
-         * in case poll callback didn't have time to run. */
-        virtio_queue_host_notifier_read(&vq->host_notifier);
-    }
-}
-
 EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq)
 {
     return &vq->host_notifier;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 9a40df7..49488bf 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -248,8 +248,6 @@ EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq);
 void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                 bool with_irqfd);
 EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
-void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
-                                               bool set_handler);
 void virtio_queue_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
                                             bool assign,
                                             VirtQueueHandleOutput handle_output);
-- 
2.8.3

[Qemu-devel] [PATCH 4/4] Revert "mirror: Workaround for unexpected iohandler events during completion"

[Fam Zheng]

This reverts commit ab27c3b5e7408693dde0b565f050aa55c4a1bcef.

The virtio host notifiers are now covered by bdrv_drained_begin/end, so
we don't need this hacky quiescing of the iohandler context anymore.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index a04ed9c..147c0d6 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -500,9 +500,6 @@ static void mirror_exit(BlockJob *job, void *opaque)
     block_job_completed(&s->common, data->ret);
     g_free(data);
     bdrv_drained_end(src);
-    if (qemu_get_aio_context() == bdrv_get_aio_context(src)) {
-        aio_enable_external(iohandler_get_aio_context());
-    }
     bdrv_unref(src);
 }
 
@@ -726,12 +723,6 @@ immediate_exit:
     /* Before we switch to target in mirror_exit, make sure data doesn't
      * change. */
     bdrv_drained_begin(bs);
-    if (qemu_get_aio_context() == bdrv_get_aio_context(bs)) {
-        /* FIXME: virtio host notifiers run on iohandler_ctx, therefore the
-         * above bdrv_drained_end isn't enough to quiesce it. This is ugly, we
-         * need a block layer API change to achieve this. */
-        aio_disable_external(iohandler_get_aio_context());
-    }
     block_job_defer_to_main_loop(&s->common, mirror_exit, data);
 }
 
-- 
2.8.3