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

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

[Fam Zheng]

v4: Drop patch 1. [Cornelia]
    Add the last trivial patch.

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: 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"
  virtio-scsi: Replace HandleOutput typedef

 block/mirror.c                  |  9 ---------
 hw/block/virtio-blk.c           |  2 +-
 hw/scsi/virtio-scsi.c           | 14 ++++++-------
 hw/virtio/virtio.c              | 45 +++++++++++++++++++++++++++++++++--------
 include/hw/virtio/virtio-scsi.h |  6 ++----
 include/hw/virtio/virtio.h      |  8 ++++++--
 6 files changed, 52 insertions(+), 32 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v4 1/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 8a681f5..3670829 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 v4 2/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 3670829..7a82f79 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 v4 3/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 v4 4/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 722c93e..45e2ee8 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 v4 5/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

[Qemu-devel] [PATCH v4 6/6] virtio-scsi: Replace HandleOutput typedef

[Fam Zheng]

There is a new common one in virtio.h, use it.

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

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 45e2ee8..88d4bf0 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -824,8 +824,9 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = {
 };
 
 void virtio_scsi_common_realize(DeviceState *dev, Error **errp,
-                                HandleOutput ctrl, HandleOutput evt,
-                                HandleOutput cmd)
+                                VirtIOHandleOutput ctrl,
+                                VirtIOHandleOutput evt,
+                                VirtIOHandleOutput cmd)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(dev);
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 5e3f088..a1e0cfb 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -121,11 +121,9 @@ typedef struct VirtIOSCSIReq {
     } req;
 } VirtIOSCSIReq;
 
-typedef void (*HandleOutput)(VirtIODevice *, VirtQueue *);
-
 void virtio_scsi_common_realize(DeviceState *dev, Error **errp,
-                                HandleOutput ctrl, HandleOutput evt,
-                                HandleOutput cmd);
+                                VirtIOHandleOutput ctrl, VirtIOHandleOutput evt,
+                                VirtIOHandleOutput cmd);
 
 void virtio_scsi_common_unrealize(DeviceState *dev, Error **errp);
 void virtio_scsi_handle_event_vq(VirtIOSCSI *s, VirtQueue *vq);
-- 
2.7.4

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

[Cornelia Huck]

On Wed, 13 Jul 2016 13:09:42 +0800
Fam Zheng <famz@redhat.com> wrote:

> v4: Drop patch 1. [Cornelia]
>     Add the last trivial patch.
> 
> 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: 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"
>   virtio-scsi: Replace HandleOutput typedef
> 
>  block/mirror.c                  |  9 ---------
>  hw/block/virtio-blk.c           |  2 +-
>  hw/scsi/virtio-scsi.c           | 14 ++++++-------
>  hw/virtio/virtio.c              | 45 +++++++++++++++++++++++++++++++++--------
>  include/hw/virtio/virtio-scsi.h |  6 ++----
>  include/hw/virtio/virtio.h      |  8 ++++++--
>  6 files changed, 52 insertions(+), 32 deletions(-)

Lightly tested with virtio-blk on s390x. Survives some disk I/O and
managedsave/restore via libvirt.

Re: [Qemu-devel] [PATCH v4 1/6] virtio: Add typedef for handle_output

[Cornelia Huck]

On Wed, 13 Jul 2016 13:09:43 +0800
Fam Zheng <famz@redhat.com> 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>
> 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(-)

Maybe merge patch 6?

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH v4 2/6] virtio: Introduce virtio_add_queue_aio

[Cornelia Huck]

On Wed, 13 Jul 2016 13:09:44 +0800
Fam Zheng <famz@redhat.com> wrote:

> 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;

After some thought: We don't need to migrate that, so your patch is
fine.

>      VirtIODevice *vdev;
>      EventNotifier guest_notifier;
>      EventNotifier host_notifier;

(...)

> +/* 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. */

/* Add a normal (non-AIO) virtqueue. */

?

> +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();

Add a blank line.

>      if (assign && set_handler) {

(...)

Other than the nits above:

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH v4 3/6] virtio-blk: Call virtio_add_queue_aio

[Cornelia Huck]

On Wed, 13 Jul 2016 13:09:45 +0800
Fam Zheng <famz@redhat.com> 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.

Not sure whether you really need to refer to an upcoming patch: I think
it is enough for the revert patch to refer to this and the virtio-scsi
patch. (I'm always a bit confused if I see things like this in the log
later on.)

> 
> 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) {

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH v4 4/6] virtio-scsi: Call virtio_add_queue_aio

[Cornelia Huck]

On Wed, 13 Jul 2016 13:09:46 +0800
Fam Zheng <famz@redhat.com> 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/scsi/virtio-scsi.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

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

[Cornelia Huck]

On Wed, 13 Jul 2016 13:09:47 +0800
Fam Zheng <famz@redhat.com> wrote:

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

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH v4 6/6] virtio-scsi: Replace HandleOutput typedef

[Cornelia Huck]

On Wed, 13 Jul 2016 13:09:48 +0800
Fam Zheng <famz@redhat.com> wrote:

> There is a new common one in virtio.h, use it.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/scsi/virtio-scsi.c           | 5 +++--
>  include/hw/virtio/virtio-scsi.h | 6 ++----
>  2 files changed, 5 insertions(+), 6 deletions(-)

I think you can merge this with patch 1, but anyway:

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

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

[Paolo Bonzini]

On 13/07/2016 07:09, Fam Zheng wrote:
> v4: Drop patch 1. [Cornelia]
>     Add the last trivial patch.
> 
> 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: 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"
>   virtio-scsi: Replace HandleOutput typedef
> 
>  block/mirror.c                  |  9 ---------
>  hw/block/virtio-blk.c           |  2 +-
>  hw/scsi/virtio-scsi.c           | 14 ++++++-------
>  hw/virtio/virtio.c              | 45 +++++++++++++++++++++++++++++++++--------
>  include/hw/virtio/virtio-scsi.h |  6 ++----
>  include/hw/virtio/virtio.h      |  8 ++++++--
>  6 files changed, 52 insertions(+), 32 deletions(-)
> 

I really would like this in 2.7, because it's a superior solution.

Paolo

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

[Stefan Hajnoczi]

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

On Wed, Jul 13, 2016 at 01:09:42PM +0800, Fam Zheng wrote:
> v4: Drop patch 1. [Cornelia]
>     Add the last trivial patch.
> 
> 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: 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"
>   virtio-scsi: Replace HandleOutput typedef
> 
>  block/mirror.c                  |  9 ---------
>  hw/block/virtio-blk.c           |  2 +-
>  hw/scsi/virtio-scsi.c           | 14 ++++++-------
>  hw/virtio/virtio.c              | 45 +++++++++++++++++++++++++++++++++--------
>  include/hw/virtio/virtio-scsi.h |  6 ++----
>  include/hw/virtio/virtio.h      |  8 ++++++--
>  6 files changed, 52 insertions(+), 32 deletions(-)
> 
> -- 
> 2.7.4
> 
> 

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

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

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

[Stefan Hajnoczi]

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

On Wed, Jul 13, 2016 at 12:32:59PM +0200, Paolo Bonzini wrote:
> On 13/07/2016 07:09, Fam Zheng wrote:
> > v4: Drop patch 1. [Cornelia]
> >     Add the last trivial patch.
> > 
> > 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: 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"
> >   virtio-scsi: Replace HandleOutput typedef
> > 
> >  block/mirror.c                  |  9 ---------
> >  hw/block/virtio-blk.c           |  2 +-
> >  hw/scsi/virtio-scsi.c           | 14 ++++++-------
> >  hw/virtio/virtio.c              | 45 +++++++++++++++++++++++++++++++++--------
> >  include/hw/virtio/virtio-scsi.h |  6 ++----
> >  include/hw/virtio/virtio.h      |  8 ++++++--
> >  6 files changed, 52 insertions(+), 32 deletions(-)
> > 
> 
> I really would like this in 2.7, because it's a superior solution.

This is going via Michael's virtio tree?

Stefan

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