[Qemu-devel] [PULL 0/3] Block patches

[Qemu-devel] [PULL 0/3] Block patches

[Stefan Hajnoczi]

The following changes since commit f6b06fcceef465de0cf2514c9f76fe0192896781:

  Merge remote-tracking branch 'remotes/kraxel/tags/ui-20190121-pull-request' into staging (2019-01-23 17:57:47 +0000)

are available in the Git repository at:

  git://github.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to 8595685986152334b1ec28c78cb0e5e855d56b54:

  qemu-coroutine-sleep: drop CoSleepCB (2019-01-24 10:05:16 +0000)

----------------------------------------------------------------
Pull request

Changelog: No user-visible changes.

----------------------------------------------------------------

Stefan Hajnoczi (2):
  throttle-groups: fix restart coroutine iothread race
  iotests: add 238 for throttling tgm unregister iothread segfault

Vladimir Sementsov-Ogievskiy (1):
  qemu-coroutine-sleep: drop CoSleepCB

 include/block/throttle-groups.h |  5 ++++
 block/throttle-groups.c         |  9 +++++++
 util/qemu-coroutine-sleep.c     | 27 +++++++------------
 tests/qemu-iotests/238          | 47 +++++++++++++++++++++++++++++++++
 tests/qemu-iotests/238.out      |  6 +++++
 tests/qemu-iotests/group        |  1 +
 6 files changed, 78 insertions(+), 17 deletions(-)
 create mode 100755 tests/qemu-iotests/238
 create mode 100644 tests/qemu-iotests/238.out

-- 
2.20.1

[Qemu-devel] [PULL 1/3] throttle-groups: fix restart coroutine iothread race

[Stefan Hajnoczi]

The following QMP command leads to a crash when iothreads are used:

  { 'execute': 'device_del', 'arguments': {'id': 'data'} }

The backtrace involves the queue restart coroutine where
tgm->throttle_state is a NULL pointer because
throttle_group_unregister_tgm() has already been called:

  (gdb) bt full
  #0  0x00005585a7a3b378 in qemu_mutex_lock_impl (mutex=0xffffffffffffffd0, file=0x5585a7bb3d54 "block/throttle-groups.c", line=412) at util/qemu-thread-posix.c:64
        err = <optimized out>
        __PRETTY_FUNCTION__ = "qemu_mutex_lock_impl"
        __func__ = "qemu_mutex_lock_impl"
  #1  0x00005585a79be074 in throttle_group_restart_queue_entry (opaque=0x5585a9de4eb0) at block/throttle-groups.c:412
        _f = <optimized out>
        data = 0x5585a9de4eb0
        tgm = 0x5585a9079440
        ts = 0x0
        tg = 0xffffffffffffff98
        is_write = false
        empty_queue = 255

This coroutine should not execute in the iothread after the throttle
group member has been unregistered!

The root cause is that the device_del code path schedules the restart
coroutine in the iothread while holding the AioContext lock.  Therefore
the iothread cannot execute the coroutine until after device_del
releases the lock - by this time it's too late.

This patch adds a reference count to ThrottleGroupMember so we can
synchronously wait for restart coroutines to complete.  Once they are
done it is safe to unregister the ThrottleGroupMember.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Message-id: 20190114133257.30299-2-stefanha@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/block/throttle-groups.h | 5 +++++
 block/throttle-groups.c         | 9 +++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index e2fd0513c4..712a8e64b4 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -43,6 +43,11 @@ typedef struct ThrottleGroupMember {
      */
     unsigned int io_limits_disabled;
 
+    /* Number of pending throttle_group_restart_queue_entry() coroutines.
+     * Accessed with atomic operations.
+     */
+    unsigned int restart_pending;
+
     /* The following fields are protected by the ThrottleGroup lock.
      * See the ThrottleGroup documentation for details.
      * throttle_state tells us if I/O limits are configured. */
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 5d8213a443..a5a2037924 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -415,6 +415,9 @@ static void coroutine_fn throttle_group_restart_queue_entry(void *opaque)
     }
 
     g_free(data);
+
+    atomic_dec(&tgm->restart_pending);
+    aio_wait_kick();
 }
 
 static void throttle_group_restart_queue(ThrottleGroupMember *tgm, bool is_write)
@@ -430,6 +433,8 @@ static void throttle_group_restart_queue(ThrottleGroupMember *tgm, bool is_write
      * be no timer pending on this tgm at this point */
     assert(!timer_pending(tgm->throttle_timers.timers[is_write]));
 
+    atomic_inc(&tgm->restart_pending);
+
     co = qemu_coroutine_create(throttle_group_restart_queue_entry, rd);
     aio_co_enter(tgm->aio_context, co);
 }
@@ -538,6 +543,7 @@ void throttle_group_register_tgm(ThrottleGroupMember *tgm,
 
     tgm->throttle_state = ts;
     tgm->aio_context = ctx;
+    atomic_set(&tgm->restart_pending, 0);
 
     qemu_mutex_lock(&tg->lock);
     /* If the ThrottleGroup is new set this ThrottleGroupMember as the token */
@@ -584,6 +590,9 @@ void throttle_group_unregister_tgm(ThrottleGroupMember *tgm)
         return;
     }
 
+    /* Wait for throttle_group_restart_queue_entry() coroutines to finish */
+    AIO_WAIT_WHILE(tgm->aio_context, atomic_read(&tgm->restart_pending) > 0);
+
     qemu_mutex_lock(&tg->lock);
     for (i = 0; i < 2; i++) {
         assert(tgm->pending_reqs[i] == 0);
-- 
2.20.1

[Qemu-devel] [PULL 2/3] iotests: add 238 for throttling tgm unregister iothread segfault

[Stefan Hajnoczi]

Hot-unplug a scsi-hd using an iothread.  The previous patch fixes a
segfault in this scenario.

This patch adds a regression test.

Suggested-by: Alberto Garcia <berto@igalia.com>
Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Message-id: 20190114133257.30299-3-stefanha@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/238     | 47 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/238.out |  6 +++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 54 insertions(+)
 create mode 100755 tests/qemu-iotests/238
 create mode 100644 tests/qemu-iotests/238.out

diff --git a/tests/qemu-iotests/238 b/tests/qemu-iotests/238
new file mode 100755
index 0000000000..f81ee1112f
--- /dev/null
+++ b/tests/qemu-iotests/238
@@ -0,0 +1,47 @@
+#!/usr/bin/env python
+#
+# Regression test for throttle group member unregister segfault with iothread
+#
+# Copyright (c) 2019 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/>.
+#
+
+import sys
+import os
+import iotests
+from iotests import log
+
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts'))
+
+from qemu import QEMUMachine
+
+if iotests.qemu_default_machine == 's390-ccw-virtio':
+    virtio_scsi_device = 'virtio-scsi-ccw'
+else:
+    virtio_scsi_device = 'virtio-scsi-pci'
+
+vm = QEMUMachine(iotests.qemu_prog)
+vm.add_args('-machine', 'accel=kvm')
+vm.launch()
+
+log(vm.qmp('blockdev-add', node_name='hd0', driver='null-co'))
+log(vm.qmp('object-add', qom_type='iothread', id='iothread0'))
+log(vm.qmp('device_add', id='scsi0', driver=virtio_scsi_device, iothread='iothread0'))
+log(vm.qmp('device_add', id='scsi-hd0', driver='scsi-hd', drive='hd0'))
+log(vm.qmp('block_set_io_throttle', id='scsi-hd0', bps=0, bps_rd=0, bps_wr=0,
+           iops=1000, iops_rd=0, iops_wr=0, conv_keys=False))
+log(vm.qmp('device_del', id='scsi-hd0'))
+
+vm.shutdown()
diff --git a/tests/qemu-iotests/238.out b/tests/qemu-iotests/238.out
new file mode 100644
index 0000000000..4de840ba8c
--- /dev/null
+++ b/tests/qemu-iotests/238.out
@@ -0,0 +1,6 @@
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index f6b245917a..0f1c3f9cdf 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -234,3 +234,4 @@
 234 auto quick migration
 235 auto quick
 236 auto quick
+238 auto quick
-- 
2.20.1

[Qemu-devel] [PULL 3/3] qemu-coroutine-sleep: drop CoSleepCB

[Stefan Hajnoczi]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Drop CoSleepCB structure. It's actually unused.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-id: 20190122143113.20331-1-vsementsov@virtuozzo.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/qemu-coroutine-sleep.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c
index afb678fbe5..4bfdd30cbf 100644
--- a/util/qemu-coroutine-sleep.c
+++ b/util/qemu-coroutine-sleep.c
@@ -17,38 +17,31 @@
 #include "qemu/timer.h"
 #include "block/aio.h"
 
-typedef struct CoSleepCB {
-    QEMUTimer *ts;
-    Coroutine *co;
-} CoSleepCB;
-
 static void co_sleep_cb(void *opaque)
 {
-    CoSleepCB *sleep_cb = opaque;
+    Coroutine *co = opaque;
 
     /* Write of schedule protected by barrier write in aio_co_schedule */
-    atomic_set(&sleep_cb->co->scheduled, NULL);
-    aio_co_wake(sleep_cb->co);
+    atomic_set(&co->scheduled, NULL);
+    aio_co_wake(co);
 }
 
 void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
 {
     AioContext *ctx = qemu_get_current_aio_context();
-    CoSleepCB sleep_cb = {
-        .co = qemu_coroutine_self(),
-    };
+    QEMUTimer *ts;
+    Coroutine *co = qemu_coroutine_self();
 
-    const char *scheduled = atomic_cmpxchg(&sleep_cb.co->scheduled, NULL,
-                                           __func__);
+    const char *scheduled = atomic_cmpxchg(&co->scheduled, NULL, __func__);
     if (scheduled) {
         fprintf(stderr,
                 "%s: Co-routine was already scheduled in '%s'\n",
                 __func__, scheduled);
         abort();
     }
-    sleep_cb.ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, &sleep_cb);
-    timer_mod(sleep_cb.ts, qemu_clock_get_ns(type) + ns);
+    ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, co);
+    timer_mod(ts, qemu_clock_get_ns(type) + ns);
     qemu_coroutine_yield();
-    timer_del(sleep_cb.ts);
-    timer_free(sleep_cb.ts);
+    timer_del(ts);
+    timer_free(ts);
 }
-- 
2.20.1

Re: [Qemu-devel] [PULL 0/3] Block patches

[Peter Maydell]

On Thu, 24 Jan 2019 at 10:29, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> The following changes since commit f6b06fcceef465de0cf2514c9f76fe0192896781:
>
>   Merge remote-tracking branch 'remotes/kraxel/tags/ui-20190121-pull-request' into staging (2019-01-23 17:57:47 +0000)
>
> are available in the Git repository at:
>
>   git://github.com/stefanha/qemu.git tags/block-pull-request
>
> for you to fetch changes up to 8595685986152334b1ec28c78cb0e5e855d56b54:
>
>   qemu-coroutine-sleep: drop CoSleepCB (2019-01-24 10:05:16 +0000)
>
> ----------------------------------------------------------------
> Pull request
>
> Changelog: No user-visible changes.
>
> ----------------------------------------------------------------

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.0
for any user-visible changes.

-- PMM