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)

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)

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

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

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

--- a/include/block/throttle-groups.h

+++ b/include/block/throttle-groups.h

@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644

--- a/block/throttle-groups.c

+++ b/block/throttle-groups.c

@@ -XXX,XX +XXX,XX @@ 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)

@@ -XXX,XX +XXX,XX @@ 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);

}

@@ -XXX,XX +XXX,XX @@ 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 */

@@ -XXX,XX +XXX,XX @@ void throttle_group_unregister_tgm(ThrottleGroupMember *tgm)

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

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

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

--- a/util/qemu-coroutine-sleep.c

+++ b/util/qemu-coroutine-sleep.c

#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

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

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 XXXXXXX..XXXXXXX

--- /dev/null

+++ b/tests/qemu-iotests/238

@@ -XXX,XX +XXX,XX @@

+#!/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 XXXXXXX..XXXXXXX

--- /dev/null

+++ b/tests/qemu-iotests/238.out

@@ -XXX,XX +XXX,XX @@

+{"return": {}}

+{"return": {}}

+{"return": {}}

+{"return": {}}

+{"return": {}}

+{"return": {}}

diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group

--- a/tests/qemu-iotests/group

+++ b/tests/qemu-iotests/group

@@ -XXX,XX +XXX,XX @@

234 auto quick migration

235 auto quick

236 auto quick

+238 auto quick

2.20.1