Changes since v1 ---------------- v1 was a two-patch series tracking two independent missed-wakeup races on the qemu shutdown path. * Patch 1, "block/graph-lock: fix missed wakeup in bdrv_graph_co_rdunlock()", was applied as e3082ab3b3 by Kevin and is now in tree. This v2 carries only the remaining race. * Per Kevin's review of v1 patch 2 [1], the cache_clean_timer hang is no longer worked around inside block/qcow2.c. Instead, the underlying primitive -- qemu_co_sleep_wake() -- is fixed, closing the lost-wakeup window for every caller (cache_clean_timer, block_copy_kick, ...) rather than just qcow2. Cancellation latency through qemu_co_sleep_wake() drops from "next 1 s tick" (v1 workaround) to aio_co_wake(). [1] https://lore.kernel.org/qemu-devel/agcYkmTWud8euTds@redhat.com/ Problem ------- The qemu shutdown / blockdev-close path can deadlock permanently on upstream master. The main thread enters ppoll(timeout=-1) holding BQL, no other thread has a wake source that points back at it, and qemu has to be SIGKILLed. The hang has no timeout -- it is a hard deadlock, not a slow operation; behind BQL, RCU, VCPUs and every iothread path that needs BQL stall with it. Two independent missed-wakeup races in the block layer contributed to the symptom on v1. Both shared the same shape: a waiter arms on one side, the waker reads stale state on its fast path and silently skips the kick, and nothing else on the AioContext fires to recover. The first (block/graph-lock) was fixed by e3082ab3b3 and is now in tree. This patch closes the second one, exposed in qcow2's cache_clean_timer cancellation path: ppoll -> aio_poll -> cache_clean_timer_del_and_wait -> qcow2_close The race diagram and the exact stale-state read are in the patch's commit message. Reproducer ---------- Environment: 4-vCPU VM guest, kernel 6.12.x, upstream master at e3082ab3b3 (with the graph-lock fix already applied). On modern bare-metal the window is narrow enough that the hang rarely reproduces without a VM -- a VM guest under full CPU saturation is what makes the timing reliable. # reproducer stress-ng --cpu "$(nproc)" --timeout 0 & for r in $(seq 20); do timeout 120 ./build/tests/qemu-iotests/check -qcow2 iothreads-create done kill %1 With `stress-ng --cpu $(nproc)` the race surfaces. With `stress-ng --cpu $(($(nproc) - 1))` or without a stressor it does not reproduce reliably across 20 iterations. When the race fires, the Python QMP client times out on vm.run_job() after 5 s, the qemu process keeps running but never makes forward progress, and the outer `timeout 120` eventually kills it. Attach gdb before the timeout kills qemu to capture the stack. Results ------- Same guest, 20 iterations of the loop above, master at e3082ab3b3: without this patch: reproduces reliably (qcow2_close in ppoll) with this patch: 20/20 PASS Signed-off-by: Denis V. Lunev <den@openvz.org> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Hanna Reitz <hreitz@redhat.com> Denis V. Lunev (1): coroutine: fix lost wakeup in qemu_co_sleep_wake() include/qemu/coroutine.h | 17 ++++++++--- util/qemu-coroutine-sleep.c | 60 +++++++++++++++++++++++++++---------- 2 files changed, 58 insertions(+), 19 deletions(-) -- 2.51.0 cache_clean_timer_del_and_wait() cancels the cache-cleaner coroutine by setting s->cache_clean_interval = 0 and calling qemu_co_sleep_wake() to cut short its qemu_co_sleep_ns_wakeable(). qemu_co_sleep_wake() is fire-and-forget: it reads w->to_wake and silently returns when it is NULL. A sleeper that is between two iterations -- has just released s->lock but has not yet set w->to_wake inside qemu_co_sleep() -- loses the wake: iothread0 timer coroutine main thread (qcow2 close) ------------------------- ------------------------- while-body (holding s->lock): read interval = 600 wait_ns = 600 * NS release s->lock take s->lock interval = 0 qemu_co_sleep_wake(w): w->to_wake == NULL -> skip return qemu_co_queue_wait(exit, s->lock): release s->lock yield qemu_co_sleep_ns_wakeable: aio_timer_init(+600 s) qemu_co_sleep: cas scheduled NULL -> "qsns" w->to_wake = co yield [sleeps 600 s] cache_clean_timer_del_and_wait() then blocks on cache_clean_timer_exit until the original 600 s expiry fires, and qcow2_close() holds BQL the whole time so the VM stalls behind it. block_copy_kick() has the same shape. Fix the primitive once instead of working around it in each caller. Use a tri-state for QemuCoSleep::to_wake: NULL - idle co - sleeper parked PENDING - wake delivered, no sleeper yet (sticky) qemu_co_sleep_wake() xchgs PENDING into to_wake: a real sleeper is woken, NULL/PENDING is left untouched so the wake stays sticky. qemu_co_sleep() consumes a sticky PENDING on entry; otherwise it cmpxchg-publishes itself, and a wake racing in between is caught because the cmpxchg observes PENDING and returns without yielding. On normal resume qemu_co_sleep() clears the PENDING the waker left behind so the next sleep starts clean. A double-fire (real wake plus timer callback) is harmless: the first xchg returns the coroutine and wakes it; the second returns PENDING and is a no-op. Cancellation latency through qemu_co_sleep_wake() is now bounded by aio_co_wake() rather than by the sleep duration. Fixes: f86dde9a15 ("qcow2: Fix cache_clean_timer") Signed-off-by: Denis V. Lunev <den@openvz.org> Cc: Hanna Czenczek <hreitz@redhat.com> Cc: Kevin Wolf <kwolf@redhat.com> --- include/qemu/coroutine.h | 17 ++++++++--- util/qemu-coroutine-sleep.c | 60 +++++++++++++++++++++++++++---------- 2 files changed, 58 insertions(+), 19 deletions(-) diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h index XXXXXXX..XXXXXXX 100644 --- a/include/qemu/coroutine.h +++ b/include/qemu/coroutine.h @@ -XXX,XX +XXX,XX @@ int coroutine_fn qemu_co_timeout(CoroutineEntry *entry, void *opaque, uint64_t timeout_ns, CleanupFunc clean); /** - * Wake a coroutine if it is sleeping in qemu_co_sleep_ns. The timer will be - * deleted. @sleep_state must be the variable whose address was given to - * qemu_co_sleep_ns() and should be checked to be non-NULL before calling - * qemu_co_sleep_wake(). + * Wake a coroutine sleeping in qemu_co_sleep() or qemu_co_sleep_ns_wakeable(). + * The timer set up by the latter is deleted on wakeup. + * + * The wake is sticky: if no sleeper is parked on @w at the time of the call, + * the wake is recorded on @w and consumed by the next qemu_co_sleep() on the + * same @w, which then returns without yielding. This closes the lost-wakeup + * window between two sleeps and is the documented behavior callers should + * rely on -- e.g. a cancellation signal raised between iterations of a + * sleep/work loop will shorten the next sleep instead of being dropped. + * + * The state persists until consumed: if no further qemu_co_sleep() is ever + * called on @w, the pending wake is harmlessly discarded when @w goes away. + * Multiple wakes coalesce -- the next sleep consumes at most one. */ void qemu_co_sleep_wake(QemuCoSleep *w); diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c index XXXXXXX..XXXXXXX 100644 --- a/util/qemu-coroutine-sleep.c +++ b/util/qemu-coroutine-sleep.c @@ -XXX,XX +XXX,XX @@ static const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns"; +/* + * Sentinel stored in QemuCoSleep::to_wake by qemu_co_sleep_wake() when no + * sleeper has parked yet. The next qemu_co_sleep() consumes it and returns + * without yielding, so a wake that races the arming of a sleep is never + * lost. + */ +#define QEMU_CO_SLEEP_PENDING ((Coroutine *)(uintptr_t)1) + void qemu_co_sleep_wake(QemuCoSleep *w) { Coroutine *co; - co = w->to_wake; - w->to_wake = NULL; - if (co) { - /* Write of schedule protected by barrier write in aio_co_schedule */ - const char *scheduled = qatomic_cmpxchg(&co->scheduled, - qemu_co_sleep_ns__scheduled, NULL); - - assert(scheduled == qemu_co_sleep_ns__scheduled); - aio_co_wake(co); + co = qatomic_xchg(&w->to_wake, QEMU_CO_SLEEP_PENDING); + if (co == NULL || co == QEMU_CO_SLEEP_PENDING) { + /* No sleeper, or a wake is already pending. */ + return; } + + /* Write of scheduled protected by barrier write in aio_co_schedule */ + const char *scheduled = qatomic_cmpxchg(&co->scheduled, + qemu_co_sleep_ns__scheduled, NULL); + assert(scheduled == qemu_co_sleep_ns__scheduled); + aio_co_wake(co); } static void co_sleep_cb(void *opaque) @@ -XXX,XX +XXX,XX @@ static void co_sleep_cb(void *opaque) void coroutine_fn qemu_co_sleep(QemuCoSleep *w) { Coroutine *co = qemu_coroutine_self(); + Coroutine *prev; + + /* Consume an already-delivered wake without yielding. */ + prev = qatomic_cmpxchg(&w->to_wake, QEMU_CO_SLEEP_PENDING, NULL); + if (prev == QEMU_CO_SLEEP_PENDING) { + return; + } + assert(prev == NULL); const char *scheduled = qatomic_cmpxchg(&co->scheduled, NULL, qemu_co_sleep_ns__scheduled); @@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep(QemuCoSleep *w) abort(); } - w->to_wake = co; + /* + * Publish ourselves as the sleeper. If a wake raced in between the + * fast path above and now, the cmpxchg observes QEMU_CO_SLEEP_PENDING + * and we consume it without yielding. + */ + prev = qatomic_cmpxchg(&w->to_wake, NULL, co); + if (prev == QEMU_CO_SLEEP_PENDING) { + qatomic_set(&w->to_wake, NULL); + qatomic_set(&co->scheduled, NULL); + return; + } + assert(prev == NULL); + qemu_coroutine_yield(); - /* w->to_wake is cleared before resuming this coroutine. */ - assert(w->to_wake == NULL); + /* The waker left QEMU_CO_SLEEP_PENDING; clear it for the next sleep. */ + qatomic_set(&w->to_wake, NULL); } void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w, @@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w, timer_mod(&ts, qemu_clock_get_ns(type) + ns); /* - * The timer will fire in the current AiOContext, so the callback - * must happen after qemu_co_sleep yields and there is no race - * between timer_mod and qemu_co_sleep. + * A wake racing with the arming of the sleep -- including the timer + * we just armed firing in another AioContext before qemu_co_sleep() + * publishes itself -- is captured by the sticky PENDING state in + * qemu_co_sleep_wake() and consumed here without yielding. */ qemu_co_sleep(w); timer_del(&ts); -- 2.51.0 Changes since v2 ---------------- * Drop the redundant fast-path check in qemu_co_sleep(); the publish cmpxchg already consumes a pending wake (Kevin). * Add a deterministic regression test, no threads or timers needed. Changes since v1 ---------------- * Patch 1 (graph-lock) applied as e3082ab3b3, dropped here. * Fix the qemu_co_sleep_wake() primitive instead of working around it in qcow2 (Kevin). Problem ------- The qemu shutdown / blockdev-close path can deadlock permanently on upstream master. The main thread enters ppoll(timeout=-1) holding BQL, no other thread has a wake source that points back at it, and qemu has to be SIGKILLed. The hang has no timeout -- it is a hard deadlock, not a slow operation; behind BQL, RCU, VCPUs and every iothread path that needs BQL stall with it. The race exposed in qcow2's cache_clean_timer cancellation path: ppoll -> aio_poll -> cache_clean_timer_del_and_wait -> qcow2_close The race diagram and the exact stale-state read are in the patch's commit message. Reproducer ---------- Environment: 4-vCPU VM guest, kernel 6.12.x, upstream master at de5d8bfd61. On modern bare-metal the window is narrow enough that the hang rarely reproduces without a VM -- a VM guest under full CPU saturation is what makes the timing reliable. # reproducer stress-ng --cpu "$(nproc)" --timeout 0 & for r in $(seq 20); do timeout 120 ./build/tests/qemu-iotests/check -qcow2 iothreads-create done kill %1 With `stress-ng --cpu $(nproc)` the race surfaces. With `stress-ng --cpu $(($(nproc) - 1))` or without a stressor it does not reproduce reliably across 20 iterations. The new unit test reproduces the same lost wakeup deterministically, without a VM or a stressor. Results ------- Same guest, 20 iterations of the loop above, master at de5d8bfd61: without this patch: reproduces reliably (qcow2_close in ppoll) with this patch: 20/20 PASS Signed-off-by: Denis V. Lunev <den@openvz.org> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Hanna Reitz <hreitz@redhat.com> Denis V. Lunev (1): coroutine: fix lost wakeup in qemu_co_sleep_wake() include/qemu/coroutine.h | 17 +++++++++--- tests/unit/test-coroutine.c | 53 +++++++++++++++++++++++++++++++++++++ util/qemu-coroutine-sleep.c | 53 ++++++++++++++++++++++++++----------- 3 files changed, 104 insertions(+), 19 deletions(-) -- 2.53.0 cache_clean_timer_del_and_wait() cancels the cache-cleaner coroutine by setting s->cache_clean_interval = 0 and calling qemu_co_sleep_wake() to cut short its qemu_co_sleep_ns_wakeable(). qemu_co_sleep_wake() is fire-and-forget: it reads w->to_wake and silently returns when it is NULL. A sleeper that is between two iterations -- has just released s->lock but has not yet set w->to_wake inside qemu_co_sleep() -- loses the wake: iothread0 timer coroutine main thread (qcow2 close) ------------------------- ------------------------- while-body (holding s->lock): read interval = 600 wait_ns = 600 * NS release s->lock take s->lock interval = 0 qemu_co_sleep_wake(w): w->to_wake == NULL -> skip return qemu_co_queue_wait(exit, s->lock): release s->lock yield qemu_co_sleep_ns_wakeable: aio_timer_init(+600 s) qemu_co_sleep: cas scheduled NULL -> "qsns" w->to_wake = co yield [sleeps 600 s] cache_clean_timer_del_and_wait() then blocks on cache_clean_timer_exit until the original 600 s expiry fires, and qcow2_close() holds BQL the whole time so the VM stalls behind it. block_copy_kick() has the same shape. Fix the primitive once instead of working around it in each caller. Use a tri-state for QemuCoSleep::to_wake: NULL - idle co - sleeper parked PENDING - wake delivered, no sleeper yet (sticky) qemu_co_sleep_wake() xchgs PENDING into to_wake: a real sleeper is woken, NULL/PENDING is left untouched so the wake stays sticky. qemu_co_sleep() cmpxchg-publishes itself as the sleeper; if a wake was delivered before it got there or races the publish, the cmpxchg observes PENDING and returns without yielding. On normal resume qemu_co_sleep() clears the PENDING the waker left behind so the next sleep starts clean. A double-fire (real wake plus timer callback) is harmless: the first xchg returns the coroutine and wakes it; the second returns PENDING and is a no-op. Cancellation latency through qemu_co_sleep_wake() is now bounded by aio_co_wake() rather than by the sleep duration. Fixes: f86dde9a15 ("qcow2: Fix cache_clean_timer") Signed-off-by: Denis V. Lunev <den@openvz.org> Cc: Hanna Czenczek <hreitz@redhat.com> Cc: Kevin Wolf <kwolf@redhat.com> --- include/qemu/coroutine.h | 17 +++++++++--- tests/unit/test-coroutine.c | 53 +++++++++++++++++++++++++++++++++++++ util/qemu-coroutine-sleep.c | 53 ++++++++++++++++++++++++++----------- 3 files changed, 104 insertions(+), 19 deletions(-) diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h index XXXXXXX..XXXXXXX 100644 --- a/include/qemu/coroutine.h +++ b/include/qemu/coroutine.h @@ -XXX,XX +XXX,XX @@ int coroutine_fn qemu_co_timeout(CoroutineEntry *entry, void *opaque, uint64_t timeout_ns, CleanupFunc clean); /** - * Wake a coroutine if it is sleeping in qemu_co_sleep_ns. The timer will be - * deleted. @sleep_state must be the variable whose address was given to - * qemu_co_sleep_ns() and should be checked to be non-NULL before calling - * qemu_co_sleep_wake(). + * Wake a coroutine sleeping in qemu_co_sleep() or qemu_co_sleep_ns_wakeable(). + * The timer set up by the latter is deleted on wakeup. + * + * The wake is sticky: if no sleeper is parked on @w at the time of the call, + * the wake is recorded on @w and consumed by the next qemu_co_sleep() on the + * same @w, which then returns without yielding. This closes the lost-wakeup + * window between two sleeps and is the documented behavior callers should + * rely on -- e.g. a cancellation signal raised between iterations of a + * sleep/work loop will shorten the next sleep instead of being dropped. + * + * The state persists until consumed: if no further qemu_co_sleep() is ever + * called on @w, the pending wake is harmlessly discarded when @w goes away. + * Multiple wakes coalesce -- the next sleep consumes at most one. */ void qemu_co_sleep_wake(QemuCoSleep *w); diff --git a/tests/unit/test-coroutine.c b/tests/unit/test-coroutine.c index XXXXXXX..XXXXXXX 100644 --- a/tests/unit/test-coroutine.c +++ b/tests/unit/test-coroutine.c @@ -XXX,XX +XXX,XX @@ static void test_co_rwlock_downgrade(void) g_assert(c1_done); } +/* + * Check that a wake delivered before the sleeper parks is not lost. + * + * qemu_co_sleep_wake() is fire-and-forget: a caller cancelling a + * sleep/work loop may call it in the window after the sleeper has + * decided to sleep but before it has published itself inside + * qemu_co_sleep(). The wake must be sticky and shorten the next sleep + * rather than being dropped (which would block until the full sleep + * duration expired). + * + * No threads, timers or AioContext are needed: coroutines are + * cooperative, so ordering the wake before the sleep deterministically + * reproduces the state the racing waker would otherwise produce. + */ + +typedef struct { + QemuCoSleep w; + bool completed; +} CoSleepWakeData; + +static void coroutine_fn co_sleep_wake_entry(void *opaque) +{ + CoSleepWakeData *d = opaque; + + /* + * The wake was already delivered before we got here. qemu_co_sleep() + * must consume it and return without yielding. + */ + qemu_co_sleep(&d->w); + d->completed = true; +} + +static void test_co_sleep_wake_before_sleep(void) +{ + CoSleepWakeData d = { .w = { 0 }, .completed = false }; + Coroutine *co = qemu_coroutine_create(co_sleep_wake_entry, &d); + + /* Waker runs first, while no sleeper is parked on w. */ + qemu_co_sleep_wake(&d.w); + + /* + * Entering runs qemu_co_sleep(), which consumes the pending wake and + * returns without yielding, so the coroutine runs straight to + * completion in this single enter. With the pre-fix primitive the wake + * is dropped, qemu_co_sleep() parks, and completed stays false. + */ + qemu_coroutine_enter(co); + + g_assert(d.completed); +} + /* * Check that creation, enter, and return work */ @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv) g_test_add_func("/locking/co-mutex/lockable", test_co_mutex_lockable); g_test_add_func("/locking/co-rwlock/upgrade", test_co_rwlock_upgrade); g_test_add_func("/locking/co-rwlock/downgrade", test_co_rwlock_downgrade); + g_test_add_func("/locking/co-sleep/wake-before-sleep", + test_co_sleep_wake_before_sleep); if (g_test_perf()) { g_test_add_func("/perf/lifecycle", perf_lifecycle); g_test_add_func("/perf/nesting", perf_nesting); diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c index XXXXXXX..XXXXXXX 100644 --- a/util/qemu-coroutine-sleep.c +++ b/util/qemu-coroutine-sleep.c @@ -XXX,XX +XXX,XX @@ static const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns"; +/* + * Sentinel stored in QemuCoSleep::to_wake by qemu_co_sleep_wake() when no + * sleeper has parked yet. The next qemu_co_sleep() consumes it and returns + * without yielding, so a wake that races the arming of a sleep is never + * lost. + */ +#define QEMU_CO_SLEEP_PENDING ((Coroutine *)(uintptr_t)1) + void qemu_co_sleep_wake(QemuCoSleep *w) { Coroutine *co; - co = w->to_wake; - w->to_wake = NULL; - if (co) { - /* Write of schedule protected by barrier write in aio_co_schedule */ - const char *scheduled = qatomic_cmpxchg(&co->scheduled, - qemu_co_sleep_ns__scheduled, NULL); - - assert(scheduled == qemu_co_sleep_ns__scheduled); - aio_co_wake(co); + co = qatomic_xchg(&w->to_wake, QEMU_CO_SLEEP_PENDING); + if (co == NULL || co == QEMU_CO_SLEEP_PENDING) { + /* No sleeper, or a wake is already pending. */ + return; } + + /* Write of scheduled protected by barrier write in aio_co_schedule */ + const char *scheduled = qatomic_cmpxchg(&co->scheduled, + qemu_co_sleep_ns__scheduled, NULL); + assert(scheduled == qemu_co_sleep_ns__scheduled); + aio_co_wake(co); } static void co_sleep_cb(void *opaque) @@ -XXX,XX +XXX,XX @@ static void co_sleep_cb(void *opaque) void coroutine_fn qemu_co_sleep(QemuCoSleep *w) { Coroutine *co = qemu_coroutine_self(); + Coroutine *prev; const char *scheduled = qatomic_cmpxchg(&co->scheduled, NULL, qemu_co_sleep_ns__scheduled); @@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep(QemuCoSleep *w) abort(); } - w->to_wake = co; + /* + * Publish ourselves as the sleeper. A wake delivered before we got here, + * or one racing this publish, leaves QEMU_CO_SLEEP_PENDING in to_wake; + * the cmpxchg then fails and we consume the wake without yielding. + */ + prev = qatomic_cmpxchg(&w->to_wake, NULL, co); + if (prev == QEMU_CO_SLEEP_PENDING) { + qatomic_set(&w->to_wake, NULL); + qatomic_set(&co->scheduled, NULL); + return; + } + assert(prev == NULL); + qemu_coroutine_yield(); - /* w->to_wake is cleared before resuming this coroutine. */ - assert(w->to_wake == NULL); + /* The waker left QEMU_CO_SLEEP_PENDING; clear it for the next sleep. */ + qatomic_set(&w->to_wake, NULL); } void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w, @@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w, timer_mod(&ts, qemu_clock_get_ns(type) + ns); /* - * The timer will fire in the current AiOContext, so the callback - * must happen after qemu_co_sleep yields and there is no race - * between timer_mod and qemu_co_sleep. + * A wake racing with the arming of the sleep -- including the timer + * we just armed firing in another AioContext before qemu_co_sleep() + * publishes itself -- is captured by the sticky PENDING state in + * qemu_co_sleep_wake() and consumed here without yielding. */ qemu_co_sleep(w); timer_del(&ts); -- 2.53.0