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

[Changlong Xie]

On 10/11/2016 06:47 PM, Paolo Bonzini wrote:

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 your explanation! will send out a patch later.


Thanks
-Xie


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

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

[Paolo Bonzini]

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
> ...   ...
> 1515  in_flight=15, Coroutine=C15
> 1616  in_flight=16, Coroutine=C16,
> free_sema->holder=C16, mutex->locked=true
> 1717  in_flight=16, Coroutine=C17, queue C17 into
> free_sema->queue
> 1818  in_flight=16, Coroutine=C18, queue C18 into
> free_sema->queue
> ...   ...
> 26N   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
> 2716  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
> 2817  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 
> 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 
> Reviewed-by: Paolo Bonzini 
> Reviewed-by: Stefan Hajnoczi 
> 
> Any ideas? Is it a nbd bug or should we revert commit 0e438cdc?
> 
> Thanks
> -Xie
> 
>