* [PULL 1/2] throttle-group: Fix race condition in throttle_group_restart_queue()
2026-03-17 13:45 [PULL 0/2] Block layer patches Hanna Czenczek
@ 2026-03-17 13:45 ` Hanna Czenczek
2026-03-17 13:45 ` [PULL 2/2] block/mirror: fix assertion failure upon duplicate complete for job using 'replaces' Hanna Czenczek
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Hanna Czenczek @ 2026-03-17 13:45 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Hanna Czenczek, Kevin Wolf
From: Alberto Garcia <berto@igalia.com>
When a timer is fired a pending I/O request is restarted and
tg->any_timer_armed is reset so other requests can be scheduled.
However we're resetting any_timer_armed first in timer_cb() before
the request is actually restarted, and there's a window between both
moments in which another thread can arm the same timer, hitting an
assertion in throttle_group_restart_queue().
This can be solved by deferring the reset of tg->any_timer_armed to
the moment when the queue is actually restarted, which is protected by
tg->lock, preventing other threads from arming the timer before that.
In addition to that, throttle_group_restart_tgm() is also updated to
hold tg->lock while the timer is being inspected. Here we consider
three different scenarios:
- If the tgm has a timer set, fire it immediately
- If another tgm has a timer set, restart the queue anyway
- If there is no timer set in this group then simulate a timer that
fires immediately, by setting tg->any_timer_armed in order to
prevent other threads from arming a timer in the meantime.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3194
Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-Id: <825598ef34ad384d936da19d634eda75598508f7.1773316842.git.berto@igalia.com>
Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
---
block/throttle-groups.c | 79 +++++++++++++++++++++++++++++++----------
1 file changed, 60 insertions(+), 19 deletions(-)
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 5329ff1fdb..4b1b1944c2 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -391,6 +391,7 @@ void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm
typedef struct {
ThrottleGroupMember *tgm;
ThrottleDirection direction;
+ bool reset_timer_armed;
} RestartData;
static void coroutine_fn throttle_group_restart_queue_entry(void *opaque)
@@ -403,6 +404,9 @@ static void coroutine_fn throttle_group_restart_queue_entry(void *opaque)
bool empty_queue;
qemu_mutex_lock(&tg->lock);
+ if (data->reset_timer_armed) {
+ tg->any_timer_armed[direction] = false;
+ }
empty_queue = !throttle_group_co_restart_queue(tgm, direction);
/* If the request queue was empty then we have to take care of
@@ -419,18 +423,23 @@ static void coroutine_fn throttle_group_restart_queue_entry(void *opaque)
}
static void throttle_group_restart_queue(ThrottleGroupMember *tgm,
- ThrottleDirection direction)
+ ThrottleDirection direction,
+ bool reset_timer_armed)
{
Coroutine *co;
RestartData *rd = g_new0(RestartData, 1);
rd->tgm = tgm;
rd->direction = direction;
+ rd->reset_timer_armed = reset_timer_armed;
- /* This function is called when a timer is fired or when
- * throttle_group_restart_tgm() is called. Either way, there can
+ /* If reset_timer_armed is set then this means that this function
+ * was called when a timer was fired (either from timer_cb() or
+ * from throttle_group_restart_tgm()). In this case there can
* be no timer pending on this tgm at this point */
- assert(!timer_pending(tgm->throttle_timers.timers[direction]));
+ if (reset_timer_armed) {
+ assert(!timer_pending(tgm->throttle_timers.timers[direction]));
+ }
qatomic_inc(&tgm->restart_pending);
@@ -444,15 +453,50 @@ void throttle_group_restart_tgm(ThrottleGroupMember *tgm)
if (tgm->throttle_state) {
for (dir = THROTTLE_READ; dir < THROTTLE_MAX; dir++) {
- QEMUTimer *t = tgm->throttle_timers.timers[dir];
+ QEMUTimer *t;
+ ThrottleState *ts = tgm->throttle_state;
+ ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
+ bool reset_timer_armed;
+
+ /*
+ * This function restarts the tgm's queue immediately.
+ * This is used for example for callers to drain all requests.
+ * There are three different scenarios depending on whether
+ * a timer is armed for this tg and which tgm owns the timer.
+ */
+
+ qemu_mutex_lock(&tg->lock);
+
+ t = tgm->throttle_timers.timers[dir];
if (timer_pending(t)) {
- /* If there's a pending timer on this tgm, fire it now */
+ /*
+ * Case 1: this tgm has a pending timer.
+ * We can fire the timer immediately.
+ */
timer_del(t);
- timer_cb(tgm, dir);
+ reset_timer_armed = true;
+ } else if (tg->any_timer_armed[dir]) {
+ /*
+ * Case 2: another tgm has a pending timer.
+ * In this case we can still restart the queue but we
+ * have to leave any_timer_armed untouched so the
+ * other tgm's timer is not disrupted.
+ */
+ reset_timer_armed = false;
} else {
- /* Else run the next request from the queue manually */
- throttle_group_restart_queue(tgm, dir);
+ /*
+ * Case 3: there is no timer set for this group.
+ * Here we can simulate a timer that fires immediately,
+ * so the queue is restarted but no other thread
+ * can arm a timer in the meantime.
+ */
+ tg->any_timer_armed[dir] = true;
+ reset_timer_armed = true;
}
+
+ qemu_mutex_unlock(&tg->lock);
+
+ throttle_group_restart_queue(tgm, dir, reset_timer_armed);
}
}
}
@@ -499,16 +543,13 @@ void throttle_group_get_config(ThrottleGroupMember *tgm, ThrottleConfig *cfg)
*/
static void timer_cb(ThrottleGroupMember *tgm, ThrottleDirection direction)
{
- ThrottleState *ts = tgm->throttle_state;
- ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
-
- /* The timer has just been fired, so we can update the flag */
- qemu_mutex_lock(&tg->lock);
- tg->any_timer_armed[direction] = false;
- qemu_mutex_unlock(&tg->lock);
-
- /* Run the request that was waiting for this timer */
- throttle_group_restart_queue(tgm, direction);
+ /*
+ * Run the request that was waiting for this timer.
+ * tg->any_timer_armed needs to be cleared, but we'll do it later
+ * when the queue is restarted in order to prevent another thread
+ * from arming the timer before that.
+ */
+ throttle_group_restart_queue(tgm, direction, true);
}
static void read_timer_cb(void *opaque)
--
2.53.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PULL 2/2] block/mirror: fix assertion failure upon duplicate complete for job using 'replaces'
2026-03-17 13:45 [PULL 0/2] Block layer patches Hanna Czenczek
2026-03-17 13:45 ` [PULL 1/2] throttle-group: Fix race condition in throttle_group_restart_queue() Hanna Czenczek
@ 2026-03-17 13:45 ` Hanna Czenczek
2026-03-18 10:12 ` [PULL 0/2] Block layer patches Peter Maydell
2026-03-18 20:35 ` Michael Tokarev
3 siblings, 0 replies; 6+ messages in thread
From: Hanna Czenczek @ 2026-03-17 13:45 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Hanna Czenczek, Kevin Wolf
From: Fiona Ebner <f.ebner@proxmox.com>
If s->replace_blocker was already set by an earlier invocation of
mirror_complete(), then there will be an assertion failure when
error_setg() is called for it a second time. The bdrv_op_block_all()
and bdrv_ref() operations should only be done a single time too.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Message-Id: <20260311145717.668492-2-f.ebner@proxmox.com>
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
---
block/mirror.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index fa1d975eb9..2fcded9e93 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1276,23 +1276,25 @@ static void mirror_complete(Job *job, Error **errp)
return;
}
- /* block all operations on to_replace bs */
- if (s->replaces) {
- s->to_replace = bdrv_find_node(s->replaces);
- if (!s->to_replace) {
- error_setg(errp, "Node name '%s' not found", s->replaces);
- return;
+ if (!s->should_complete) {
+ /* block all operations on to_replace bs */
+ if (s->replaces) {
+ s->to_replace = bdrv_find_node(s->replaces);
+ if (!s->to_replace) {
+ error_setg(errp, "Node name '%s' not found", s->replaces);
+ return;
+ }
+
+ /* TODO Translate this into child freeze system. */
+ error_setg(&s->replace_blocker,
+ "block device is in use by block-job-complete");
+ bdrv_op_block_all(s->to_replace, s->replace_blocker);
+ bdrv_ref(s->to_replace);
}
- /* TODO Translate this into child freeze system. */
- error_setg(&s->replace_blocker,
- "block device is in use by block-job-complete");
- bdrv_op_block_all(s->to_replace, s->replace_blocker);
- bdrv_ref(s->to_replace);
+ s->should_complete = true;
}
- s->should_complete = true;
-
/* If the job is paused, it will be re-entered when it is resumed */
WITH_JOB_LOCK_GUARD() {
if (!job->paused) {
--
2.53.0
^ permalink raw reply related [flat|nested] 6+ messages in thread