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