diff mbox

[06/11] nbd: make it thread-safe

Message ID 20170531094330.1808-7-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini May 31, 2017, 9:43 a.m. UTC 
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/nbd-client.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

Comments

Eric Blake May 31, 2017, 9:20 p.m. UTC | #1 
On 05/31/2017 04:43 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/nbd-client.c | 30 +++++++++---------------------
>  1 file changed, 9 insertions(+), 21 deletions(-)
> 
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 1e2952fdae..43e0292ac1 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -114,6 +114,10 @@ static int nbd_co_send_request(BlockDriverState *bs,
>      int rc, ret, i;
>  
>      qemu_co_mutex_lock(&s->send_mutex);
> +    while (s->in_flight == MAX_NBD_REQUESTS) {
> +        qemu_co_queue_wait(&s->free_sema, &s->send_mutex);
> +    }
> +    s->in_flight++;

Nice - if I'm not mistaken, this also solves
https://bugzilla.redhat.com/show_bug.cgi?id=1454582 - you have a while
loop here...

> -static void nbd_coroutine_start(NBDClientSession *s,
> -                                NBDRequest *request)
> -{
> -    /* 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) {
> -        qemu_co_queue_wait(&s->free_sema, NULL);
> -        assert(s->in_flight < MAX_NBD_REQUESTS);
> -    }

...compared to the old code that only tried once, and could therefore
hit the assertion failure depending on thread load.

Reviewed-by: Eric Blake <eblake@redhat.com>
diff mbox

Patch

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 1e2952fdae..43e0292ac1 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -114,6 +114,10 @@  static int nbd_co_send_request(BlockDriverState *bs,
     int rc, ret, i;
 
     qemu_co_mutex_lock(&s->send_mutex);
+    while (s->in_flight == MAX_NBD_REQUESTS) {
+        qemu_co_queue_wait(&s->free_sema, &s->send_mutex);
+    }
+    s->in_flight++;
 
     for (i = 0; i < MAX_NBD_REQUESTS; i++) {
         if (s->recv_coroutine[i] == NULL) {
@@ -176,20 +180,6 @@  static void nbd_co_receive_reply(NBDClientSession *s,
     }
 }
 
-static void nbd_coroutine_start(NBDClientSession *s,
-                                NBDRequest *request)
-{
-    /* 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) {
-        qemu_co_queue_wait(&s->free_sema, NULL);
-        assert(s->in_flight < MAX_NBD_REQUESTS);
-    }
-    s->in_flight++;
-
-    /* s->recv_coroutine[i] is set as soon as we get the send_lock.  */
-}
-
 static void nbd_coroutine_end(BlockDriverState *bs,
                               NBDRequest *request)
 {
@@ -197,13 +187,16 @@  static void nbd_coroutine_end(BlockDriverState *bs,
     int i = HANDLE_TO_INDEX(s, request->handle);
 
     s->recv_coroutine[i] = NULL;
-    s->in_flight--;
-    qemu_co_queue_next(&s->free_sema);
 
     /* Kick the read_reply_co to get the next reply.  */
     if (s->read_reply_co) {
         aio_co_wake(s->read_reply_co);
     }
+
+    qemu_co_mutex_lock(&s->send_mutex);
+    s->in_flight--;
+    qemu_co_queue_next(&s->free_sema);
+    qemu_co_mutex_unlock(&s->send_mutex);
 }
 
 int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
@@ -221,7 +214,6 @@  int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
     assert(bytes <= NBD_MAX_BUFFER_SIZE);
     assert(!flags);
 
-    nbd_coroutine_start(client, &request);
     ret = nbd_co_send_request(bs, &request, NULL);
     if (ret < 0) {
         reply.error = -ret;
@@ -251,7 +243,6 @@  int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,
 
     assert(bytes <= NBD_MAX_BUFFER_SIZE);
 
-    nbd_coroutine_start(client, &request);
     ret = nbd_co_send_request(bs, &request, qiov);
     if (ret < 0) {
         reply.error = -ret;
@@ -286,7 +277,6 @@  int nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
         request.flags |= NBD_CMD_FLAG_NO_HOLE;
     }
 
-    nbd_coroutine_start(client, &request);
     ret = nbd_co_send_request(bs, &request, NULL);
     if (ret < 0) {
         reply.error = -ret;
@@ -311,7 +301,6 @@  int nbd_client_co_flush(BlockDriverState *bs)
     request.from = 0;
     request.len = 0;
 
-    nbd_coroutine_start(client, &request);
     ret = nbd_co_send_request(bs, &request, NULL);
     if (ret < 0) {
         reply.error = -ret;
@@ -337,7 +326,6 @@  int nbd_client_co_pdiscard(BlockDriverState *bs, int64_t offset, int count)
         return 0;
     }
 
-    nbd_coroutine_start(client, &request);
     ret = nbd_co_send_request(bs, &request, NULL);
     if (ret < 0) {
         reply.error = -ret;