[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] [PULL 02/30] nbd: Use CoQueue for free_sema instead of CoMu
From: |
Paolo Bonzini |
Subject: |
[Qemu-devel] [PULL 02/30] nbd: Use CoQueue for free_sema instead of CoMutex |
Date: |
Tue, 1 Nov 2016 17:29:15 +0100 |
From: Changlong Xie <address@hidden>
NBD is using the CoMutex in a way that wasn't anticipated. For example, if
there are
N(N=26, MAX_NBD_REQUESTS=16) nbd write requests, so we will invoke
nbd_client_co_pwritev
N times.
----------------------------------------------------------------------------------------
time request 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 C16. It's ok,
because
it's equal to 'free_sema->holder'.
----------------------------------------------------------------------------------------
time request 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 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. As time goes by, nbd
client will
almostly recieves replies from requests 1 to 15 rather than request 17 who owns
C17. In this
case, we will encounter assert "mutex->holder == self" failed since Kevin's
commit 0e438cdc
"coroutine: Let CoMutex remember who holds it". For example, if nbd client
recieves request
No.15' reply, qemu will stop unexpectedly:
----------------------------------------------------------------------------------------
time request Actions
29 15(most case) in_flight=15, Coroutine=C15, free_sema->holder=C17,
mutex->locked=false
----------------------------------------------------------------------------------------
Per Paolo's suggestion "The simplest fix is to change it to CoQueue, which is
like a condition
variable", this patch replaces CoMutex with CoQueue.
Cc: Wen Congyang <address@hidden>
Reported-by: zhanghailiang <address@hidden>
Suggested-by: Paolo Bonzini <address@hidden>
Signed-off-by: Changlong Xie <address@hidden>
Message-Id: <address@hidden>
Signed-off-by: Paolo Bonzini <address@hidden>
---
block/nbd-client.c | 8 ++++----
block/nbd-client.h | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 2cf3237..40b28ab 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -199,8 +199,8 @@ static void nbd_coroutine_start(NbdClientSession *s,
{
/* Poor man semaphore. The free_sema is locked when no other request
* can be accepted, and unlocked after receiving one reply. */
- if (s->in_flight >= MAX_NBD_REQUESTS - 1) {
- qemu_co_mutex_lock(&s->free_sema);
+ if (s->in_flight == MAX_NBD_REQUESTS) {
+ qemu_co_queue_wait(&s->free_sema);
assert(s->in_flight < MAX_NBD_REQUESTS);
}
s->in_flight++;
@@ -214,7 +214,7 @@ static void nbd_coroutine_end(NbdClientSession *s,
int i = HANDLE_TO_INDEX(s, request->handle);
s->recv_coroutine[i] = NULL;
if (s->in_flight-- == MAX_NBD_REQUESTS) {
- qemu_co_mutex_unlock(&s->free_sema);
+ qemu_co_queue_next(&s->free_sema);
}
}
@@ -386,7 +386,7 @@ int nbd_client_init(BlockDriverState *bs,
}
qemu_co_mutex_init(&client->send_mutex);
- qemu_co_mutex_init(&client->free_sema);
+ qemu_co_queue_init(&client->free_sema);
client->sioc = sioc;
object_ref(OBJECT(client->sioc));
diff --git a/block/nbd-client.h b/block/nbd-client.h
index 044aca4..307b8b1 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -24,7 +24,7 @@ typedef struct NbdClientSession {
off_t size;
CoMutex send_mutex;
- CoMutex free_sema;
+ CoQueue free_sema;
Coroutine *send_coroutine;
int in_flight;
--
2.7.4
- [Qemu-devel] [PULL v2 00/30] Misc patches for 2016-10-31, Paolo Bonzini, 2016/11/01
- [Qemu-devel] [PULL 01/30] checkpatch: tweak "struct should normally be const" warning, Paolo Bonzini, 2016/11/01
- [Qemu-devel] [PULL 02/30] nbd: Use CoQueue for free_sema instead of CoMutex,
Paolo Bonzini <=
- [Qemu-devel] [PULL 04/30] tests: send error_report to test log, Paolo Bonzini, 2016/11/01
- [Qemu-devel] [PULL 03/30] qemu-error: remove dependency of stubs on monitor, Paolo Bonzini, 2016/11/01
- [Qemu-devel] [PULL 05/30] exec.c: ensure all AddressSpaceDispatch updates under RCU, Paolo Bonzini, 2016/11/01
- [Qemu-devel] [PULL 07/30] exec.c: check memory backend file size with 'size' option, Paolo Bonzini, 2016/11/01
- [Qemu-devel] [PULL 06/30] exec.c: do not truncate non-empty memory backend file, Paolo Bonzini, 2016/11/01
- [Qemu-devel] [PULL 08/30] nbd: Add qemu-nbd -D for human-readable description, Paolo Bonzini, 2016/11/01
- [Qemu-devel] [PULL 10/30] nbd: Rename NBDRequest to NBDRequestData, Paolo Bonzini, 2016/11/01
- [Qemu-devel] [PULL 09/30] nbd: Treat flags vs. command type as separate fields, Paolo Bonzini, 2016/11/01
- [Qemu-devel] [PULL 13/30] nbd: Share common reply-sending code in server, Paolo Bonzini, 2016/11/01
- [Qemu-devel] [PULL 11/30] nbd: Rename NbdClientSession to NBDClientSession, Paolo Bonzini, 2016/11/01