[PULL,06/13] nbd: remove peppering of nbd_client_connected

Commit Message

Eric Blake April 26, 2022, 8:15 p.m. UTC

From: Paolo Bonzini <[e-mail]>

It is unnecessary to check nbd_client_connected() because every time
s->state is moved out of NBD_CLIENT_CONNECTED the socket is shut down
and all coroutines are resumed.

The only case where it was actually needed is when the NBD
server disconnects and there is no reconnect-delay.  In that
case, nbd_receive_replies() does not set s->reply.handle and
nbd_co_do_receive_one_chunk() cannot continue.  For that one case,
check the return value of nbd_receive_replies().

As to the others:

* nbd_receive_replies() can put the current coroutine to sleep if another
reply is ongoing; then it will be woken by nbd_channel_error(), called
by the ongoing reply.  Or it can try itself to read a reply header and
fail, thus calling nbd_channel_error() itself.

* nbd_co_send_request() will write the body of the request and fail

* nbd_reply_chunk_iter_receive() will call nbd_co_receive_one_chunk()
and then nbd_co_do_receive_one_chunk(), which will handle the failure as
above; or it will just detect a previous call to nbd_iter_channel_error()
via iter->ret < 0.

Reviewed-by: Eric Blake <[e-mail]>
Signed-off-by: Paolo Bonzini <[e-mail]>
Message-Id: <[e-mail]>
Reviewed-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
Reviewed-by: Lukas Straub <[e-mail]>
Signed-off-by: Eric Blake <[e-mail]>
---
 block/nbd.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

Comments

Peter Maydell May 12, 2022, 4:16 p.m. UTC | #1

On Tue, 26 Apr 2022 at 21:21, Eric Blake <[e-mail]> wrote:
>
> From: Paolo Bonzini <[e-mail]>
>
> It is unnecessary to check nbd_client_connected() because every time
> s->state is moved out of NBD_CLIENT_CONNECTED the socket is shut down
> and all coroutines are resumed.

Hi; Coverity points out (CID 1488362) that this part of this change
has resulted in some dead code:

> @@ -512,7 +508,7 @@ static int coroutine_fn nbd_co_send_request(BlockDriverState *bs,
>      if (qiov) {
>          qio_channel_set_cork(s->ioc, true);
>          rc = nbd_send_request(s->ioc, request);
> -        if (nbd_client_connected(s) && rc >= 0) {
> +        if (rc >= 0) {
>              if (qio_channel_writev_all(s->ioc, qiov->iov, qiov->niov,
>                                         NULL) < 0) {
>                  rc = -EIO;

because the change means this code is now

        if (rc >= 0) {
            if (qio_channel_writev_all(s->ioc, qiov->iov, qiov->niov,
                                       NULL) < 0) {
                rc = -EIO;
            }
        } else if (rc >= 0) {
            rc = -EIO;
        }

and the "else if" clause is dead and can be deleted.

thanks
-- PMM

Eric Blake May 13, 2022, 8:42 p.m. UTC | #2

On Thu, May 12, 2022 at 05:16:04PM +0100, Peter Maydell wrote:
> On Tue, 26 Apr 2022 at 21:21, Eric Blake <[e-mail]> wrote:
> >
> > From: Paolo Bonzini <[e-mail]>
> >
> > It is unnecessary to check nbd_client_connected() because every time
> > s->state is moved out of NBD_CLIENT_CONNECTED the socket is shut down
> > and all coroutines are resumed.
> 
> Hi; Coverity points out (CID 1488362) that this part of this change
> has resulted in some dead code:
> 
> > @@ -512,7 +508,7 @@ static int coroutine_fn nbd_co_send_request(BlockDriverState *bs,
> >      if (qiov) {
> >          qio_channel_set_cork(s->ioc, true);
> >          rc = nbd_send_request(s->ioc, request);
> > -        if (nbd_client_connected(s) && rc >= 0) {
> > +        if (rc >= 0) {
> >              if (qio_channel_writev_all(s->ioc, qiov->iov, qiov->niov,
> >                                         NULL) < 0) {
> >                  rc = -EIO;
> 
> because the change means this code is now
> 
>         if (rc >= 0) {
>             if (qio_channel_writev_all(s->ioc, qiov->iov, qiov->niov,
>                                        NULL) < 0) {
>                 rc = -EIO;
>             }
>         } else if (rc >= 0) {
>             rc = -EIO;
>         }
> 
> and the "else if" clause is dead and can be deleted.

Thanks for reporting it.  I can prepare a patch.

Patch

diff --git a/block/nbd.c b/block/nbd.c
index 3a6e60920304..326546f6cd4c 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -409,10 +409,6 @@  static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t handle)
             return 0;
         }

-        if (!nbd_client_connected(s)) {
-            return -EIO;
-        }
-
         if (s->reply.handle != 0) {
             /*
              * Some other request is being handled now. It should already be
@@ -512,7 +508,7 @@  static int coroutine_fn nbd_co_send_request(BlockDriverState *bs,
     if (qiov) {
         qio_channel_set_cork(s->ioc, true);
         rc = nbd_send_request(s->ioc, request);
-        if (nbd_client_connected(s) && rc >= 0) {
+        if (rc >= 0) {
             if (qio_channel_writev_all(s->ioc, qiov->iov, qiov->niov,
                                        NULL) < 0) {
                 rc = -EIO;
@@ -829,8 +825,8 @@  static coroutine_fn int nbd_co_do_receive_one_chunk(
     }
     *request_ret = 0;

-    nbd_receive_replies(s, handle);
-    if (!nbd_client_connected(s)) {
+    ret = nbd_receive_replies(s, handle);
+    if (ret < 0) {
         error_setg(errp, "Connection closed");
         return -EIO;
     }
@@ -982,11 +978,6 @@  static bool nbd_reply_chunk_iter_receive(BDRVNBDState *s,
     NBDReply local_reply;
     NBDStructuredReplyChunk *chunk;
     Error *local_err = NULL;
-    if (!nbd_client_connected(s)) {
-        error_setg(&local_err, "Connection closed");
-        nbd_iter_channel_error(iter, -EIO, &local_err);
-        goto break_loop;
-    }

     if (iter->done) {
         /* Previous iteration was last. */
@@ -1007,7 +998,7 @@  static bool nbd_reply_chunk_iter_receive(BDRVNBDState *s,
     }

     /* Do not execute the body of NBD_FOREACH_REPLY_CHUNK for simple reply. */
-    if (nbd_reply_is_simple(reply) || !nbd_client_connected(s)) {
+    if (nbd_reply_is_simple(reply) || iter->ret < 0) {
         goto break_loop;
     }