@samitouri / QOSamiQemu / commits / 9c8430f5d6

throttle-group: Fix race condition in throttle_group_restart_queue()

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>

Alberto Garcia committed Mar 12, 2026 at 13:12 UTC 9c8430f5d65144b85ad76433369288182a1c7baa
1 file changed +60 -19
block/throttle-groups.c
+60 -19
@@ -391,6 +391,7 @@ void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm
391 typedef struct {
392 ThrottleGroupMember *tgm;
393 ThrottleDirection direction;
394 + bool reset_timer_armed;
395 } RestartData;
396
397 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)
404 bool empty_queue;
405
406 qemu_mutex_lock(&tg->lock);
407 + if (data->reset_timer_armed) {
408 + tg->any_timer_armed[direction] = false;
409 + }
410 empty_queue = !throttle_group_co_restart_queue(tgm, direction);
411
412 /* 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)
423 }
424
425 static void throttle_group_restart_queue(ThrottleGroupMember *tgm,
422 - ThrottleDirection direction)
426 + ThrottleDirection direction,
427 + bool reset_timer_armed)
428 {
429 Coroutine *co;
430 RestartData *rd = g_new0(RestartData, 1);
431
432 rd->tgm = tgm;
433 rd->direction = direction;
434 + rd->reset_timer_armed = reset_timer_armed;
435
430 - /* This function is called when a timer is fired or when
431 - * throttle_group_restart_tgm() is called. Either way, there can
436 + /* If reset_timer_armed is set then this means that this function
437 + * was called when a timer was fired (either from timer_cb() or
438 + * from throttle_group_restart_tgm()). In this case there can
439 * be no timer pending on this tgm at this point */
433 - assert(!timer_pending(tgm->throttle_timers.timers[direction]));
440 + if (reset_timer_armed) {
441 + assert(!timer_pending(tgm->throttle_timers.timers[direction]));
442 + }
443
444 qatomic_inc(&tgm->restart_pending);
445
@@ -444,15 +453,50 @@ void throttle_group_restart_tgm(ThrottleGroupMember *tgm)
453
454 if (tgm->throttle_state) {
455 for (dir = THROTTLE_READ; dir < THROTTLE_MAX; dir++) {
447 - QEMUTimer *t = tgm->throttle_timers.timers[dir];
456 + QEMUTimer *t;
457 + ThrottleState *ts = tgm->throttle_state;
458 + ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
459 + bool reset_timer_armed;
460 +
461 + /*
462 + * This function restarts the tgm's queue immediately.
463 + * This is used for example for callers to drain all requests.
464 + * There are three different scenarios depending on whether
465 + * a timer is armed for this tg and which tgm owns the timer.
466 + */
467 +
468 + qemu_mutex_lock(&tg->lock);
469 +
470 + t = tgm->throttle_timers.timers[dir];
471 if (timer_pending(t)) {
449 - /* If there's a pending timer on this tgm, fire it now */
472 + /*
473 + * Case 1: this tgm has a pending timer.
474 + * We can fire the timer immediately.
475 + */
476 timer_del(t);
451 - timer_cb(tgm, dir);
477 + reset_timer_armed = true;
478 + } else if (tg->any_timer_armed[dir]) {
479 + /*
480 + * Case 2: another tgm has a pending timer.
481 + * In this case we can still restart the queue but we
482 + * have to leave any_timer_armed untouched so the
483 + * other tgm's timer is not disrupted.
484 + */
485 + reset_timer_armed = false;
486 } else {
453 - /* Else run the next request from the queue manually */
454 - throttle_group_restart_queue(tgm, dir);
487 + /*
488 + * Case 3: there is no timer set for this group.
489 + * Here we can simulate a timer that fires immediately,
490 + * so the queue is restarted but no other thread
491 + * can arm a timer in the meantime.
492 + */
493 + tg->any_timer_armed[dir] = true;
494 + reset_timer_armed = true;
495 }
496 +
497 + qemu_mutex_unlock(&tg->lock);
498 +
499 + throttle_group_restart_queue(tgm, dir, reset_timer_armed);
500 }
501 }
502 }
@@ -499,16 +543,13 @@ void throttle_group_get_config(ThrottleGroupMember *tgm, ThrottleConfig *cfg)
543 */
544 static void timer_cb(ThrottleGroupMember *tgm, ThrottleDirection direction)
545 {
502 - ThrottleState *ts = tgm->throttle_state;
503 - ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
504 -
505 - /* The timer has just been fired, so we can update the flag */
506 - qemu_mutex_lock(&tg->lock);
507 - tg->any_timer_armed[direction] = false;
508 - qemu_mutex_unlock(&tg->lock);
509 -
510 - /* Run the request that was waiting for this timer */
511 - throttle_group_restart_queue(tgm, direction);
546 + /*
547 + * Run the request that was waiting for this timer.
548 + * tg->any_timer_armed needs to be cleared, but we'll do it later
549 + * when the queue is restarted in order to prevent another thread
550 + * from arming the timer before that.
551 + */
552 + throttle_group_restart_queue(tgm, direction, true);
553 }
554
555 static void read_timer_cb(void *opaque)