Hi! Could, someone, please, comment? The issue in gitlab is still open. On Mon, Dec 8, 2025 at 11:55 AM Dmitry Guryanov <[email protected]> wrote:
> Details: https://gitlab.com/qemu-project/qemu/-/issues/3144 > > The function schedule_next_request is called with tg->lock held and > it may call throttle_group_co_restart_queue, which takes > tgm->throttled_reqs_lock, qemu_co_mutex_lock may leave current > coroutine if other iothread has taken the lock. If the next > coroutine will call throttle_group_co_io_limits_intercept - it > will try to take the mutex tg->lock which will never be released. > > Here is the backtrace of the iothread: > Thread 30 (Thread 0x7f8aad1fd6c0 (LWP 24240) "IO iothread2"): > #0 futex_wait (futex_word=0x5611adb7d828, expected=2, private=0) at > ../sysdeps/nptl/futex-internal.h:146 > #1 __GI___lll_lock_wait (futex=futex@entry=0x5611adb7d828, private=0) > at lowlevellock.c:49 > #2 0x00007f8ab5a97501 in lll_mutex_lock_optimized (mutex=0x5611adb7d828) > at pthread_mutex_lock.c:48 > #3 ___pthread_mutex_lock (mutex=0x5611adb7d828) at > pthread_mutex_lock.c:93 > #4 0x00005611823f5482 in qemu_mutex_lock_impl (mutex=0x5611adb7d828, > file=0x56118289daca "../block/throttle-groups.c", line=372) at > ../util/qemu-thread-posix.c:94 > #5 0x00005611822b0b39 in throttle_group_co_io_limits_intercept > (tgm=0x5611af1bb4d8, bytes=4096, direction=THROTTLE_READ) at > ../block/throttle-groups.c:372 > #6 0x00005611822473b1 in blk_co_do_preadv_part (blk=0x5611af1bb490, > offset=15972311040, bytes=4096, qiov=0x7f8aa4000f98, qiov_offset=0, > flags=BDRV_REQ_REGISTERED_BUF) at ../block/block-backend.c:1354 > #7 0x0000561182247fa0 in blk_aio_read_entry (opaque=0x7f8aa4005910) at > ../block/block-backend.c:1619 > #8 0x000056118241952e in coroutine_trampoline (i0=-1543497424, i1=32650) > at ../util/coroutine-ucontext.c:175 > #9 0x00007f8ab5a56f70 in ?? () at > ../sysdeps/unix/sysv/linux/x86_64/__start_context.S:66 from > target:/lib64/libc.so.6 > #10 0x00007f8aad1ef190 in ?? () > #11 0x0000000000000000 in ?? () > > The lock is taken in line 386: > (gdb) p tg.lock > $1 = {lock = {__data = {__lock = 2, __count = 0, __owner = 24240, __nusers > = 1, __kind = 0, __spins = 0, __elision = 0, __list = {__prev = 0x0, __next > = 0x0}}, > __size = "\002\000\000\000\000\000\000\000\260^\000\000\001", '\000' > <repeats 26 times>, __align = 2}, file = 0x56118289daca > "../block/throttle-groups.c", > line = 386, initialized = true} > > The solution is to use tg->lock to protect both ThreadGroup fields and > ThrottleGroupMember.throttled_reqs. It doesn't seem to be possible > to use separate locks because we need to first manipulate ThrottleGroup > fields, then schedule next coroutine using throttled_reqs and after than > update token field from ThrottleGroup depending on the throttled_reqs > state. > > Signed-off-by: Dmitry Guryanov <[email protected]> > --- > block/throttle-groups.c | 21 ++++++--------------- > include/block/throttle-groups.h | 1 - > 2 files changed, 6 insertions(+), 16 deletions(-) > > diff --git a/block/throttle-groups.c b/block/throttle-groups.c > index 66fdce9a90..bb01e52516 100644 > --- a/block/throttle-groups.c > +++ b/block/throttle-groups.c > @@ -295,19 +295,15 @@ static bool > throttle_group_schedule_timer(ThrottleGroupMember *tgm, > /* Start the next pending I/O request for a ThrottleGroupMember. Return > whether > * any request was actually pending. > * > + * This assumes that tg->lock is held. > + * > * @tgm: the current ThrottleGroupMember > * @direction: the ThrottleDirection > */ > static bool coroutine_fn > throttle_group_co_restart_queue(ThrottleGroupMember *tgm, > > ThrottleDirection direction) > { > - bool ret; > - > - qemu_co_mutex_lock(&tgm->throttled_reqs_lock); > - ret = qemu_co_queue_next(&tgm->throttled_reqs[direction]); > - qemu_co_mutex_unlock(&tgm->throttled_reqs_lock); > - > - return ret; > + return qemu_co_queue_next(&tgm->throttled_reqs[direction]);; > } > > /* Look for the next pending I/O request and schedule it. > @@ -378,12 +374,8 @@ void coroutine_fn > throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm > /* Wait if there's a timer set or queued requests of this type */ > if (must_wait || tgm->pending_reqs[direction]) { > tgm->pending_reqs[direction]++; > - qemu_mutex_unlock(&tg->lock); > - qemu_co_mutex_lock(&tgm->throttled_reqs_lock); > qemu_co_queue_wait(&tgm->throttled_reqs[direction], > - &tgm->throttled_reqs_lock); > - qemu_co_mutex_unlock(&tgm->throttled_reqs_lock); > - qemu_mutex_lock(&tg->lock); > + &tg->lock); > tgm->pending_reqs[direction]--; > } > > @@ -410,15 +402,15 @@ static void coroutine_fn > throttle_group_restart_queue_entry(void *opaque) > ThrottleDirection direction = data->direction; > bool empty_queue; > > + qemu_mutex_lock(&tg->lock); > empty_queue = !throttle_group_co_restart_queue(tgm, direction); > > /* If the request queue was empty then we have to take care of > * scheduling the next one */ > if (empty_queue) { > - qemu_mutex_lock(&tg->lock); > schedule_next_request(tgm, direction); > - qemu_mutex_unlock(&tg->lock); > } > + qemu_mutex_unlock(&tg->lock); > > g_free(data); > > @@ -569,7 +561,6 @@ void throttle_group_register_tgm(ThrottleGroupMember > *tgm, > read_timer_cb, > write_timer_cb, > tgm); > - qemu_co_mutex_init(&tgm->throttled_reqs_lock); > } > > /* Unregister a ThrottleGroupMember from its group, removing it from the > list, > diff --git a/include/block/throttle-groups.h > b/include/block/throttle-groups.h > index 2355e8d9de..374c7c0215 100644 > --- a/include/block/throttle-groups.h > +++ b/include/block/throttle-groups.h > @@ -36,7 +36,6 @@ > typedef struct ThrottleGroupMember { > AioContext *aio_context; > /* throttled_reqs_lock protects the CoQueues for throttled requests. > */ > - CoMutex throttled_reqs_lock; > CoQueue throttled_reqs[THROTTLE_MAX]; > > /* Nonzero if the I/O limits are currently being ignored; generally > -- > 2.51.1 > > -- Dmitry Guryanov
