[Qemu-devel] [PATCH 0/6] virtio: Merge virtio-{blk, scsi} host notifier handling paths

[Qemu-devel] [PATCH 0/6] virtio: Merge virtio-{blk, scsi} host notifier handling paths

[Fam Zheng]

v3: Rebase to master.
    Squash 4 into 3. [Paolo]
    Add comment and commit message. [Stefan]
    Add Stefan's r-b in patch 1 and 2.

v2: Only convert virtio-{blk,scsi}. [Paolo]

The benifit of this 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 (6):
  virtio-bus: Drop "set_handler" parameter
  virtio: Add typedef for handle_output
  virtio: Introduce virtio_add_queue_aio
  virtio-blk: Call virtio_add_queue_aio
  virtio-scsi: Call virtio_add_queue_aio
  Revert "mirror: Workaround for unexpected iohandler events during
    completion"

 block/mirror.c             |  9 ---------
 hw/block/virtio-blk.c      |  2 +-
 hw/scsi/virtio-scsi.c      |  9 +++------
 hw/virtio/virtio-bus.c     | 13 ++++++-------
 hw/virtio/virtio.c         | 45 +++++++++++++++++++++++++++++++++++++--------
 include/hw/virtio/virtio.h |  8 ++++++--
 6 files changed, 53 insertions(+), 33 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/6] virtio-bus: Drop "set_handler" parameter

[Fam Zheng]

It always equals to assign now.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio-bus.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index a85b7c8..8700de0 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -150,10 +150,9 @@ void virtio_bus_set_vdev_config(VirtioBusState *bus, uint8_t *config)
  * This function handles both assigning the ioeventfd handler and
  * registering it with the kernel.
  * assign: register/deregister ioeventfd with the kernel
- * set_handler: use the generic ioeventfd handler
  */
 static int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
-                                      int n, bool assign, bool set_handler)
+                                      int n, bool assign)
 {
     VirtIODevice *vdev = virtio_bus_get_device(bus);
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
@@ -167,7 +166,7 @@ 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, set_handler);
+        virtio_queue_set_host_notifier_fd_handler(vq, true, true);
         r = k->ioeventfd_assign(proxy, notifier, n, assign);
         if (r < 0) {
             error_report("%s: unable to assign ioeventfd: %d", __func__, r);
@@ -201,7 +200,7 @@ void virtio_bus_start_ioeventfd(VirtioBusState *bus)
         if (!virtio_queue_get_num(vdev, n)) {
             continue;
         }
-        r = set_host_notifier_internal(proxy, bus, n, true, true);
+        r = set_host_notifier_internal(proxy, bus, n, true);
         if (r < 0) {
             goto assign_error;
         }
@@ -215,7 +214,7 @@ assign_error:
             continue;
         }
 
-        r = set_host_notifier_internal(proxy, bus, n, false, false);
+        r = set_host_notifier_internal(proxy, bus, n, false);
         assert(r >= 0);
     }
     k->ioeventfd_set_started(proxy, false, true);
@@ -237,7 +236,7 @@ void virtio_bus_stop_ioeventfd(VirtioBusState *bus)
         if (!virtio_queue_get_num(vdev, n)) {
             continue;
         }
-        r = set_host_notifier_internal(proxy, bus, n, false, false);
+        r = set_host_notifier_internal(proxy, bus, n, false);
         assert(r >= 0);
     }
     k->ioeventfd_set_started(proxy, false, false);
@@ -269,7 +268,7 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
          */
         virtio_bus_stop_ioeventfd(bus);
     }
-    return set_host_notifier_internal(proxy, bus, n, assign, false);
+    return set_host_notifier_internal(proxy, bus, n, assign);
 }
 
 static char *virtio_bus_get_dev_path(DeviceState *dev)
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/6] virtio-bus: Drop "set_handler" parameter

[Cornelia Huck]

On Tue, 12 Jul 2016 13:19:55 +0800
Fam Zheng <famz@redhat.com> wrote:

> It always equals to assign now.

I think this needs further elaboration...

> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/virtio/virtio-bus.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 

> @@ -167,7 +166,7 @@ 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, set_handler);
> +        virtio_queue_set_host_notifier_fd_handler(vq, true, true);
>          r = k->ioeventfd_assign(proxy, notifier, n, assign);
>          if (r < 0) {
>              error_report("%s: unable to assign ioeventfd: %d", __func__, r);

> @@ -269,7 +268,7 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
>           */
>          virtio_bus_stop_ioeventfd(bus);
>      }
> -    return set_host_notifier_internal(proxy, bus, n, assign, false);
> +    return set_host_notifier_internal(proxy, bus, n, assign);

...because this changes the behaviour for assign==true.

>  }
> 
>  static char *virtio_bus_get_dev_path(DeviceState *dev)

Re: [Qemu-devel] [PATCH 1/6] virtio-bus: Drop "set_handler" parameter

[Fam Zheng]

On Tue, 07/12 11:09, Cornelia Huck wrote:
> On Tue, 12 Jul 2016 13:19:55 +0800
> Fam Zheng <famz@redhat.com> wrote:
> 
> > It always equals to assign now.
> 
> I think this needs further elaboration...
> 
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  hw/virtio/virtio-bus.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> 
> > @@ -167,7 +166,7 @@ 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, set_handler);
> > +        virtio_queue_set_host_notifier_fd_handler(vq, true, true);
> >          r = k->ioeventfd_assign(proxy, notifier, n, assign);
> >          if (r < 0) {
> >              error_report("%s: unable to assign ioeventfd: %d", __func__, r);
> 
> > @@ -269,7 +268,7 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
> >           */
> >          virtio_bus_stop_ioeventfd(bus);
> >      }
> > -    return set_host_notifier_internal(proxy, bus, n, assign, false);
> > +    return set_host_notifier_internal(proxy, bus, n, assign);
> 
> ...because this changes the behaviour for assign==true.

Oh this is the one I overlook in rebase because it wasn't present with your
refactoring but it is now back in commit 0830c96d70b.

Good catch, I need to fix this, and therefore Stefan's r-b shouldn't have been
kept along.

Fam

> 
> >  }
> > 
> >  static char *virtio_bus_get_dev_path(DeviceState *dev)
> 

Re: [Qemu-devel] [PATCH 1/6] virtio-bus: Drop "set_handler" parameter

[Cornelia Huck]

On Tue, 12 Jul 2016 17:16:42 +0800
Fam Zheng <famz@redhat.com> wrote:

> On Tue, 07/12 11:09, Cornelia Huck wrote:
> > On Tue, 12 Jul 2016 13:19:55 +0800

> > > @@ -269,7 +268,7 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
> > >           */
> > >          virtio_bus_stop_ioeventfd(bus);
> > >      }
> > > -    return set_host_notifier_internal(proxy, bus, n, assign, false);
> > > +    return set_host_notifier_internal(proxy, bus, n, assign);
> > 
> > ...because this changes the behaviour for assign==true.
> 
> Oh this is the one I overlook in rebase because it wasn't present with your
> refactoring but it is now back in commit 0830c96d70b.

Yes, we need to come up with a proper solution, but I currently don't
see this for 2.7.

> 
> Good catch, I need to fix this, and therefore Stefan's r-b shouldn't have been
> kept along.

The whole host notifier stuff is good at causing headaches :(

[Qemu-devel] [PATCH 2/6] 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>
Reviewed-by: Stefan Hajnoczi <stefanha@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 18153d5..2cc68d24 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);
+    VirtIOHandleOutput handle_output;
+    VirtIOHandleOutput 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 *))
+                            VirtIOHandleOutput handle_output)
 {
     int i;
 
@@ -1804,8 +1804,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 *))
+                                                VirtIOHandleOutput 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..b104104 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 (*VirtIOHandleOutput)(VirtIODevice *, VirtQueue *);
+
 VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
-                            void (*handle_output)(VirtIODevice *,
-                                                  VirtQueue *));
+                            VirtIOHandleOutput handle_output);
 
 void virtio_del_queue(VirtIODevice *vdev, int n);
 
-- 
2.7.4

[Qemu-devel] [PATCH 3/6] virtio: Introduce virtio_add_queue_aio

[Fam Zheng]

Using this function instead of virtio_add_queue marks the vq as aio
based. This differentiation will be useful in later patches.

Distinguish between virtqueue processing in the iohandler context and main loop
AioContext.  iohandler context is isolated from AioContexts and therefore does
not run during aio_poll().

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

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 2cc68d24..2fbed0c 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -97,6 +97,7 @@ struct VirtQueue
     uint16_t vector;
     VirtIOHandleOutput handle_output;
     VirtIOHandleOutput handle_aio_output;
+    bool use_aio;
     VirtIODevice *vdev;
     EventNotifier guest_notifier;
     EventNotifier host_notifier;
@@ -1130,8 +1131,9 @@ void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector)
     }
 }
 
-VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
-                            VirtIOHandleOutput handle_output)
+static VirtQueue *virtio_add_queue_internal(VirtIODevice *vdev, int queue_size,
+                                            VirtIOHandleOutput handle_output,
+                                            bool use_aio)
 {
     int i;
 
@@ -1148,10 +1150,28 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
     vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN;
     vdev->vq[i].handle_output = handle_output;
     vdev->vq[i].handle_aio_output = NULL;
+    vdev->vq[i].use_aio = use_aio;
 
     return &vdev->vq[i];
 }
 
+/* Add a virt queue and mark AIO.
+ * An AIO queue will use the AioContext based event interface instead of the
+ * default IOHandler and EventNotifier interface.
+ */
+VirtQueue *virtio_add_queue_aio(VirtIODevice *vdev, int queue_size,
+                                VirtIOHandleOutput handle_output)
+{
+    return virtio_add_queue_internal(vdev, queue_size, handle_output, true);
+}
+
+/* Add a normal virt queue (on the contrary to the AIO version above. */
+VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
+                            VirtIOHandleOutput handle_output)
+{
+    return virtio_add_queue_internal(vdev, queue_size, handle_output, false);
+}
+
 void virtio_del_queue(VirtIODevice *vdev, int n)
 {
     if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
@@ -1830,11 +1850,21 @@ static void virtio_queue_host_notifier_read(EventNotifier *n)
 void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                bool set_handler)
 {
+    AioContext *ctx = qemu_get_aio_context();
     if (assign && set_handler) {
-        event_notifier_set_handler(&vq->host_notifier, true,
+        if (vq->use_aio) {
+            aio_set_event_notifier(ctx, &vq->host_notifier, true,
                                    virtio_queue_host_notifier_read);
+        } else {
+            event_notifier_set_handler(&vq->host_notifier, true,
+                                       virtio_queue_host_notifier_read);
+        }
     } else {
-        event_notifier_set_handler(&vq->host_notifier, true, NULL);
+        if (vq->use_aio) {
+            aio_set_event_notifier(ctx, &vq->host_notifier, true, NULL);
+        } else {
+            event_notifier_set_handler(&vq->host_notifier, true, NULL);
+        }
     }
     if (!assign) {
         /* Test and clear notifier before after disabling event,
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index b104104..1e8cae5 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -143,6 +143,9 @@ typedef void (*VirtIOHandleOutput)(VirtIODevice *, VirtQueue *);
 VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
                             VirtIOHandleOutput handle_output);
 
+VirtQueue *virtio_add_queue_aio(VirtIODevice *vdev, int queue_size,
+                                VirtIOHandleOutput handle_output);
+
 void virtio_del_queue(VirtIODevice *vdev, int n);
 
 void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_num);
-- 
2.7.4

[Qemu-devel] [PATCH 4/6] virtio-blk: Call virtio_add_queue_aio

[Fam Zheng]

AIO based handler is more appropriate here because it will then
cooperate with bdrv_drained_begin/end. It is needed by the coming
revert patch.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/virtio-blk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index ae86e94..97578a4 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -913,7 +913,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     s->sector_mask = (s->conf.conf.logical_block_size / BDRV_SECTOR_SIZE) - 1;
 
     for (i = 0; i < conf->num_queues; i++) {
-        virtio_add_queue(vdev, 128, virtio_blk_handle_output);
+        virtio_add_queue_aio(vdev, 128, virtio_blk_handle_output);
     }
     virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err);
     if (err != NULL) {
-- 
2.7.4

[Qemu-devel] [PATCH 5/6] virtio-scsi: Call virtio_add_queue_aio

[Fam Zheng]

AIO based handler is more appropriate here because it will then
cooperate with bdrv_drained_begin/end. It is needed by the coming
revert patch.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/virtio-scsi.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index e8179d6..2bc2fca 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -846,13 +846,10 @@ void virtio_scsi_common_realize(DeviceState *dev, Error **errp,
     s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
     s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;
 
-    s->ctrl_vq = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE,
-                                  ctrl);
-    s->event_vq = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE,
-                                   evt);
+    s->ctrl_vq = virtio_add_queue_aio(vdev, VIRTIO_SCSI_VQ_SIZE, ctrl);
+    s->event_vq = virtio_add_queue_aio(vdev, VIRTIO_SCSI_VQ_SIZE, evt);
     for (i = 0; i < s->conf.num_queues; i++) {
-        s->cmd_vqs[i] = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE,
-                                         cmd);
+        s->cmd_vqs[i] = virtio_add_queue_aio(vdev, VIRTIO_SCSI_VQ_SIZE, cmd);
     }
 
     if (s->conf.iothread) {
-- 
2.7.4

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

[Fam Zheng]

This reverts commit ab27c3b5e7408693dde0b565f050aa55c4a1bcef.

The virtio storage device host notifiers now work with
bdrv_drained_begin/end, so we don't need this hack any more.

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 8d96049..8a452a2 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -506,9 +506,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);
 }
 
@@ -732,12 +729,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.7.4