Re: [Qemu-block] [PATCH 3/5] nbd/server: fix: check client->closing before reply sending

[Eric Blake]

On 03/08/2018 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>
> 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