Re: [Qemu-devel] [Questions] NBD issue or CoMutex->holder issue?

[Paolo Bonzini <pbonzini@redhat.com>]

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 <kwolf@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 <kwolf@redhat.com>
>     Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>     Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Any ideas? Is it a nbd bug or should we revert commit 0e438cdc?
> 
> Thanks
>     -Xie
> 
>