[Qemu-devel] [PATCH v2 0/4] vl: introduce vm_shutdown()

[Qemu-devel] [PATCH v2 0/4] vl: introduce vm_shutdown()

[Stefan Hajnoczi]

v2:
 * Tackle the .ioeventfd_stop() vs vq handler race by removing the ioeventfd
   from a BH in the IOThread [Fam]

There are several race conditions in virtio-blk/virtio-scsi dataplane code.
This patch series addresses them, see the commit description for details on the
individual cases.

Stefan Hajnoczi (4):
  block: add aio_wait_bh_oneshot()
  virtio-blk: fix race between .ioeventfd_stop() and vq handler
  virtio-scsi: fix race between .ioeventfd_stop() and vq handler
  vl: introduce vm_shutdown()

 include/block/aio-wait.h        | 13 +++++++++++++
 include/sysemu/iothread.h       |  1 -
 include/sysemu/sysemu.h         |  1 +
 cpus.c                          | 16 +++++++++++++---
 hw/block/dataplane/virtio-blk.c | 24 +++++++++++++++++-------
 hw/scsi/virtio-scsi-dataplane.c |  9 +++++----
 iothread.c                      | 31 -------------------------------
 util/aio-wait.c                 | 31 +++++++++++++++++++++++++++++++
 vl.c                            | 13 +++----------
 9 files changed, 83 insertions(+), 56 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v2 1/4] block: add aio_wait_bh_oneshot()

[Stefan Hajnoczi]

Sometimes it's necessary for the main loop thread to run a BH in an
IOThread and wait for its completion.  This primitive is useful during
startup/shutdown to synchronize and avoid race conditions.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/block/aio-wait.h | 13 +++++++++++++
 util/aio-wait.c          | 31 +++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
index a48c744fa8..f7a3972200 100644
--- a/include/block/aio-wait.h
+++ b/include/block/aio-wait.h
@@ -113,4 +113,17 @@ typedef struct {
  */
 void aio_wait_kick(AioWait *wait);
 
+/**
+ * aio_wait_bh_oneshot:
+ * @ctx: the aio context
+ * @cb: the BH callback function
+ * @opaque: user data for the BH callback function
+ *
+ * Run a BH in @ctx and wait for it to complete.
+ *
+ * Must be called from the main loop thread with @ctx acquired exactly once.
+ * Note that main loop event processing may occur.
+ */
+void aio_wait_bh_oneshot(AioContext *ctx, QEMUBHFunc *cb, void *opaque);
+
 #endif /* QEMU_AIO_WAIT */
diff --git a/util/aio-wait.c b/util/aio-wait.c
index a487cdb852..975afddf4c 100644
--- a/util/aio-wait.c
+++ b/util/aio-wait.c
@@ -38,3 +38,34 @@ void aio_wait_kick(AioWait *wait)
         aio_bh_schedule_oneshot(qemu_get_aio_context(), dummy_bh_cb, NULL);
     }
 }
+
+typedef struct {
+    AioWait wait;
+    bool done;
+    QEMUBHFunc *cb;
+    void *opaque;
+} AioWaitBHData;
+
+/* Context: BH in IOThread */
+static void aio_wait_bh(void *opaque)
+{
+    AioWaitBHData *data = opaque;
+
+    data->cb(data->opaque);
+
+    data->done = true;
+    aio_wait_kick(&data->wait);
+}
+
+void aio_wait_bh_oneshot(AioContext *ctx, QEMUBHFunc *cb, void *opaque)
+{
+    AioWaitBHData data = {
+        .cb = cb,
+        .opaque = opaque,
+    };
+
+    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
+
+    aio_bh_schedule_oneshot(ctx, aio_wait_bh, &data);
+    AIO_WAIT_WHILE(&data.wait, ctx, !data.done);
+}
-- 
2.14.3

[Qemu-devel] [PATCH v2 2/4] virtio-blk: fix race between .ioeventfd_stop() and vq handler

[Stefan Hajnoczi]

If the main loop thread invokes .ioeventfd_stop() just as the vq handler
function begins in the IOThread then the handler may lose the race for
the AioContext lock.  By the time the vq handler is able to acquire the
AioContext lock the ioeventfd has already been removed and the handler
isn't supposed to run anymore!

Use the new aio_wait_bh_oneshot() function to perform ioeventfd removal
from within the IOThread.  This way no races with the vq handler are
possible.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 2cb990997e..d3bb09bc4e 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -229,6 +229,22 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
     return -ENOSYS;
 }
 
+/* Stop notifications for new requests from guest.
+ *
+ * Context: BH in IOThread
+ */
+static void virtio_blk_data_plane_stop_bh(void *opaque)
+{
+    VirtIOBlockDataPlane *s = opaque;
+    unsigned i;
+
+    for (i = 0; i < s->conf->num_queues; i++) {
+        VirtQueue *vq = virtio_get_queue(s->vdev, i);
+
+        virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, NULL);
+    }
+}
+
 /* Context: QEMU global mutex held */
 void virtio_blk_data_plane_stop(VirtIODevice *vdev)
 {
@@ -253,13 +269,7 @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
     trace_virtio_blk_data_plane_stop(s);
 
     aio_context_acquire(s->ctx);
-
-    /* Stop notifications for new requests from guest */
-    for (i = 0; i < nvqs; i++) {
-        VirtQueue *vq = virtio_get_queue(s->vdev, i);
-
-        virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, NULL);
-    }
+    aio_wait_bh_oneshot(s->ctx, virtio_blk_data_plane_stop_bh, s);
 
     /* Drain and switch bs back to the QEMU main loop */
     blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context());
-- 
2.14.3

[Qemu-devel] [PATCH v2 3/4] virtio-scsi: fix race between .ioeventfd_stop() and vq handler

[Stefan Hajnoczi]

If the main loop thread invokes .ioeventfd_stop() just as the vq handler
function begins in the IOThread then the handler may lose the race for
the AioContext lock.  By the time the vq handler is able to acquire the
AioContext lock the ioeventfd has already been removed and the handler
isn't supposed to run anymore!

Use the new aio_wait_bh_oneshot() function to perform ioeventfd removal
from within the IOThread.  This way no races with the vq handler are
possible.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/scsi/virtio-scsi-dataplane.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 1c33322ba6..912e5005d8 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -107,9 +107,10 @@ static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n,
     return 0;
 }
 
-/* assumes s->ctx held */
-static void virtio_scsi_clear_aio(VirtIOSCSI *s)
+/* Context: BH in IOThread */
+static void virtio_scsi_dataplane_stop_bh(void *opaque)
 {
+    VirtIOSCSI *s = opaque;
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
     int i;
 
@@ -171,7 +172,7 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
     return 0;
 
 fail_vrings:
-    virtio_scsi_clear_aio(s);
+    aio_wait_bh_oneshot(s->ctx, virtio_scsi_dataplane_stop_bh, s);
     aio_context_release(s->ctx);
     for (i = 0; i < vs->conf.num_queues + 2; i++) {
         virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
@@ -207,7 +208,7 @@ void virtio_scsi_dataplane_stop(VirtIODevice *vdev)
     s->dataplane_stopping = true;
 
     aio_context_acquire(s->ctx);
-    virtio_scsi_clear_aio(s);
+    aio_wait_bh_oneshot(s->ctx, virtio_scsi_dataplane_stop_bh, s);
     aio_context_release(s->ctx);
 
     blk_drain_all(); /* ensure there are no in-flight requests */
-- 
2.14.3

[Qemu-devel] [PATCH v2 4/4] vl: introduce vm_shutdown()

[Stefan Hajnoczi]

Commit 00d09fdbbae5f7864ce754913efc84c12fdf9f1a ("vl: pause vcpus before
stopping iothreads") and commit dce8921b2baaf95974af8176406881872067adfa
("iothread: Stop threads before main() quits") tried to work around the
fact that emulation was still active during termination by stopping
iothreads.  They suffer from race conditions:
1. virtio_scsi_handle_cmd_vq() racing with iothread_stop_all() hits the
   virtio_scsi_ctx_check() assertion failure because the BDS AioContext
   has been modified by iothread_stop_all().
2. Guest vq kick racing with main loop termination leaves a readable
   ioeventfd that is handled by the next aio_poll() when external
   clients are enabled again, resulting in unwanted emulation activity.

This patch obsoletes those commits by fully disabling emulation activity
when vcpus are stopped.

Use the new vm_shutdown() function instead of pause_all_vcpus() so that
vm change state handlers are invoked too.  Virtio devices will now stop
their ioeventfds, preventing further emulation activity after vm_stop().

Note that vm_stop(RUN_STATE_SHUTDOWN) cannot be used because it emits a
QMP STOP event that may affect existing clients.

It is no longer necessary to call replay_disable_events() directly since
vm_shutdown() does so already.

Drop iothread_stop_all() since it is no longer used.

Cc: Fam Zheng <famz@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/sysemu/iothread.h |  1 -
 include/sysemu/sysemu.h   |  1 +
 cpus.c                    | 16 +++++++++++++---
 iothread.c                | 31 -------------------------------
 vl.c                      | 13 +++----------
 5 files changed, 17 insertions(+), 45 deletions(-)

diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
index 799614ffd2..8a7ac2c528 100644
--- a/include/sysemu/iothread.h
+++ b/include/sysemu/iothread.h
@@ -45,7 +45,6 @@ typedef struct {
 char *iothread_get_id(IOThread *iothread);
 IOThread *iothread_by_id(const char *id);
 AioContext *iothread_get_aio_context(IOThread *iothread);
-void iothread_stop_all(void);
 GMainContext *iothread_get_g_main_context(IOThread *iothread);
 
 /*
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 77bb3da582..54f91dbc03 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -55,6 +55,7 @@ void vm_start(void);
 int vm_prepare_start(void);
 int vm_stop(RunState state);
 int vm_stop_force_state(RunState state);
+int vm_shutdown(void);
 
 typedef enum WakeupReason {
     /* Always keep QEMU_WAKEUP_REASON_NONE = 0 */
diff --git a/cpus.c b/cpus.c
index f298b659f4..90279f73fc 100644
--- a/cpus.c
+++ b/cpus.c
@@ -993,7 +993,7 @@ void cpu_synchronize_all_pre_loadvm(void)
     }
 }
 
-static int do_vm_stop(RunState state)
+static int do_vm_stop(RunState state, bool send_stop)
 {
     int ret = 0;
 
@@ -1002,7 +1002,9 @@ static int do_vm_stop(RunState state)
         pause_all_vcpus();
         runstate_set(state);
         vm_state_notify(0, state);
-        qapi_event_send_stop(&error_abort);
+        if (send_stop) {
+            qapi_event_send_stop(&error_abort);
+        }
     }
 
     bdrv_drain_all();
@@ -1012,6 +1014,14 @@ static int do_vm_stop(RunState state)
     return ret;
 }
 
+/* Special vm_stop() variant for terminating the process.  Historically clients
+ * did not expect a QMP STOP event and so we need to retain compatibility.
+ */
+int vm_shutdown(void)
+{
+    return do_vm_stop(RUN_STATE_SHUTDOWN, false);
+}
+
 static bool cpu_can_run(CPUState *cpu)
 {
     if (cpu->stop) {
@@ -2007,7 +2017,7 @@ int vm_stop(RunState state)
         return 0;
     }
 
-    return do_vm_stop(state);
+    return do_vm_stop(state, true);
 }
 
 /**
diff --git a/iothread.c b/iothread.c
index 4b9bbde4cd..68d92086e3 100644
--- a/iothread.c
+++ b/iothread.c
@@ -101,18 +101,6 @@ void iothread_stop(IOThread *iothread)
     qemu_thread_join(&iothread->thread);
 }
 
-static int iothread_stop_iter(Object *object, void *opaque)
-{
-    IOThread *iothread;
-
-    iothread = (IOThread *)object_dynamic_cast(object, TYPE_IOTHREAD);
-    if (!iothread) {
-        return 0;
-    }
-    iothread_stop(iothread);
-    return 0;
-}
-
 static void iothread_instance_init(Object *obj)
 {
     IOThread *iothread = IOTHREAD(obj);
@@ -333,25 +321,6 @@ IOThreadInfoList *qmp_query_iothreads(Error **errp)
     return head;
 }
 
-void iothread_stop_all(void)
-{
-    Object *container = object_get_objects_root();
-    BlockDriverState *bs;
-    BdrvNextIterator it;
-
-    for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
-        AioContext *ctx = bdrv_get_aio_context(bs);
-        if (ctx == qemu_get_aio_context()) {
-            continue;
-        }
-        aio_context_acquire(ctx);
-        bdrv_set_aio_context(bs, qemu_get_aio_context());
-        aio_context_release(ctx);
-    }
-
-    object_child_foreach(container, iothread_stop_iter, NULL);
-}
-
 static gpointer iothread_g_main_context_init(gpointer opaque)
 {
     AioContext *ctx;
diff --git a/vl.c b/vl.c
index 9e7235df6d..de719ae756 100644
--- a/vl.c
+++ b/vl.c
@@ -4755,17 +4755,10 @@ int main(int argc, char **argv, char **envp)
     os_setup_post();
 
     main_loop();
-    replay_disable_events();
 
-    /* The ordering of the following is delicate.  Stop vcpus to prevent new
-     * I/O requests being queued by the guest.  Then stop IOThreads (this
-     * includes a drain operation and completes all request processing).  At
-     * this point emulated devices are still associated with their IOThreads
-     * (if any) but no longer have any work to do.  Only then can we close
-     * block devices safely because we know there is no more I/O coming.
-     */
-    pause_all_vcpus();
-    iothread_stop_all();
+    /* No more vcpu or device emulation activity beyond this point */
+    vm_shutdown();
+
     bdrv_close_all();
 
     res_free();
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 0/4] vl: introduce vm_shutdown()

[Fam Zheng]

On Wed, 02/28 18:19, Stefan Hajnoczi wrote:
> v2:
>  * Tackle the .ioeventfd_stop() vs vq handler race by removing the ioeventfd
>    from a BH in the IOThread [Fam]
> 
> There are several race conditions in virtio-blk/virtio-scsi dataplane code.
> This patch series addresses them, see the commit description for details on the
> individual cases.

This doesn't apply to master. Which branch is it based off?

Fam

Re: [Qemu-devel] [PATCH v2 0/4] vl: introduce vm_shutdown()

[Stefan Hajnoczi]

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

On Thu, Mar 01, 2018 at 09:15:17AM +0800, Fam Zheng wrote:
> On Wed, 02/28 18:19, Stefan Hajnoczi wrote:
> > v2:
> >  * Tackle the .ioeventfd_stop() vs vq handler race by removing the ioeventfd
> >    from a BH in the IOThread [Fam]
> > 
> > There are several race conditions in virtio-blk/virtio-scsi dataplane code.
> > This patch series addresses them, see the commit description for details on the
> > individual cases.
> 
> This doesn't apply to master. Which branch is it based off?

Sorry, I was going to look up the Based-on: syntax and then forgot.  I'm
not sure where it's documented, here is a guess at how it works:

Based-on: "[PATCH v3 0/5] block: fix blk_aio_*() segfault when blk->root == NULL" <20180216165015.30715-1-stefanha@redhat.com>

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

Re: [Qemu-devel] [PATCH v2 0/4] vl: introduce vm_shutdown()

[Fam Zheng]

On Thu, 03/01 14:54, Stefan Hajnoczi wrote:
> On Thu, Mar 01, 2018 at 09:15:17AM +0800, Fam Zheng wrote:
> > On Wed, 02/28 18:19, Stefan Hajnoczi wrote:
> > > v2:
> > >  * Tackle the .ioeventfd_stop() vs vq handler race by removing the ioeventfd
> > >    from a BH in the IOThread [Fam]
> > > 
> > > There are several race conditions in virtio-blk/virtio-scsi dataplane code.
> > > This patch series addresses them, see the commit description for details on the
> > > individual cases.
> > 
> > This doesn't apply to master. Which branch is it based off?
> 
> Sorry, I was going to look up the Based-on: syntax and then forgot.  I'm
> not sure where it's documented, here is a guess at how it works:
> 
> Based-on: "[PATCH v3 0/5] block: fix blk_aio_*() segfault when blk->root == NULL" <20180216165015.30715-1-stefanha@redhat.com>

Sorry for the poor documentation of Patchew, we should improve the situation
soon:

Based-on: <20180216165015.30715-1-stefanha@redhat.com>
([PATCH v3 0/5] block: fix blk_aio_*() segfault when blk->root == NULL)

(Only the "Based-on:" line matters)

Fam

Re: [Qemu-devel] [PATCH v2 0/4] vl: introduce vm_shutdown()

[Stefan Hajnoczi]

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

On Fri, Mar 02, 2018 at 08:58:26AM +0800, Fam Zheng wrote:
> On Thu, 03/01 14:54, Stefan Hajnoczi wrote:
> > On Thu, Mar 01, 2018 at 09:15:17AM +0800, Fam Zheng wrote:
> > > On Wed, 02/28 18:19, Stefan Hajnoczi wrote:
> > > > v2:
> > > >  * Tackle the .ioeventfd_stop() vs vq handler race by removing the ioeventfd
> > > >    from a BH in the IOThread [Fam]
> > > > 
> > > > There are several race conditions in virtio-blk/virtio-scsi dataplane code.
> > > > This patch series addresses them, see the commit description for details on the
> > > > individual cases.
> > > 
> > > This doesn't apply to master. Which branch is it based off?
> > 
> > Sorry, I was going to look up the Based-on: syntax and then forgot.  I'm
> > not sure where it's documented, here is a guess at how it works:
> > 
> > Based-on: "[PATCH v3 0/5] block: fix blk_aio_*() segfault when blk->root == NULL" <20180216165015.30715-1-stefanha@redhat.com>
> 
> Sorry for the poor documentation of Patchew, we should improve the situation
> soon:

It would be nice if the patchew.org front page linked to a guide for
patch authors that documents available features.

Stefan

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