[PATCH v3 32/33] block/nbd: safer transition to receiving request

Vladimir Sementsov-Ogievskiy posted 33 patches 4 years, 9 months ago
There is a newer version of this series
[PATCH v3 32/33] block/nbd: safer transition to receiving request
Posted by Vladimir Sementsov-Ogievskiy 4 years, 9 months ago
req->receiving is a flag of request being in one concrete yield point
in nbd_co_do_receive_one_chunk().

Such kind of boolean flag is always better to unset before scheduling
the coroutine, to avoid double scheduling. So, let's be more careful.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 6cc563e13d..03391bb231 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -152,6 +152,7 @@ static void nbd_recv_coroutines_wake_all(BDRVNBDState *s)
         NBDClientRequest *req = &s->requests[i];
 
         if (req->coroutine && req->receiving) {
+            req->receiving = false;
             aio_co_wake(req->coroutine);
         }
     }
@@ -552,6 +553,7 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
          *   connection_co happens through a bottom half, which can only
          *   run after we yield.
          */
+        s->requests[i].receiving = false;
         aio_co_wake(s->requests[i].coroutine);
         qemu_coroutine_yield();
     }
@@ -614,7 +616,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
     if (qiov) {
         qio_channel_set_cork(s->ioc, true);
         rc = nbd_send_request(s->ioc, request);
-        if (nbd_clinet_connected(s) && rc >= 0) {
+        if (nbd_client_connected(s) && rc >= 0) {
             if (qio_channel_writev_all(s->ioc, qiov->iov, qiov->niov,
                                        NULL) < 0) {
                 rc = -EIO;
@@ -938,7 +940,7 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
     /* Wait until we're woken up by nbd_connection_entry.  */
     s->requests[i].receiving = true;
     qemu_coroutine_yield();
-    s->requests[i].receiving = false;
+    assert(!s->requests[i].receiving);
     if (!nbd_client_connected(s)) {
         error_setg(errp, "Connection closed");
         return -EIO;
-- 
2.29.2


Re: [PATCH v3 32/33] block/nbd: safer transition to receiving request
Posted by Eric Blake 4 years, 8 months ago
On Fri, Apr 16, 2021 at 11:09:10AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> req->receiving is a flag of request being in one concrete yield point
> in nbd_co_do_receive_one_chunk().
> 
> Such kind of boolean flag is always better to unset before scheduling
> the coroutine, to avoid double scheduling. So, let's be more careful.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/nbd.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 

> @@ -614,7 +616,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
>      if (qiov) {
>          qio_channel_set_cork(s->ioc, true);
>          rc = nbd_send_request(s->ioc, request);
> -        if (nbd_clinet_connected(s) && rc >= 0) {
> +        if (nbd_client_connected(s) && rc >= 0) {

Ouch - typo fix in clinet seems unrelated in this fix; please hoist it
into the correct point in the series so that we don't have the typo in
the first place.

Otherwise,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


Re: [PATCH v3 32/33] block/nbd: safer transition to receiving request
Posted by Vladimir Sementsov-Ogievskiy 4 years, 8 months ago
04.06.2021 00:11, Eric Blake wrote:
> On Fri, Apr 16, 2021 at 11:09:10AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> req->receiving is a flag of request being in one concrete yield point
>> in nbd_co_do_receive_one_chunk().
>>
>> Such kind of boolean flag is always better to unset before scheduling
>> the coroutine, to avoid double scheduling. So, let's be more careful.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/nbd.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
> 
>> @@ -614,7 +616,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
>>       if (qiov) {
>>           qio_channel_set_cork(s->ioc, true);
>>           rc = nbd_send_request(s->ioc, request);
>> -        if (nbd_clinet_connected(s) && rc >= 0) {
>> +        if (nbd_client_connected(s) && rc >= 0) {
> 
> Ouch - typo fix in clinet seems unrelated in this fix; please hoist it
> into the correct point in the series so that we don't have the typo in
> the first place.

That also means that I didn't made my favorite "git rebase -x 'make -j9' master".. Not good, will fix of course.

> 
> Otherwise,
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 


-- 
Best regards,
Vladimir