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)

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

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.

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

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

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

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

Drop CoSleepCB structure. It's actually unused.

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

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

#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