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

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

[Fam Zheng]

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

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 (7):
  virtio-bus: Drop "set_handler" parameter
  virtio: Add typedef for handle_output
  virtio: Introduce virtio_add_queue_aio
  virtio: Use aio_set_event_notifier for aio vq
  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     | 11 +++++------
 hw/virtio/virtio.c         | 40 ++++++++++++++++++++++++++++++++--------
 include/hw/virtio/virtio.h |  8 ++++++--
 6 files changed, 47 insertions(+), 32 deletions(-)

-- 
2.8.3

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

[Fam Zheng]

It always equals to assign now.

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

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 1313760..f34b4fc 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);
-- 
2.8.3

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

[Stefan Hajnoczi]

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

On Fri, Jun 24, 2016 at 08:39:28PM +0800, Fam Zheng wrote:
> It always equals to assign now.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/virtio/virtio-bus.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)

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

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

[Qemu-devel] [PATCH v2 2/7] 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..bba73bb 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;
 
@@ -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 *))
+                                                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.8.3

Re: [Qemu-devel] [PATCH v2 2/7] virtio: Add typedef for handle_output

[Stefan Hajnoczi]

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

On Fri, Jun 24, 2016 at 08:39:29PM +0800, Fam Zheng wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH v2 3/7] 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.

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

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index bba73bb..1ea6f66 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,23 @@ 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];
 }
 
+VirtQueue *virtio_add_queue_aio(VirtIODevice *vdev, int queue_size,
+                                VirtIOHandleOutput handle_output)
+{
+    return virtio_add_queue_internal(vdev, queue_size, handle_output, true);
+}
+
+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) {
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.8.3

Re: [Qemu-devel] [PATCH v2 3/7] virtio: Introduce virtio_add_queue_aio

[Stefan Hajnoczi]

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

On Fri, Jun 24, 2016 at 08:39:30PM +0800, Fam Zheng wrote:
> +VirtQueue *virtio_add_queue_aio(VirtIODevice *vdev, int queue_size,
> +                                VirtIOHandleOutput handle_output)
> +{
> +    return virtio_add_queue_internal(vdev, queue_size, handle_output, true);
> +}
> +
> +VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
> +                            VirtIOHandleOutput handle_output)
> +{
> +    return virtio_add_queue_internal(vdev, queue_size, handle_output, false);
> +}

Please include doc comments for this new API explaining the meaning of
aio vs non-aio queues.

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

[Qemu-devel] [PATCH v2 4/7] virtio: Use aio_set_event_notifier for aio vq

[Fam Zheng]

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

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 1ea6f66..a586529 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1835,11 +1835,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,
-- 
2.8.3

Re: [Qemu-devel] [PATCH v2 4/7] virtio: Use aio_set_event_notifier for aio vq

[Stefan Hajnoczi]

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

On Fri, Jun 24, 2016 at 08:39:31PM +0800, Fam Zheng wrote:

Commit description?  For example:

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().

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

[Qemu-devel] [PATCH v2 5/7] 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 284e646..b1f56c3 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -888,7 +888,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     s->rq = NULL;
     s->sector_mask = (s->conf.conf.logical_block_size / BDRV_SECTOR_SIZE) - 1;
 
-    s->vq = virtio_add_queue(vdev, 128, virtio_blk_handle_output);
+    s->vq = virtio_add_queue_aio(vdev, 128, virtio_blk_handle_output);
     virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err);
     if (err != NULL) {
         error_propagate(errp, err);
-- 
2.8.3

Re: [Qemu-devel] [PATCH v2 5/7] virtio-blk: Call virtio_add_queue_aio

[Stefan Hajnoczi]

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

On Fri, Jun 24, 2016 at 08:39:32PM +0800, Fam Zheng wrote:
> 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 284e646..b1f56c3 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -888,7 +888,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>      s->rq = NULL;
>      s->sector_mask = (s->conf.conf.logical_block_size / BDRV_SECTOR_SIZE) - 1;
>  
> -    s->vq = virtio_add_queue(vdev, 128, virtio_blk_handle_output);
> +    s->vq = virtio_add_queue_aio(vdev, 128, virtio_blk_handle_output);

It's weird that virtio_add_queue_aio() doesn't take an AioContext.

This change moves us one step closer to dropping dataplane-specific
code.  The difference between dataplane and non-dataplane should simply
by the virtqueue's AioContext, which should come from BlockBackend's
AioContext.

Anyway, not a deal-breaker but I think we should make the AioContext
explicit in the future...

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

Re: [Qemu-devel] [PATCH v2 5/7] virtio-blk: Call virtio_add_queue_aio

[Paolo Bonzini]

On 28/06/2016 17:12, Stefan Hajnoczi wrote:\
> It's weird that virtio_add_queue_aio() doesn't take an AioContext.
> 
> This change moves us one step closer to dropping dataplane-specific
> code.  The difference between dataplane and non-dataplane should simply
> by the virtqueue's AioContext, which should come from BlockBackend's
> AioContext.
> 
> Anyway, not a deal-breaker but I think we should make the AioContext
> explicit in the future...

I agree, and in fact I think that the virtio_add_queue_aio API should be
temporary.

Hopefully, now that ioeventfd APIs have been cleaned up and all backends
have support for it, we can move the ioeventfd down from the proxy
devices (virtio-*-pci) to the actual virtio devices.  This would let
virtio-blk and virtio-scsi know whether ioeventfd is in use (they
currently can't see that), and start the dataplane event handlers on the
first kick when ioeventfd is enabled.  For them to do this it is not
necessary to use virtio_add_queue_aio; the first kick can use the normal
virtio_add_queue registration functions.

However, this fix is much nicer than the one we currently have, so I am
in favor of it.  I would squash patches 3 and 4 together, though.

Paolo
Paolo

[Qemu-devel] [PATCH v2 6/7] 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 71d09d3..57deddd 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -851,13 +851,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.8.3

[Qemu-devel] [PATCH v2 7/7] 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 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

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

[Stefan Hajnoczi]

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

On Fri, Jun 24, 2016 at 08:39:27PM +0800, Fam Zheng wrote:
> v2: Only convert virtio-{blk,scsi}. [Paolo]
> 
> 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.

Looks good overall.  I have left comments.

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