Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

It's like an RFC. I'm not sure, but this place looks like a bug. Shouldn't
we chack client-closing even before nbd_client_receive_next_request() call?

 nbd/server.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index e0de431e10..97b45a21fa 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1547,10 +1547,6 @@ static coroutine_fn void nbd_trip(void *opaque)
         goto disconnect;
     }
 
-    if (ret < 0) {
-        goto reply;
-    }
-
     if (client->closing) {
         /*
          * The client may be closed when we are blocked in
@@ -1559,6 +1555,10 @@ static coroutine_fn void nbd_trip(void *opaque)
         goto done;
     }
 
+    if (ret < 0) {
+        goto reply;
+    }
+
     switch (request.type) {
     case NBD_CMD_READ:
         /* XXX: NBD Protocol only documents use of FUA with WRITE */
-- 
2.11.1

On 03/08/2018 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> It's like an RFC. I'm not sure, but this place looks like a bug. Shouldn't
> we chack client-closing even before nbd_client_receive_next_request() call?
> 
>   nbd/server.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index e0de431e10..97b45a21fa 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1547,10 +1547,6 @@ static coroutine_fn void nbd_trip(void *opaque)

More context:

     ret = nbd_co_receive_request(req, &request, &local_err);
     client->recv_coroutine = NULL;
     nbd_client_receive_next_request(client);
     if (ret == -EIO) {

>           goto disconnect;
>       }
>   

Calling nbd_client_receive_next_request() checks whether recv_coroutine 
is NULL (it is, because we just set it that way) and whether we are up 
to our maximum in parallel request handling; so it likely queues another 
coroutine to call nbd_trip() again.  However, when the next nbd_trip() 
is invoked, the first thing it does (after a trace call) is check 
client->closing, at which point it is a nop.

Your argument is that if ret was -EIO, we goto disconnect (which sets 
client->closing if it was not already set), and thus the just-scheduled 
next nbd_trip() will be a nop.  If ret was anything else, we used to try 
to reply to the client no matter what, which generally works; although 
if client->closing is already set, the attempt to reply is instead 
likely to fail and result in a later attempt to goto disconnect - but at 
that point disconnect is a nop since client->closing is already set. 
Whereas your patch skips the reply to the client if client->closing (and 
can't reach the disconnect code, but that doesn't matter as the 
disconnect attempt did nothing).  Swapping the check for client->closing 
to be earlier than the reply to the client thus looks safe.

Your RFC question is whether we can just check client->closing before 
checking ret, and skip nbd_client_receive_next_request() in that case 
(in other words, why even bother to queue up a coroutine that will do 
nothing, if we already know the client is going away).  And my answer is 
yes, I think that makes more sense.  So that would be:

diff --git c/nbd/server.c w/nbd/server.c
index 5f292064af0..b230ecb4fb8 100644
--- c/nbd/server.c
+++ w/nbd/server.c
@@ -1543,14 +1543,6 @@ static coroutine_fn void nbd_trip(void *opaque)
      req = nbd_request_get(client);
      ret = nbd_co_receive_request(req, &request, &local_err);
      client->recv_coroutine = NULL;
-    nbd_client_receive_next_request(client);
-    if (ret == -EIO) {
-        goto disconnect;
-    }
-
-    if (ret < 0) {
-        goto reply;
-    }

      if (client->closing) {
          /*
@@ -1560,6 +1552,15 @@ static coroutine_fn void nbd_trip(void *opaque)
          goto done;
      }

+    nbd_client_receive_next_request(client);
+    if (ret == -EIO) {
+        goto disconnect;
+    }
+
+    if (ret < 0) {
+        goto reply;
+    }
+
      switch (request.type) {
      case NBD_CMD_READ:
          /* XXX: NBD Protocol only documents use of FUA with WRITE */


Unless this revised form fails testing or gets any other review 
comments, I will go ahead and amend your commit in this manner.

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

On 03/09/2018 03:51 PM, Eric Blake wrote:
> On 03/08/2018 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:

In the subject: s/reply sending/sending reply/

>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> It's like an RFC. I'm not sure, but this place looks like a bug. 
>> Shouldn't
>> we chack client-closing even before nbd_client_receive_next_request() 
>> call?
>>

> Your RFC question is whether we can just check client->closing before 
> checking ret, and skip nbd_client_receive_next_request() in that case 
> (in other words, why even bother to queue up a coroutine that will do 
> nothing, if we already know the client is going away).  And my answer is 
> yes, I think that makes more sense.  So that would be:
> 
> diff --git c/nbd/server.c w/nbd/server.c
> index 5f292064af0..b230ecb4fb8 100644
> --- c/nbd/server.c
> +++ w/nbd/server.c
> @@ -1543,14 +1543,6 @@ static coroutine_fn void nbd_trip(void *opaque)
>       req = nbd_request_get(client);
>       ret = nbd_co_receive_request(req, &request, &local_err);
>       client->recv_coroutine = NULL;
> -    nbd_client_receive_next_request(client);
> -    if (ret == -EIO) {
> -        goto disconnect;
> -    }
> -
> -    if (ret < 0) {
> -        goto reply;
> -    }
> 
>       if (client->closing) {
>           /*
> @@ -1560,6 +1552,15 @@ static coroutine_fn void nbd_trip(void *opaque)
>           goto done;
>       }
> 
> +    nbd_client_receive_next_request(client);
> +    if (ret == -EIO) {
> +        goto disconnect;
> +    }
> +
> +    if (ret < 0) {
> +        goto reply;
> +    }
> +
>       switch (request.type) {
>       case NBD_CMD_READ:
>           /* XXX: NBD Protocol only documents use of FUA with WRITE */
> 
> 
> Unless this revised form fails testing or gets any other review 
> comments, I will go ahead and amend your commit in this manner.

I figured out why iotests were failing for an unrelated issue (thanks to 
Max for recognizing the symptoms on IRC), and my changes still passed, 
so I'm squashing them in as part of staging this series on my NBD queue.

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