[Qemu-devel] [PATCH v2 0/6] block: avoid recursive AioContext acquire in bdrv_inactivate_all()

[Qemu-devel] [PATCH v2 0/6] block: avoid recursive AioContext acquire in bdrv_inactivate_all()

[Stefan Hajnoczi]

v2:
 * Added docs/devel/multiple-iothreads.txt doc update [Kevin]
 * Added qemu-iotests 203 test case [Kevin]
 * Added iothread_stop() race fix to make 203 reliable

Patch 1 is Paolo's recursive locking removal in bdrv_inactivate_all().  This
solve migration hangs and is the main point of the patch series.

Patches 2-6 add a qemu-iotests test case and update the multiple-iothreads.txt
documentation to discourage recursive AioContext locking.

Based-on: <20171206144550.22295-1-stefanha@redhat.com>

Paolo Bonzini (1):
  block: avoid recursive AioContext acquire in bdrv_inactivate_all()

Stefan Hajnoczi (5):
  docs: mark nested AioContext locking as a legacy API
  blockdev: add x-blockdev-set-iothread force boolean
  iotests: add VM.add_object()
  iothread: fix iothread_stop() race condition
  qemu-iotests: add 203 savevm with IOThreads test

 docs/devel/multiple-iothreads.txt |  7 +++--
 qapi/block-core.json              |  6 +++-
 include/sysemu/iothread.h         |  3 +-
 block.c                           | 14 ++++++++--
 blockdev.c                        | 11 ++++----
 iothread.c                        | 20 +++++++++----
 tests/qemu-iotests/203            | 59 +++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/203.out        |  6 ++++
 tests/qemu-iotests/group          |  1 +
 tests/qemu-iotests/iotests.py     |  5 ++++
 10 files changed, 114 insertions(+), 18 deletions(-)
 create mode 100755 tests/qemu-iotests/203
 create mode 100644 tests/qemu-iotests/203.out

-- 
2.14.3

[Qemu-devel] [PATCH v2 1/6] block: avoid recursive AioContext acquire in bdrv_inactivate_all()

[Stefan Hajnoczi]

From: Paolo Bonzini <pbonzini@redhat.com>

BDRV_POLL_WHILE() does not support recursive AioContext locking.  It
only releases the AioContext lock once regardless of how many times the
caller has acquired it.  This results in a hang since the IOThread does
not make progress while the AioContext is still locked.

The following steps trigger the hang:

  $ qemu-system-x86_64 -M accel=kvm -m 1G -cpu host \
                       -object iothread,id=iothread0 \
                       -device virtio-scsi-pci,iothread=iothread0 \
                       -drive if=none,id=drive0,file=test.img,format=raw \
                       -device scsi-hd,drive=drive0 \
                       -drive if=none,id=drive1,file=test.img,format=raw \
                       -device scsi-hd,drive=drive1
  $ qemu-system-x86_64 ...same options... \
                       -incoming tcp::1234
  (qemu) migrate tcp:127.0.0.1:1234
  ...hang...

Tested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 9a1a0d1e73..1c37ce4554 100644
--- a/block.c
+++ b/block.c
@@ -4320,9 +4320,15 @@ int bdrv_inactivate_all(void)
     BdrvNextIterator it;
     int ret = 0;
     int pass;
+    GSList *aio_ctxs = NULL, *ctx;
 
     for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
-        aio_context_acquire(bdrv_get_aio_context(bs));
+        AioContext *aio_context = bdrv_get_aio_context(bs);
+
+        if (!g_slist_find(aio_ctxs, aio_context)) {
+            aio_ctxs = g_slist_prepend(aio_ctxs, aio_context);
+            aio_context_acquire(aio_context);
+        }
     }
 
     /* We do two passes of inactivation. The first pass calls to drivers'
@@ -4340,9 +4346,11 @@ int bdrv_inactivate_all(void)
     }
 
 out:
-    for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
-        aio_context_release(bdrv_get_aio_context(bs));
+    for (ctx = aio_ctxs; ctx != NULL; ctx = ctx->next) {
+        AioContext *aio_context = ctx->data;
+        aio_context_release(aio_context);
     }
+    g_slist_free(aio_ctxs);
 
     return ret;
 }
-- 
2.14.3

[Qemu-devel] [PATCH v2 2/6] docs: mark nested AioContext locking as a legacy API

[Stefan Hajnoczi]

See the patch for why nested AioContext locking is no longer allowed.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 docs/devel/multiple-iothreads.txt | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/docs/devel/multiple-iothreads.txt b/docs/devel/multiple-iothreads.txt
index e4d340bbb7..4f9012d154 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -1,4 +1,4 @@
-Copyright (c) 2014 Red Hat Inc.
+Copyright (c) 2014-2017 Red Hat Inc.
 
 This work is licensed under the terms of the GNU GPL, version 2 or later.  See
 the COPYING file in the top-level directory.
@@ -92,8 +92,9 @@ aio_context_acquire()/aio_context_release() for mutual exclusion.  Once the
 context is acquired no other thread can access it or run event loop iterations
 in this AioContext.
 
-aio_context_acquire()/aio_context_release() calls may be nested.  This
-means you can call them if you're not sure whether #2 applies.
+Legacy code sometimes nests aio_context_acquire()/aio_context_release() calls.
+Do not use nesting anymore, it is incompatible with the BDRV_POLL_WHILE() macro
+used in the block layer and can lead to hangs.
 
 There is currently no lock ordering rule if a thread needs to acquire multiple
 AioContexts simultaneously.  Therefore, it is only safe for code holding the
-- 
2.14.3

[Qemu-devel] [PATCH v2 3/6] blockdev: add x-blockdev-set-iothread force boolean

[Stefan Hajnoczi]

When a node is already associated with a BlockBackend the
x-blockdev-set-iothread command refuses to set the IOThread.  This is to
prevent accidentally changing the IOThread when the nodes are in use.

When the nodes are created with -drive they automatically get a
BlockBackend.  In that case we know nothing is using them yet and it's
safe to set the IOThread.  Add a force boolean to override the check.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qapi/block-core.json |  6 +++++-
 blockdev.c           | 11 ++++++-----
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 741d6c4367..a8cdbc300b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3962,6 +3962,9 @@
 #
 # @iothread: the name of the IOThread object or null for the main loop
 #
+# @force: true if the node and its children should be moved when a BlockBackend
+#         is already attached
+#
 # Note: this command is experimental and intended for test cases that need
 # control over IOThreads only.
 #
@@ -3984,4 +3987,5 @@
 ##
 { 'command': 'x-blockdev-set-iothread',
   'data' : { 'node-name': 'str',
-             'iothread': 'StrOrNull' } }
+             'iothread': 'StrOrNull',
+             '*force': 'bool' } }
diff --git a/blockdev.c b/blockdev.c
index f75c01f664..9c3a430cfb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4131,7 +4131,7 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
 }
 
 void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
-                                 Error **errp)
+                                 bool has_force, bool force, Error **errp)
 {
     AioContext *old_context;
     AioContext *new_context;
@@ -4143,10 +4143,11 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
         return;
     }
 
-    /* If we want to allow more extreme test scenarios this guard could be
-     * removed.  For now it protects against accidents. */
-    if (bdrv_has_blk(bs)) {
-        error_setg(errp, "Node %s is in use", node_name);
+    /* Protects against accidents. */
+    if (!(has_force && force) && bdrv_has_blk(bs)) {
+        error_setg(errp, "Node %s is associated with a BlockBackend and could "
+                         "be in use (use force=true to override this check)",
+                         node_name);
         return;
     }
 
-- 
2.14.3

[Qemu-devel] [PATCH v2 4/6] iotests: add VM.add_object()

[Stefan Hajnoczi]

The VM.add_object() method can be used to add IOThreads or memory
backend objects.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/iotests.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6f057904a9..44477e9295 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -197,6 +197,11 @@ class VM(qtest.QEMUQtestMachine):
                                  socket_scm_helper=socket_scm_helper)
         self._num_drives = 0
 
+    def add_object(self, opts):
+        self._args.append('-object')
+        self._args.append(opts)
+        return self
+
     def add_device(self, opts):
         self._args.append('-device')
         self._args.append(opts)
-- 
2.14.3

[Qemu-devel] [PATCH v2 5/6] iothread: fix iothread_stop() race condition

[Stefan Hajnoczi]

There is a small chance that iothread_stop() hangs as follows:

  Thread 3 (Thread 0x7f63eba5f700 (LWP 16105)):
  #0  0x00007f64012c09b6 in ppoll () at /lib64/libc.so.6
  #1  0x000055959992eac9 in ppoll (__ss=0x0, __timeout=0x0, __nfds=<optimized out>, __fds=<optimized out>) at /usr/include/bits/poll2.h:77
  #2  0x000055959992eac9 in qemu_poll_ns (fds=<optimized out>, nfds=<optimized out>, timeout=<optimized out>) at util/qemu-timer.c:322
  #3  0x0000559599930711 in aio_poll (ctx=0x55959bdb83c0, blocking=blocking@entry=true) at util/aio-posix.c:629
  #4  0x00005595996806fe in iothread_run (opaque=0x55959bd78400) at iothread.c:59
  #5  0x00007f640159f609 in start_thread () at /lib64/libpthread.so.0
  #6  0x00007f64012cce6f in clone () at /lib64/libc.so.6

  Thread 1 (Thread 0x7f640b45b280 (LWP 16103)):
  #0  0x00007f64015a0b6d in pthread_join () at /lib64/libpthread.so.0
  #1  0x00005595999332ef in qemu_thread_join (thread=<optimized out>) at util/qemu-thread-posix.c:547
  #2  0x00005595996808ae in iothread_stop (iothread=<optimized out>) at iothread.c:91
  #3  0x000055959968094d in iothread_stop_iter (object=<optimized out>, opaque=<optimized out>) at iothread.c:102
  #4  0x0000559599857d97 in do_object_child_foreach (obj=obj@entry=0x55959bdb8100, fn=fn@entry=0x559599680930 <iothread_stop_iter>, opaque=opaque@entry=0x0, recurse=recurse@entry=false) at qom/object.c:852
  #5  0x0000559599859477 in object_child_foreach (obj=obj@entry=0x55959bdb8100, fn=fn@entry=0x559599680930 <iothread_stop_iter>, opaque=opaque@entry=0x0) at qom/object.c:867
  #6  0x0000559599680a6e in iothread_stop_all () at iothread.c:341
  #7  0x000055959955b1d5 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at vl.c:4913

The relevant code from iothread_run() is:

  while (!atomic_read(&iothread->stopping)) {
      aio_poll(iothread->ctx, true);

and iothread_stop():

  iothread->stopping = true;
  aio_notify(iothread->ctx);
  ...
  qemu_thread_join(&iothread->thread);

The following scenario can occur:

1. IOThread:
  while (!atomic_read(&iothread->stopping)) -> stopping=false

2. Main loop:
  iothread->stopping = true;
  aio_notify(iothread->ctx);

3. IOThread:
  aio_poll(iothread->ctx, true); -> hang

The bug is explained by the AioContext->notify_me doc comments:

  "If this field is 0, everything (file descriptors, bottom halves,
  timers) will be re-evaluated before the next blocking poll(), thus the
  event_notifier_set call can be skipped."

The problem is that "everything" does not include checking
iothread->stopping.  This means iothread_run() will block in aio_poll()
if aio_notify() was called just before aio_poll().

This patch fixes the hang by replacing aio_notify() with
aio_bh_schedule_oneshot().  This makes aio_poll() or g_main_loop_run()
to return.

Implementing this properly required a new bool running flag.  The new
flag prevents races that are tricky if we try to use iothread->stopping.
Now iothread->stopping is purely for iothread_stop() and
iothread->running is purely for the iothread_run() thread.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
I'm including this patch in this series because it is needed to make the
test case reliable.  It's unrelated to the main goal of the patch
series.
---
 include/sysemu/iothread.h |  3 ++-
 iothread.c                | 20 +++++++++++++++-----
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
index 55de1715c7..799614ffd2 100644
--- a/include/sysemu/iothread.h
+++ b/include/sysemu/iothread.h
@@ -29,7 +29,8 @@ typedef struct {
     GOnce once;
     QemuMutex init_done_lock;
     QemuCond init_done_cond;    /* is thread initialization done? */
-    bool stopping;
+    bool stopping;              /* has iothread_stop() been called? */
+    bool running;               /* should iothread_run() continue? */
     int thread_id;
 
     /* AioContext poll parameters */
diff --git a/iothread.c b/iothread.c
index e7b93e02a3..d8b6c1fb27 100644
--- a/iothread.c
+++ b/iothread.c
@@ -55,7 +55,7 @@ static void *iothread_run(void *opaque)
     qemu_cond_signal(&iothread->init_done_cond);
     qemu_mutex_unlock(&iothread->init_done_lock);
 
-    while (!atomic_read(&iothread->stopping)) {
+    while (iothread->running) {
         aio_poll(iothread->ctx, true);
 
         if (atomic_read(&iothread->worker_context)) {
@@ -78,16 +78,25 @@ static void *iothread_run(void *opaque)
     return NULL;
 }
 
+/* Runs in iothread_run() thread */
+static void iothread_stop_bh(void *opaque)
+{
+    IOThread *iothread = opaque;
+
+    iothread->running = false; /* stop iothread_run() */
+
+    if (iothread->main_loop) {
+        g_main_loop_quit(iothread->main_loop);
+    }
+}
+
 void iothread_stop(IOThread *iothread)
 {
     if (!iothread->ctx || iothread->stopping) {
         return;
     }
     iothread->stopping = true;
-    aio_notify(iothread->ctx);
-    if (atomic_read(&iothread->main_loop)) {
-        g_main_loop_quit(iothread->main_loop);
-    }
+    aio_bh_schedule_oneshot(iothread->ctx, iothread_stop_bh, iothread);
     qemu_thread_join(&iothread->thread);
 }
 
@@ -134,6 +143,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
     char *name, *thread_name;
 
     iothread->stopping = false;
+    iothread->running = true;
     iothread->thread_id = -1;
     iothread->ctx = aio_context_new(&local_error);
     if (!iothread->ctx) {
-- 
2.14.3

[Qemu-devel] [PATCH v2 6/6] qemu-iotests: add 203 savevm with IOThreads test

[Stefan Hajnoczi]

This test case will prevent future regressions with savevm and
IOThreads.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/203     | 59 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/203.out |  6 +++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 66 insertions(+)
 create mode 100755 tests/qemu-iotests/203
 create mode 100644 tests/qemu-iotests/203.out

diff --git a/tests/qemu-iotests/203 b/tests/qemu-iotests/203
new file mode 100755
index 0000000000..2c811917d8
--- /dev/null
+++ b/tests/qemu-iotests/203
@@ -0,0 +1,59 @@
+#!/usr/bin/env python
+#
+# Copyright (C) 2017 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# Creator/Owner: Stefan Hajnoczi <stefanha@redhat.com>
+#
+# Check that QMP 'migrate' with multiple drives on a single IOThread completes
+# successfully.  This particular command triggered a hang in the source QEMU
+# process due to recursive AioContext locking in bdrv_invalidate_all() and
+# BDRV_POLL_WHILE().
+
+import iotests
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.verify_platform(['linux'])
+
+with iotests.FilePath('disk0.img') as disk0_img_path, \
+     iotests.FilePath('disk1.img') as disk1_img_path, \
+     iotests.VM() as vm:
+
+    img_size = '10M'
+    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk0_img_path, img_size)
+    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk1_img_path, img_size)
+
+    iotests.log('Launching VM...')
+    (vm.add_object('iothread,id=iothread0')
+       .add_drive(disk0_img_path, 'node-name=drive0-node', interface='none')
+       .add_drive(disk1_img_path, 'node-name=drive1-node', interface='none')
+       .launch())
+
+    iotests.log('Setting IOThreads...')
+    iotests.log(vm.qmp('x-blockdev-set-iothread',
+                       node_name='drive0-node', iothread='iothread0',
+                       force=True))
+    iotests.log(vm.qmp('x-blockdev-set-iothread',
+                       node_name='drive1-node', iothread='iothread0',
+                       force=True))
+
+    iotests.log('Starting migration...')
+    iotests.log(vm.qmp('migrate', uri='exec:cat >/dev/null'))
+    while True:
+        vm.get_qmp_event(wait=60.0)
+        result = vm.qmp('query-migrate')
+        status = result.get('return', {}).get('status', None)
+        if status == 'completed':
+            break
diff --git a/tests/qemu-iotests/203.out b/tests/qemu-iotests/203.out
new file mode 100644
index 0000000000..3f1ff900e4
--- /dev/null
+++ b/tests/qemu-iotests/203.out
@@ -0,0 +1,6 @@
+Launching VM...
+Setting IOThreads...
+{u'return': {}}
+{u'return': {}}
+Starting migration...
+{u'return': {}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d0ee1e2e55..93d96fb22f 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -198,3 +198,4 @@
 198 rw auto
 200 rw auto
 202 rw auto quick
+203 rw auto
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 1/6] block: avoid recursive AioContext acquire in bdrv_inactivate_all()

[Eric Blake]

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

On 12/07/2017 02:13 PM, Stefan Hajnoczi wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> BDRV_POLL_WHILE() does not support recursive AioContext locking.  It
> only releases the AioContext lock once regardless of how many times the
> caller has acquired it.  This results in a hang since the IOThread does
> not make progress while the AioContext is still locked.
> 
> The following steps trigger the hang:
> 
>   $ qemu-system-x86_64 -M accel=kvm -m 1G -cpu host \
>                        -object iothread,id=iothread0 \
>                        -device virtio-scsi-pci,iothread=iothread0 \
>                        -drive if=none,id=drive0,file=test.img,format=raw \
>                        -device scsi-hd,drive=drive0 \
>                        -drive if=none,id=drive1,file=test.img,format=raw \
>                        -device scsi-hd,drive=drive1
>   $ qemu-system-x86_64 ...same options... \
>                        -incoming tcp::1234
>   (qemu) migrate tcp:127.0.0.1:1234
>   ...hang...
> 
> Tested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 2/6] docs: mark nested AioContext locking as a legacy API

[Eric Blake]

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

On 12/07/2017 02:13 PM, Stefan Hajnoczi wrote:
> See the patch for why nested AioContext locking is no longer allowed.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  docs/devel/multiple-iothreads.txt | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 3/6] blockdev: add x-blockdev-set-iothread force boolean

[Eric Blake]

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

On 12/07/2017 02:13 PM, Stefan Hajnoczi wrote:
> When a node is already associated with a BlockBackend the
> x-blockdev-set-iothread command refuses to set the IOThread.  This is to
> prevent accidentally changing the IOThread when the nodes are in use.
> 
> When the nodes are created with -drive they automatically get a
> BlockBackend.  In that case we know nothing is using them yet and it's
> safe to set the IOThread.  Add a force boolean to override the check.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  qapi/block-core.json |  6 +++++-
>  blockdev.c           | 11 ++++++-----
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 4/6] iotests: add VM.add_object()

[Eric Blake]

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

On 12/07/2017 02:13 PM, Stefan Hajnoczi wrote:
> The VM.add_object() method can be used to add IOThreads or memory
> backend objects.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 5 +++++
>  1 file changed, 5 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 5/6] iothread: fix iothread_stop() race condition

[Eric Blake]

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

On 12/07/2017 02:13 PM, Stefan Hajnoczi wrote:
> There is a small chance that iothread_stop() hangs as follows:
> 

> 
> The bug is explained by the AioContext->notify_me doc comments:
> 
>   "If this field is 0, everything (file descriptors, bottom halves,
>   timers) will be re-evaluated before the next blocking poll(), thus the
>   event_notifier_set call can be skipped."
> 
> The problem is that "everything" does not include checking
> iothread->stopping.  This means iothread_run() will block in aio_poll()
> if aio_notify() was called just before aio_poll().
> 
> This patch fixes the hang by replacing aio_notify() with
> aio_bh_schedule_oneshot().  This makes aio_poll() or g_main_loop_run()
> to return.

s/to //

> 
> Implementing this properly required a new bool running flag.  The new
> flag prevents races that are tricky if we try to use iothread->stopping.
> Now iothread->stopping is purely for iothread_stop() and
> iothread->running is purely for the iothread_run() thread.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> I'm including this patch in this series because it is needed to make the
> test case reliable.  It's unrelated to the main goal of the patch
> series.
> ---
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 6/6] qemu-iotests: add 203 savevm with IOThreads test

[Eric Blake]

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

On 12/07/2017 02:13 PM, Stefan Hajnoczi wrote:
> This test case will prevent future regressions with savevm and
> IOThreads.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qemu-iotests/203     | 59 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/203.out |  6 +++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 66 insertions(+)
>  create mode 100755 tests/qemu-iotests/203
>  create mode 100644 tests/qemu-iotests/203.out
> 

> +#
> +# Creator/Owner: Stefan Hajnoczi <stefanha@redhat.com>

Again, not sure we need this line.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 0/6] block: avoid recursive AioContext acquire in bdrv_inactivate_all()

[Paolo Bonzini]

On 07/12/2017 21:13, Stefan Hajnoczi wrote:
> v2:
>  * Added docs/devel/multiple-iothreads.txt doc update [Kevin]
>  * Added qemu-iotests 203 test case [Kevin]
>  * Added iothread_stop() race fix to make 203 reliable
> 
> Patch 1 is Paolo's recursive locking removal in bdrv_inactivate_all().  This
> solve migration hangs and is the main point of the patch series.
> 
> Patches 2-6 add a qemu-iotests test case and update the multiple-iothreads.txt
> documentation to discourage recursive AioContext locking.
> 
> Based-on: <20171206144550.22295-1-stefanha@redhat.com>
> 
> Paolo Bonzini (1):
>   block: avoid recursive AioContext acquire in bdrv_inactivate_all()
> 
> Stefan Hajnoczi (5):
>   docs: mark nested AioContext locking as a legacy API
>   blockdev: add x-blockdev-set-iothread force boolean
>   iotests: add VM.add_object()
>   iothread: fix iothread_stop() race condition
>   qemu-iotests: add 203 savevm with IOThreads test
> 
>  docs/devel/multiple-iothreads.txt |  7 +++--
>  qapi/block-core.json              |  6 +++-
>  include/sysemu/iothread.h         |  3 +-
>  block.c                           | 14 ++++++++--
>  blockdev.c                        | 11 ++++----
>  iothread.c                        | 20 +++++++++----
>  tests/qemu-iotests/203            | 59 +++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/203.out        |  6 ++++
>  tests/qemu-iotests/group          |  1 +
>  tests/qemu-iotests/iotests.py     |  5 ++++
>  10 files changed, 114 insertions(+), 18 deletions(-)
>  create mode 100755 tests/qemu-iotests/203
>  create mode 100644 tests/qemu-iotests/203.out
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

aio_notify should also be put on the blacklist of things that have
slowly become bad ideas. :|

Paolo

Re: [Qemu-devel] [PATCH v2 0/6] block: avoid recursive AioContext acquire in bdrv_inactivate_all()

[Stefan Hajnoczi]

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

On Thu, Dec 07, 2017 at 08:13:14PM +0000, Stefan Hajnoczi wrote:
> v2:
>  * Added docs/devel/multiple-iothreads.txt doc update [Kevin]
>  * Added qemu-iotests 203 test case [Kevin]
>  * Added iothread_stop() race fix to make 203 reliable
> 
> Patch 1 is Paolo's recursive locking removal in bdrv_inactivate_all().  This
> solve migration hangs and is the main point of the patch series.
> 
> Patches 2-6 add a qemu-iotests test case and update the multiple-iothreads.txt
> documentation to discourage recursive AioContext locking.
> 
> Based-on: <20171206144550.22295-1-stefanha@redhat.com>
> 
> Paolo Bonzini (1):
>   block: avoid recursive AioContext acquire in bdrv_inactivate_all()
> 
> Stefan Hajnoczi (5):
>   docs: mark nested AioContext locking as a legacy API
>   blockdev: add x-blockdev-set-iothread force boolean
>   iotests: add VM.add_object()
>   iothread: fix iothread_stop() race condition
>   qemu-iotests: add 203 savevm with IOThreads test
> 
>  docs/devel/multiple-iothreads.txt |  7 +++--
>  qapi/block-core.json              |  6 +++-
>  include/sysemu/iothread.h         |  3 +-
>  block.c                           | 14 ++++++++--
>  blockdev.c                        | 11 ++++----
>  iothread.c                        | 20 +++++++++----
>  tests/qemu-iotests/203            | 59 +++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/203.out        |  6 ++++
>  tests/qemu-iotests/group          |  1 +
>  tests/qemu-iotests/iotests.py     |  5 ++++
>  10 files changed, 114 insertions(+), 18 deletions(-)
>  create mode 100755 tests/qemu-iotests/203
>  create mode 100644 tests/qemu-iotests/203.out
> 
> -- 
> 2.14.3
> 

Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan

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