On 11/10/2016 12:35, Changlong Xie wrote: > For nbd client, if request number is large than MAX_NBD_REQUESTS(16), we > will queue the rest requests into free_sema->queue. > When nbd client receives one reply, it will unlock free_sema, then pop > the free_sema->queue head, so set free_sema->holder as > revelant coroutine.
NBD is using the CoMutex in a way that wasn't anticipated. The simplest fix is to change it to CoQueue, which is like a condition variable. Instead of locking if in_flight >= MAX_NBD_REQUESTS - 1, wait on the queue while in_flight == MAX_NBD_REQUESTS. Instead of unlocking, use qemu_co_queue_next to wake up one request. Thanks for the report! Paolo > For example if there are N(N=26 and MAX_NBD_REQUESTS=16) nbd write > requests, so we'll invoke nbd_client_co_pwritev 26 times. > time request No Actions > 1 1 in_flight=1, Coroutine=C1 > 2 2 in_flight=2, Coroutine=C2 > ... ... > 15 15 in_flight=15, Coroutine=C15 > 16 16 in_flight=16, Coroutine=C16, > free_sema->holder=C16, mutex->locked=true > 17 17 in_flight=16, Coroutine=C17, queue C17 into > free_sema->queue > 18 18 in_flight=16, Coroutine=C18, queue C18 into > free_sema->queue > ... ... > 26 N in_flight=16, Coroutine=C26, queue C26 into > free_sema->queue > > Once nbd client recieves request No.16' reply, we will re-enter request > C16. It's ok, because it's equal to 'free_sema->holder'. > time request No Actions > 27 16 in_flight=15, Coroutine=C16, > free_sema->holder=C16, mutex->locked=false > > Then nbd_coroutine_end invokes qemu_co_mutex_unlock, what will pop > coroutines from free_sema->queue's head and enter C17. More > free_sema->holder is C17 now. > time request No Actions > 28 17 in_flight=16, Coroutine=C17, > free_sema->holder=C17, mutex->locked=true > > In above scenario, we only recieves request No.16' reply. So as time go > on, nbd client will almostly recieves replies from requests > 1 to 15 rather than request 17 who owns C17. In this case, we will > encounter Assertion "`mutex->holder == self' failed" in nbd_coroutine_end. > For example, if nbd client recieves request No.15' reply: > time request No Actions > 29 15(most case) in_flight=15, Coroutine=C15, > free_sema->holder=C17, mutex->locked = false > > qemu-system-x86_64: util/qemu-coroutine-lock.c:148: > qemu_co_mutex_unlock: Assertion `mutex->holder == self' failed. > > This is introduced by Kevin's patch > commit 0e438cdc932a785de72166af4641aafa103a6670 > Author: Kevin Wolf <kw...@redhat.com> > Date: Thu Aug 11 17:45:06 2016 +0200 > > coroutine: Let CoMutex remember who holds it > > In cases of deadlocks, knowing who holds a given CoMutex is really > helpful for debugging. Keeping the information around doesn't cost much > and allows us to add another assertion to keep the code correct, so > let's just add it. > > Signed-off-by: Kevin Wolf <kw...@redhat.com> > Reviewed-by: Paolo Bonzini <pbonz...@redhat.com> > Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com> > > Any ideas? Is it a nbd bug or should we revert commit 0e438cdc? > > Thanks > -Xie > >