[Qemu-devel] [PATCH v6 0/7] NBD reconnect

[Qemu-devel] [PATCH v6 0/7] NBD reconnect

[Vladimir Sementsov-Ogievskiy]

Here is NBD reconnect. Previously, if connection failed all current
and future requests will fail. After the series, nbd-client driver
will try to reconnect unlimited times. During first @reconnect-delay
seconds of reconnecting all requests will wait for the connection,
and if it is established requests will be resent. After
@reconnect-delay period all requests will be failed (until successful
reconnect).

v4->v6:
 (some preparing patches from v4 was merged as v5)
01: new
02: new
03: - drop unused things
       - future states
       - future s/break/continue/
04: - fix typos
    - s/3.1/4.1/
    - set 0 as default
05: new
06: - new states and s/break/continue/ moved here from 03
    - drop NBDClientSession.receiving, as now in_flight
      requests used instread
    - add copyright
    - go to NBD_CLIENT_CONNECTING_NOWAIT immediately if
      reconnect_delay is 0 (so, reconnect_delay moved to
      NBDClientSession)
    - on close, do qemu_co_sleep_wake(client->connection_co),
      to not wait for reconnect loop iteration
    - handle state transition to QUIT during reconnect loop
      (assert(nbd_client_connecting(s)) was bad idea)
    - don't try to fail on protocol errors after
      nbd_client_connect, as we can't distinguish them
    - decrement in_flight around reconnect sleep to make
      it possible to drain and exit during it
      (v4 was based on something before in_flight logic
       introduced into nbd-client)
    - changed logic in nbd_client_attach_aio_context 
07: - refactor, using log and qmp_log
    - drop export name
    - drop strange try/except
    - add reconnect-delay option (as 0 is a default now)


Vladimir Sementsov-Ogievskiy (7):
  block/nbd-client: split connection_co start out of nbd_client_connect
  block/nbd-client: use non-blocking io channel for nbd negotiation
  block/nbd-client: move from quit to state
  block/nbd: add cmdline and qapi parameter reconnect-delay
  qemu-coroutine-sleep: introduce qemu_co_sleep_wake
  block/nbd-client: nbd reconnect
  iotests: test nbd reconnect

 qapi/block-core.json          |  12 +-
 block/nbd-client.h            |  15 +-
 include/block/nbd.h           |   3 +-
 include/qemu/coroutine.h      |   6 +
 block/nbd-client.c            | 416 +++++++++++++++++++++++++---------
 block/nbd.c                   |  16 +-
 nbd/client.c                  |  16 +-
 qemu-nbd.c                    |   2 +-
 util/qemu-coroutine-sleep.c   |  20 +-
 tests/qemu-iotests/249        |  63 +++++
 tests/qemu-iotests/249.out    |  10 +
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |   4 +
 13 files changed, 468 insertions(+), 116 deletions(-)
 create mode 100755 tests/qemu-iotests/249
 create mode 100644 tests/qemu-iotests/249.out

-- 
2.18.0

[Qemu-devel] [PATCH v6 0/7] NBD reconnect

[Vladimir Sementsov-Ogievskiy]

Here is NBD reconnect. Previously, if connection failed all current
and future requests will fail. After the series, nbd-client driver
will try to reconnect unlimited times. During first @reconnect-delay
seconds of reconnecting all requests will wait for the connection,
and if it is established requests will be resent. After
@reconnect-delay period all requests will be failed (until successful
reconnect).

v4->v6:
 (some preparing patches from v4 was merged as v5)
01: new
02: new
03: - drop unused things
       - future states
       - future s/break/continue/
04: - fix typos
    - s/3.1/4.1/
    - set 0 as default
05: new
06: - new states and s/break/continue/ moved here from 03
    - drop NBDClientSession.receiving, as now in_flight
      requests used instread
    - add copyright
    - go to NBD_CLIENT_CONNECTING_NOWAIT immediately if
      reconnect_delay is 0 (so, reconnect_delay moved to
      NBDClientSession)
    - on close, do qemu_co_sleep_wake(client->connection_co),
      to not wait for reconnect loop iteration
    - handle state transition to QUIT during reconnect loop
      (assert(nbd_client_connecting(s)) was bad idea)
    - don't try to fail on protocol errors after
      nbd_client_connect, as we can't distinguish them
    - decrement in_flight around reconnect sleep to make
      it possible to drain and exit during it
      (v4 was based on something before in_flight logic
       introduced into nbd-client)
    - changed logic in nbd_client_attach_aio_context 
07: - refactor, using log and qmp_log
    - drop export name
    - drop strange try/except
    - add reconnect-delay option (as 0 is a default now)


Vladimir Sementsov-Ogievskiy (7):
  block/nbd-client: split connection_co start out of nbd_client_connect
  block/nbd-client: use non-blocking io channel for nbd negotiation
  block/nbd-client: move from quit to state
  block/nbd: add cmdline and qapi parameter reconnect-delay
  qemu-coroutine-sleep: introduce qemu_co_sleep_wake
  block/nbd-client: nbd reconnect
  iotests: test nbd reconnect

 qapi/block-core.json          |  12 +-
 block/nbd-client.h            |  15 +-
 include/block/nbd.h           |   3 +-
 include/qemu/coroutine.h      |   6 +
 block/nbd-client.c            | 416 +++++++++++++++++++++++++---------
 block/nbd.c                   |  16 +-
 nbd/client.c                  |  16 +-
 qemu-nbd.c                    |   2 +-
 util/qemu-coroutine-sleep.c   |  20 +-
 tests/qemu-iotests/249        |  63 +++++
 tests/qemu-iotests/249.out    |  10 +
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |   4 +
 13 files changed, 468 insertions(+), 116 deletions(-)
 create mode 100755 tests/qemu-iotests/249
 create mode 100644 tests/qemu-iotests/249.out

-- 
2.18.0

[Qemu-devel] [PATCH v6 1/7] block/nbd-client: split connection_co start out of nbd_client_connect

[Vladimir Sementsov-Ogievskiy]

nbd_client_connect is going to be used from connection_co, so, let's
refactor nbd_client_connect in advance, leaving io channel
configuration all in nbd_client_connect.

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

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 790ecc1ee1..facf8f8099 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -1180,12 +1180,8 @@ static int nbd_client_connect(BlockDriverState *bs,
         object_ref(OBJECT(client->ioc));
     }
 
-    /* Now that we're connected, set the socket to be non-blocking and
-     * kick the reply mechanism.  */
     qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
-    client->connection_co = qemu_coroutine_create(nbd_connection_entry, client);
-    bdrv_inc_in_flight(bs);
-    nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));
+    qio_channel_attach_aio_context(client->ioc, bdrv_get_aio_context(bs));
 
     logout("Established connection with NBD server\n");
     return 0;
@@ -1215,12 +1211,22 @@ int nbd_client_init(BlockDriverState *bs,
                     const char *x_dirty_bitmap,
                     Error **errp)
 {
+    int ret;
     NBDClientSession *client = nbd_get_client_session(bs);
 
     client->bs = bs;
     qemu_co_mutex_init(&client->send_mutex);
     qemu_co_queue_init(&client->free_sema);
 
-    return nbd_client_connect(bs, saddr, export, tlscreds, hostname,
-                              x_dirty_bitmap, errp);
+    ret = nbd_client_connect(bs, saddr, export, tlscreds, hostname,
+                             x_dirty_bitmap, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    client->connection_co = qemu_coroutine_create(nbd_connection_entry, client);
+    bdrv_inc_in_flight(bs);
+    aio_co_schedule(bdrv_get_aio_context(bs), client->connection_co);
+
+    return 0;
 }
-- 
2.18.0

[Qemu-devel] [PATCH v6 1/7] block/nbd-client: split connection_co start out of nbd_client_connect

[Vladimir Sementsov-Ogievskiy]

nbd_client_connect is going to be used from connection_co, so, let's
refactor nbd_client_connect in advance, leaving io channel
configuration all in nbd_client_connect.

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

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 790ecc1ee1..facf8f8099 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -1180,12 +1180,8 @@ static int nbd_client_connect(BlockDriverState *bs,
         object_ref(OBJECT(client->ioc));
     }
 
-    /* Now that we're connected, set the socket to be non-blocking and
-     * kick the reply mechanism.  */
     qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
-    client->connection_co = qemu_coroutine_create(nbd_connection_entry, client);
-    bdrv_inc_in_flight(bs);
-    nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));
+    qio_channel_attach_aio_context(client->ioc, bdrv_get_aio_context(bs));
 
     logout("Established connection with NBD server\n");
     return 0;
@@ -1215,12 +1211,22 @@ int nbd_client_init(BlockDriverState *bs,
                     const char *x_dirty_bitmap,
                     Error **errp)
 {
+    int ret;
     NBDClientSession *client = nbd_get_client_session(bs);
 
     client->bs = bs;
     qemu_co_mutex_init(&client->send_mutex);
     qemu_co_queue_init(&client->free_sema);
 
-    return nbd_client_connect(bs, saddr, export, tlscreds, hostname,
-                              x_dirty_bitmap, errp);
+    ret = nbd_client_connect(bs, saddr, export, tlscreds, hostname,
+                             x_dirty_bitmap, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    client->connection_co = qemu_coroutine_create(nbd_connection_entry, client);
+    bdrv_inc_in_flight(bs);
+    aio_co_schedule(bdrv_get_aio_context(bs), client->connection_co);
+
+    return 0;
 }
-- 
2.18.0

Re: [Qemu-devel] [PATCH v6 1/7] block/nbd-client: split connection_co start out of nbd_client_connect

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 606 bytes --]

On 4/11/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
> nbd_client_connect is going to be used from connection_co, so, let's
> refactor nbd_client_connect in advance, leaving io channel
> configuration all in nbd_client_connect.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/nbd-client.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v6 2/7] block/nbd-client: use non-blocking io channel for nbd negotiation

[Vladimir Sementsov-Ogievskiy]

No reason to use blocking channel for negotiation and we'll benefit in
further reconnect feature, as qio_channel reads and writes will do
qemu_coroutine_yield while waiting for io completion.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/nbd.h |  3 ++-
 block/nbd-client.c  | 16 +++++++---------
 nbd/client.c        | 16 +++++++++++-----
 qemu-nbd.c          |  2 +-
 4 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 6d05983a55..6961835654 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -290,7 +290,8 @@ struct NBDExportInfo {
 };
 typedef struct NBDExportInfo NBDExportInfo;
 
-int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
+int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc,
+                          QCryptoTLSCreds *tlscreds,
                           const char *hostname, QIOChannel **outioc,
                           NBDExportInfo *info, Error **errp);
 void nbd_free_export_list(NBDExportInfo *info, int count);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index facf8f8099..f9cee25865 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -1123,6 +1123,7 @@ static int nbd_client_connect(BlockDriverState *bs,
                               Error **errp)
 {
     NBDClientSession *client = nbd_get_client_session(bs);
+    AioContext *aio_context = bdrv_get_aio_context(bs);
     int ret;
 
     /*
@@ -1137,15 +1138,16 @@ static int nbd_client_connect(BlockDriverState *bs,
 
     /* NBD handshake */
     logout("session init %s\n", export);
-    qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
+    qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
+    qio_channel_attach_aio_context(QIO_CHANNEL(sioc), aio_context);
 
     client->info.request_sizes = true;
     client->info.structured_reply = true;
     client->info.base_allocation = true;
     client->info.x_dirty_bitmap = g_strdup(x_dirty_bitmap);
     client->info.name = g_strdup(export ?: "");
-    ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), tlscreds, hostname,
-                                &client->ioc, &client->info, errp);
+    ret = nbd_receive_negotiate(aio_context, QIO_CHANNEL(sioc), tlscreds,
+                                hostname, &client->ioc, &client->info, errp);
     g_free(client->info.x_dirty_bitmap);
     g_free(client->info.name);
     if (ret < 0) {
@@ -1180,17 +1182,13 @@ static int nbd_client_connect(BlockDriverState *bs,
         object_ref(OBJECT(client->ioc));
     }
 
-    qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
-    qio_channel_attach_aio_context(client->ioc, bdrv_get_aio_context(bs));
-
     logout("Established connection with NBD server\n");
     return 0;
 
  fail:
     /*
-     * We have connected, but must fail for other reasons. The
-     * connection is still blocking; send NBD_CMD_DISC as a courtesy
-     * to the server.
+     * We have connected, but must fail for other reasons.
+     * Send NBD_CMD_DISC as a courtesy to the server.
      */
     {
         NBDRequest request = { .type = NBD_CMD_DISC };
diff --git a/nbd/client.c b/nbd/client.c
index 4de30630c7..8f524c3e35 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -867,7 +867,8 @@ static int nbd_list_meta_contexts(QIOChannel *ioc,
  *          2: server is newstyle, but lacks structured replies
  *          3: server is newstyle and set up for structured replies
  */
-static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
+static int nbd_start_negotiate(AioContext *aio_context, QIOChannel *ioc,
+                               QCryptoTLSCreds *tlscreds,
                                const char *hostname, QIOChannel **outioc,
                                bool structured_reply, bool *zeroes,
                                Error **errp)
@@ -934,6 +935,10 @@ static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
                     return -EINVAL;
                 }
                 ioc = *outioc;
+                if (aio_context) {
+                    qio_channel_set_blocking(ioc, false, NULL);
+                    qio_channel_attach_aio_context(ioc, aio_context);
+                }
             } else {
                 error_setg(errp, "Server does not support STARTTLS");
                 return -EINVAL;
@@ -998,7 +1003,8 @@ static int nbd_negotiate_finish_oldstyle(QIOChannel *ioc, NBDExportInfo *info,
  * Returns: negative errno: failure talking to server
  *          0: server is connected
  */
-int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
+int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc,
+                          QCryptoTLSCreds *tlscreds,
                           const char *hostname, QIOChannel **outioc,
                           NBDExportInfo *info, Error **errp)
 {
@@ -1009,7 +1015,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
     assert(info->name);
     trace_nbd_receive_negotiate_name(info->name);
 
-    result = nbd_start_negotiate(ioc, tlscreds, hostname, outioc,
+    result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname, outioc,
                                  info->structured_reply, &zeroes, errp);
 
     info->structured_reply = false;
@@ -1129,8 +1135,8 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
     QIOChannel *sioc = NULL;
 
     *info = NULL;
-    result = nbd_start_negotiate(ioc, tlscreds, hostname, &sioc, true, NULL,
-                                 errp);
+    result = nbd_start_negotiate(NULL, ioc, tlscreds, hostname, &sioc, true,
+                                 NULL, errp);
     if (tlscreds && sioc) {
         ioc = sioc;
     }
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 941ba729c2..ed38b55428 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -370,7 +370,7 @@ static void *nbd_client_thread(void *arg)
         goto out;
     }
 
-    ret = nbd_receive_negotiate(QIO_CHANNEL(sioc),
+    ret = nbd_receive_negotiate(NULL, QIO_CHANNEL(sioc),
                                 NULL, NULL, NULL, &info, &local_error);
     if (ret < 0) {
         if (local_error) {
-- 
2.18.0

[Qemu-devel] [PATCH v6 2/7] block/nbd-client: use non-blocking io channel for nbd negotiation

[Vladimir Sementsov-Ogievskiy]

No reason to use blocking channel for negotiation and we'll benefit in
further reconnect feature, as qio_channel reads and writes will do
qemu_coroutine_yield while waiting for io completion.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/nbd.h |  3 ++-
 block/nbd-client.c  | 16 +++++++---------
 nbd/client.c        | 16 +++++++++++-----
 qemu-nbd.c          |  2 +-
 4 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 6d05983a55..6961835654 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -290,7 +290,8 @@ struct NBDExportInfo {
 };
 typedef struct NBDExportInfo NBDExportInfo;
 
-int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
+int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc,
+                          QCryptoTLSCreds *tlscreds,
                           const char *hostname, QIOChannel **outioc,
                           NBDExportInfo *info, Error **errp);
 void nbd_free_export_list(NBDExportInfo *info, int count);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index facf8f8099..f9cee25865 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -1123,6 +1123,7 @@ static int nbd_client_connect(BlockDriverState *bs,
                               Error **errp)
 {
     NBDClientSession *client = nbd_get_client_session(bs);
+    AioContext *aio_context = bdrv_get_aio_context(bs);
     int ret;
 
     /*
@@ -1137,15 +1138,16 @@ static int nbd_client_connect(BlockDriverState *bs,
 
     /* NBD handshake */
     logout("session init %s\n", export);
-    qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
+    qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
+    qio_channel_attach_aio_context(QIO_CHANNEL(sioc), aio_context);
 
     client->info.request_sizes = true;
     client->info.structured_reply = true;
     client->info.base_allocation = true;
     client->info.x_dirty_bitmap = g_strdup(x_dirty_bitmap);
     client->info.name = g_strdup(export ?: "");
-    ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), tlscreds, hostname,
-                                &client->ioc, &client->info, errp);
+    ret = nbd_receive_negotiate(aio_context, QIO_CHANNEL(sioc), tlscreds,
+                                hostname, &client->ioc, &client->info, errp);
     g_free(client->info.x_dirty_bitmap);
     g_free(client->info.name);
     if (ret < 0) {
@@ -1180,17 +1182,13 @@ static int nbd_client_connect(BlockDriverState *bs,
         object_ref(OBJECT(client->ioc));
     }
 
-    qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
-    qio_channel_attach_aio_context(client->ioc, bdrv_get_aio_context(bs));
-
     logout("Established connection with NBD server\n");
     return 0;
 
  fail:
     /*
-     * We have connected, but must fail for other reasons. The
-     * connection is still blocking; send NBD_CMD_DISC as a courtesy
-     * to the server.
+     * We have connected, but must fail for other reasons.
+     * Send NBD_CMD_DISC as a courtesy to the server.
      */
     {
         NBDRequest request = { .type = NBD_CMD_DISC };
diff --git a/nbd/client.c b/nbd/client.c
index 4de30630c7..8f524c3e35 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -867,7 +867,8 @@ static int nbd_list_meta_contexts(QIOChannel *ioc,
  *          2: server is newstyle, but lacks structured replies
  *          3: server is newstyle and set up for structured replies
  */
-static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
+static int nbd_start_negotiate(AioContext *aio_context, QIOChannel *ioc,
+                               QCryptoTLSCreds *tlscreds,
                                const char *hostname, QIOChannel **outioc,
                                bool structured_reply, bool *zeroes,
                                Error **errp)
@@ -934,6 +935,10 @@ static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
                     return -EINVAL;
                 }
                 ioc = *outioc;
+                if (aio_context) {
+                    qio_channel_set_blocking(ioc, false, NULL);
+                    qio_channel_attach_aio_context(ioc, aio_context);
+                }
             } else {
                 error_setg(errp, "Server does not support STARTTLS");
                 return -EINVAL;
@@ -998,7 +1003,8 @@ static int nbd_negotiate_finish_oldstyle(QIOChannel *ioc, NBDExportInfo *info,
  * Returns: negative errno: failure talking to server
  *          0: server is connected
  */
-int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
+int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc,
+                          QCryptoTLSCreds *tlscreds,
                           const char *hostname, QIOChannel **outioc,
                           NBDExportInfo *info, Error **errp)
 {
@@ -1009,7 +1015,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
     assert(info->name);
     trace_nbd_receive_negotiate_name(info->name);
 
-    result = nbd_start_negotiate(ioc, tlscreds, hostname, outioc,
+    result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname, outioc,
                                  info->structured_reply, &zeroes, errp);
 
     info->structured_reply = false;
@@ -1129,8 +1135,8 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
     QIOChannel *sioc = NULL;
 
     *info = NULL;
-    result = nbd_start_negotiate(ioc, tlscreds, hostname, &sioc, true, NULL,
-                                 errp);
+    result = nbd_start_negotiate(NULL, ioc, tlscreds, hostname, &sioc, true,
+                                 NULL, errp);
     if (tlscreds && sioc) {
         ioc = sioc;
     }
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 941ba729c2..ed38b55428 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -370,7 +370,7 @@ static void *nbd_client_thread(void *arg)
         goto out;
     }
 
-    ret = nbd_receive_negotiate(QIO_CHANNEL(sioc),
+    ret = nbd_receive_negotiate(NULL, QIO_CHANNEL(sioc),
                                 NULL, NULL, NULL, &info, &local_error);
     if (ret < 0) {
         if (local_error) {
-- 
2.18.0

Re: [Qemu-devel] [PATCH v6 2/7] block/nbd-client: use non-blocking io channel for nbd negotiation

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 739 bytes --]

On 4/11/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
> No reason to use blocking channel for negotiation and we'll benefit in
> further reconnect feature, as qio_channel reads and writes will do
> qemu_coroutine_yield while waiting for io completion.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/nbd.h |  3 ++-
>  block/nbd-client.c  | 16 +++++++---------
>  nbd/client.c        | 16 +++++++++++-----
>  qemu-nbd.c          |  2 +-
>  4 files changed, 21 insertions(+), 16 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@Redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v6 3/7] block/nbd-client: move from quit to state

[Vladimir Sementsov-Ogievskiy]

To implement reconnect we need several states for the client:
CONNECTED, QUIT and two different CONNECTING states. CONNECTING states
will be added in the following patches. This patch implements CONNECTED
and QUIT.

QUIT means, that we should close the connection and fail all current
and further requests (like old quit = true).

CONNECTED means that connection is ok, we can send requests (like old
quit = false).

For receiving loop we use a comparison of the current state with QUIT,
because reconnect will be in the same loop, so it should be looping
until the end.

Opposite, for requests we use a comparison of the current state with
CONNECTED, as we don't want to send requests in future CONNECTING
states.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.h |  7 ++++++-
 block/nbd-client.c | 51 ++++++++++++++++++++++++++++------------------
 2 files changed, 37 insertions(+), 21 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index 09e03013d2..e45b7b0a14 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -23,6 +23,11 @@ typedef struct {
     bool receiving;         /* waiting for connection_co? */
 } NBDClientRequest;
 
+typedef enum NBDClientState {
+    NBD_CLIENT_CONNECTED,
+    NBD_CLIENT_QUIT
+} NBDClientState;
+
 typedef struct NBDClientSession {
     QIOChannelSocket *sioc; /* The master data channel */
     QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
@@ -32,11 +37,11 @@ typedef struct NBDClientSession {
     CoQueue free_sema;
     Coroutine *connection_co;
     int in_flight;
+    NBDClientState state;
 
     NBDClientRequest requests[MAX_NBD_REQUESTS];
     NBDReply reply;
     BlockDriverState *bs;
-    bool quit;
 } NBDClientSession;
 
 NBDClientSession *nbd_get_client_session(BlockDriverState *bs);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index f9cee25865..9f5c86eaa3 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -36,6 +36,12 @@
 #define HANDLE_TO_INDEX(bs, handle) ((handle) ^ (uint64_t)(intptr_t)(bs))
 #define INDEX_TO_HANDLE(bs, index)  ((index)  ^ (uint64_t)(intptr_t)(bs))
 
+/* @ret will be used for reconnect in future */
+static void nbd_channel_error(NBDClientSession *s, int ret)
+{
+    s->state = NBD_CLIENT_QUIT;
+}
+
 static void nbd_recv_coroutines_wake_all(NBDClientSession *s)
 {
     int i;
@@ -75,7 +81,7 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
     int ret = 0;
     Error *local_err = NULL;
 
-    while (!s->quit) {
+    while (s->state != NBD_CLIENT_QUIT) {
         /*
          * The NBD client can only really be considered idle when it has
          * yielded from qio_channel_readv_all_eof(), waiting for data. This is
@@ -93,6 +99,7 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
             error_free(local_err);
         }
         if (ret <= 0) {
+            nbd_channel_error(s, ret ? ret : -EIO);
             break;
         }
 
@@ -106,6 +113,7 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
             !s->requests[i].receiving ||
             (nbd_reply_is_structured(&s->reply) && !s->info.structured_reply))
         {
+            nbd_channel_error(s, -EINVAL);
             break;
         }
 
@@ -124,7 +132,6 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
         qemu_coroutine_yield();
     }
 
-    s->quit = true;
     nbd_recv_coroutines_wake_all(s);
     bdrv_dec_in_flight(s->bs);
 
@@ -137,12 +144,18 @@ static int nbd_co_send_request(BlockDriverState *bs,
                                QEMUIOVector *qiov)
 {
     NBDClientSession *s = nbd_get_client_session(bs);
-    int rc, i;
+    int rc, i = -1;
 
     qemu_co_mutex_lock(&s->send_mutex);
     while (s->in_flight == MAX_NBD_REQUESTS) {
         qemu_co_queue_wait(&s->free_sema, &s->send_mutex);
     }
+
+    if (s->state != NBD_CLIENT_CONNECTED) {
+        rc = -EIO;
+        goto err;
+    }
+
     s->in_flight++;
 
     for (i = 0; i < MAX_NBD_REQUESTS; i++) {
@@ -160,16 +173,12 @@ static int nbd_co_send_request(BlockDriverState *bs,
 
     request->handle = INDEX_TO_HANDLE(s, i);
 
-    if (s->quit) {
-        rc = -EIO;
-        goto err;
-    }
     assert(s->ioc);
 
     if (qiov) {
         qio_channel_set_cork(s->ioc, true);
         rc = nbd_send_request(s->ioc, request);
-        if (rc >= 0 && !s->quit) {
+        if (rc >= 0 && s->state == NBD_CLIENT_CONNECTED) {
             if (qio_channel_writev_all(s->ioc, qiov->iov, qiov->niov,
                                        NULL) < 0) {
                 rc = -EIO;
@@ -184,9 +193,11 @@ static int nbd_co_send_request(BlockDriverState *bs,
 
 err:
     if (rc < 0) {
-        s->quit = true;
-        s->requests[i].coroutine = NULL;
-        s->in_flight--;
+        nbd_channel_error(s, rc);
+        if (i != -1) {
+            s->requests[i].coroutine = NULL;
+            s->in_flight--;
+        }
         qemu_co_queue_next(&s->free_sema);
     }
     qemu_co_mutex_unlock(&s->send_mutex);
@@ -477,7 +488,7 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
     s->requests[i].receiving = true;
     qemu_coroutine_yield();
     s->requests[i].receiving = false;
-    if (s->quit) {
+    if (s->state != NBD_CLIENT_CONNECTED) {
         error_setg(errp, "Connection closed");
         return -EIO;
     }
@@ -560,7 +571,7 @@ static coroutine_fn int nbd_co_receive_one_chunk(
                                           request_ret, qiov, payload, errp);
 
     if (ret < 0) {
-        s->quit = true;
+        nbd_channel_error(s, ret);
     } else {
         /* For assert at loop start in nbd_connection_entry */
         if (reply) {
@@ -626,7 +637,7 @@ static bool nbd_reply_chunk_iter_receive(NBDClientSession *s,
     NBDReply local_reply;
     NBDStructuredReplyChunk *chunk;
     Error *local_err = NULL;
-    if (s->quit) {
+    if (s->state != NBD_CLIENT_CONNECTED) {
         error_setg(&local_err, "Connection closed");
         nbd_iter_channel_error(iter, -EIO, &local_err);
         goto break_loop;
@@ -651,7 +662,7 @@ static bool nbd_reply_chunk_iter_receive(NBDClientSession *s,
     }
 
     /* Do not execute the body of NBD_FOREACH_REPLY_CHUNK for simple reply. */
-    if (nbd_reply_is_simple(reply) || s->quit) {
+    if (nbd_reply_is_simple(reply) || s->state != NBD_CLIENT_CONNECTED) {
         goto break_loop;
     }
 
@@ -723,14 +734,14 @@ static int nbd_co_receive_cmdread_reply(NBDClientSession *s, uint64_t handle,
             ret = nbd_parse_offset_hole_payload(s, &reply.structured, payload,
                                                 offset, qiov, &local_err);
             if (ret < 0) {
-                s->quit = true;
+                nbd_channel_error(s, ret);
                 nbd_iter_channel_error(&iter, ret, &local_err);
             }
             break;
         default:
             if (!nbd_reply_type_is_error(chunk->type)) {
                 /* not allowed reply type */
-                s->quit = true;
+                nbd_channel_error(s, -EINVAL);
                 error_setg(&local_err,
                            "Unexpected reply type: %d (%s) for CMD_READ",
                            chunk->type, nbd_reply_type_lookup(chunk->type));
@@ -768,7 +779,7 @@ static int nbd_co_receive_blockstatus_reply(NBDClientSession *s,
         switch (chunk->type) {
         case NBD_REPLY_TYPE_BLOCK_STATUS:
             if (received) {
-                s->quit = true;
+                nbd_channel_error(s, -EINVAL);
                 error_setg(&local_err, "Several BLOCK_STATUS chunks in reply");
                 nbd_iter_channel_error(&iter, -EINVAL, &local_err);
             }
@@ -778,13 +789,13 @@ static int nbd_co_receive_blockstatus_reply(NBDClientSession *s,
                                                 payload, length, extent,
                                                 &local_err);
             if (ret < 0) {
-                s->quit = true;
+                nbd_channel_error(s, ret);
                 nbd_iter_channel_error(&iter, ret, &local_err);
             }
             break;
         default:
             if (!nbd_reply_type_is_error(chunk->type)) {
-                s->quit = true;
+                nbd_channel_error(s, -EINVAL);
                 error_setg(&local_err,
                            "Unexpected reply type: %d (%s) "
                            "for CMD_BLOCK_STATUS",
-- 
2.18.0

[Qemu-devel] [PATCH v6 3/7] block/nbd-client: move from quit to state

[Vladimir Sementsov-Ogievskiy]

To implement reconnect we need several states for the client:
CONNECTED, QUIT and two different CONNECTING states. CONNECTING states
will be added in the following patches. This patch implements CONNECTED
and QUIT.

QUIT means, that we should close the connection and fail all current
and further requests (like old quit = true).

CONNECTED means that connection is ok, we can send requests (like old
quit = false).

For receiving loop we use a comparison of the current state with QUIT,
because reconnect will be in the same loop, so it should be looping
until the end.

Opposite, for requests we use a comparison of the current state with
CONNECTED, as we don't want to send requests in future CONNECTING
states.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.h |  7 ++++++-
 block/nbd-client.c | 51 ++++++++++++++++++++++++++++------------------
 2 files changed, 37 insertions(+), 21 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index 09e03013d2..e45b7b0a14 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -23,6 +23,11 @@ typedef struct {
     bool receiving;         /* waiting for connection_co? */
 } NBDClientRequest;
 
+typedef enum NBDClientState {
+    NBD_CLIENT_CONNECTED,
+    NBD_CLIENT_QUIT
+} NBDClientState;
+
 typedef struct NBDClientSession {
     QIOChannelSocket *sioc; /* The master data channel */
     QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
@@ -32,11 +37,11 @@ typedef struct NBDClientSession {
     CoQueue free_sema;
     Coroutine *connection_co;
     int in_flight;
+    NBDClientState state;
 
     NBDClientRequest requests[MAX_NBD_REQUESTS];
     NBDReply reply;
     BlockDriverState *bs;
-    bool quit;
 } NBDClientSession;
 
 NBDClientSession *nbd_get_client_session(BlockDriverState *bs);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index f9cee25865..9f5c86eaa3 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -36,6 +36,12 @@
 #define HANDLE_TO_INDEX(bs, handle) ((handle) ^ (uint64_t)(intptr_t)(bs))
 #define INDEX_TO_HANDLE(bs, index)  ((index)  ^ (uint64_t)(intptr_t)(bs))
 
+/* @ret will be used for reconnect in future */
+static void nbd_channel_error(NBDClientSession *s, int ret)
+{
+    s->state = NBD_CLIENT_QUIT;
+}
+
 static void nbd_recv_coroutines_wake_all(NBDClientSession *s)
 {
     int i;
@@ -75,7 +81,7 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
     int ret = 0;
     Error *local_err = NULL;
 
-    while (!s->quit) {
+    while (s->state != NBD_CLIENT_QUIT) {
         /*
          * The NBD client can only really be considered idle when it has
          * yielded from qio_channel_readv_all_eof(), waiting for data. This is
@@ -93,6 +99,7 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
             error_free(local_err);
         }
         if (ret <= 0) {
+            nbd_channel_error(s, ret ? ret : -EIO);
             break;
         }
 
@@ -106,6 +113,7 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
             !s->requests[i].receiving ||
             (nbd_reply_is_structured(&s->reply) && !s->info.structured_reply))
         {
+            nbd_channel_error(s, -EINVAL);
             break;
         }
 
@@ -124,7 +132,6 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
         qemu_coroutine_yield();
     }
 
-    s->quit = true;
     nbd_recv_coroutines_wake_all(s);
     bdrv_dec_in_flight(s->bs);
 
@@ -137,12 +144,18 @@ static int nbd_co_send_request(BlockDriverState *bs,
                                QEMUIOVector *qiov)
 {
     NBDClientSession *s = nbd_get_client_session(bs);
-    int rc, i;
+    int rc, i = -1;
 
     qemu_co_mutex_lock(&s->send_mutex);
     while (s->in_flight == MAX_NBD_REQUESTS) {
         qemu_co_queue_wait(&s->free_sema, &s->send_mutex);
     }
+
+    if (s->state != NBD_CLIENT_CONNECTED) {
+        rc = -EIO;
+        goto err;
+    }
+
     s->in_flight++;
 
     for (i = 0; i < MAX_NBD_REQUESTS; i++) {
@@ -160,16 +173,12 @@ static int nbd_co_send_request(BlockDriverState *bs,
 
     request->handle = INDEX_TO_HANDLE(s, i);
 
-    if (s->quit) {
-        rc = -EIO;
-        goto err;
-    }
     assert(s->ioc);
 
     if (qiov) {
         qio_channel_set_cork(s->ioc, true);
         rc = nbd_send_request(s->ioc, request);
-        if (rc >= 0 && !s->quit) {
+        if (rc >= 0 && s->state == NBD_CLIENT_CONNECTED) {
             if (qio_channel_writev_all(s->ioc, qiov->iov, qiov->niov,
                                        NULL) < 0) {
                 rc = -EIO;
@@ -184,9 +193,11 @@ static int nbd_co_send_request(BlockDriverState *bs,
 
 err:
     if (rc < 0) {
-        s->quit = true;
-        s->requests[i].coroutine = NULL;
-        s->in_flight--;
+        nbd_channel_error(s, rc);
+        if (i != -1) {
+            s->requests[i].coroutine = NULL;
+            s->in_flight--;
+        }
         qemu_co_queue_next(&s->free_sema);
     }
     qemu_co_mutex_unlock(&s->send_mutex);
@@ -477,7 +488,7 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
     s->requests[i].receiving = true;
     qemu_coroutine_yield();
     s->requests[i].receiving = false;
-    if (s->quit) {
+    if (s->state != NBD_CLIENT_CONNECTED) {
         error_setg(errp, "Connection closed");
         return -EIO;
     }
@@ -560,7 +571,7 @@ static coroutine_fn int nbd_co_receive_one_chunk(
                                           request_ret, qiov, payload, errp);
 
     if (ret < 0) {
-        s->quit = true;
+        nbd_channel_error(s, ret);
     } else {
         /* For assert at loop start in nbd_connection_entry */
         if (reply) {
@@ -626,7 +637,7 @@ static bool nbd_reply_chunk_iter_receive(NBDClientSession *s,
     NBDReply local_reply;
     NBDStructuredReplyChunk *chunk;
     Error *local_err = NULL;
-    if (s->quit) {
+    if (s->state != NBD_CLIENT_CONNECTED) {
         error_setg(&local_err, "Connection closed");
         nbd_iter_channel_error(iter, -EIO, &local_err);
         goto break_loop;
@@ -651,7 +662,7 @@ static bool nbd_reply_chunk_iter_receive(NBDClientSession *s,
     }
 
     /* Do not execute the body of NBD_FOREACH_REPLY_CHUNK for simple reply. */
-    if (nbd_reply_is_simple(reply) || s->quit) {
+    if (nbd_reply_is_simple(reply) || s->state != NBD_CLIENT_CONNECTED) {
         goto break_loop;
     }
 
@@ -723,14 +734,14 @@ static int nbd_co_receive_cmdread_reply(NBDClientSession *s, uint64_t handle,
             ret = nbd_parse_offset_hole_payload(s, &reply.structured, payload,
                                                 offset, qiov, &local_err);
             if (ret < 0) {
-                s->quit = true;
+                nbd_channel_error(s, ret);
                 nbd_iter_channel_error(&iter, ret, &local_err);
             }
             break;
         default:
             if (!nbd_reply_type_is_error(chunk->type)) {
                 /* not allowed reply type */
-                s->quit = true;
+                nbd_channel_error(s, -EINVAL);
                 error_setg(&local_err,
                            "Unexpected reply type: %d (%s) for CMD_READ",
                            chunk->type, nbd_reply_type_lookup(chunk->type));
@@ -768,7 +779,7 @@ static int nbd_co_receive_blockstatus_reply(NBDClientSession *s,
         switch (chunk->type) {
         case NBD_REPLY_TYPE_BLOCK_STATUS:
             if (received) {
-                s->quit = true;
+                nbd_channel_error(s, -EINVAL);
                 error_setg(&local_err, "Several BLOCK_STATUS chunks in reply");
                 nbd_iter_channel_error(&iter, -EINVAL, &local_err);
             }
@@ -778,13 +789,13 @@ static int nbd_co_receive_blockstatus_reply(NBDClientSession *s,
                                                 payload, length, extent,
                                                 &local_err);
             if (ret < 0) {
-                s->quit = true;
+                nbd_channel_error(s, ret);
                 nbd_iter_channel_error(&iter, ret, &local_err);
             }
             break;
         default:
             if (!nbd_reply_type_is_error(chunk->type)) {
-                s->quit = true;
+                nbd_channel_error(s, -EINVAL);
                 error_setg(&local_err,
                            "Unexpected reply type: %d (%s) "
                            "for CMD_BLOCK_STATUS",
-- 
2.18.0

Re: [Qemu-devel] [PATCH v6 3/7] block/nbd-client: move from quit to state

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1261 bytes --]

On 4/11/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
> To implement reconnect we need several states for the client:
> CONNECTED, QUIT and two different CONNECTING states. CONNECTING states
> will be added in the following patches. This patch implements CONNECTED
> and QUIT.
> 
> QUIT means, that we should close the connection and fail all current
> and further requests (like old quit = true).
> 
> CONNECTED means that connection is ok, we can send requests (like old
> quit = false).
> 
> For receiving loop we use a comparison of the current state with QUIT,
> because reconnect will be in the same loop, so it should be looping
> until the end.
> 
> Opposite, for requests we use a comparison of the current state with
> CONNECTED, as we don't want to send requests in future CONNECTING
> states.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/nbd-client.h |  7 ++++++-
>  block/nbd-client.c | 51 ++++++++++++++++++++++++++++------------------
>  2 files changed, 37 insertions(+), 21 deletions(-)
> 

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v6 4/7] block/nbd: add cmdline and qapi parameter reconnect-delay

[Vladimir Sementsov-Ogievskiy]

Reconnect will be implemented in the following commit, so for now,
in semantics below, disconnect itself is a "serious error".

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json | 12 +++++++++++-
 block/nbd-client.h   |  1 +
 block/nbd-client.c   |  1 +
 block/nbd.c          | 16 +++++++++++++++-
 4 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7ccbfff9d0..815258bd89 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3798,13 +3798,23 @@
 #                  traditional "base:allocation" block status (see
 #                  NBD_OPT_LIST_META_CONTEXT in the NBD protocol) (since 3.0)
 #
+# @reconnect-delay: Reconnect delay. On unexpected disconnect, nbd client tries
+#                   to connect again, until success or serious error. During
+#                   first @reconnect-delay seconds of reconnecting loop all
+#                   requests are paused and have a chance to rerun, if
+#                   successful connect occurs during this time. After
+#                   @reconnect-delay seconds all delayed requests are failed
+#                   and all following requests will be failed too (until
+#                   successful reconnect). Default 0 (Since 4.1)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsNbd',
   'data': { 'server': 'SocketAddress',
             '*export': 'str',
             '*tls-creds': 'str',
-            '*x-dirty-bitmap': 'str' } }
+            '*x-dirty-bitmap': 'str',
+            '*reconnect-delay': 'uint32' } }
 
 ##
 # @BlockdevOptionsRaw:
diff --git a/block/nbd-client.h b/block/nbd-client.h
index e45b7b0a14..91a6b32bdd 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -52,6 +52,7 @@ int nbd_client_init(BlockDriverState *bs,
                     QCryptoTLSCreds *tlscreds,
                     const char *hostname,
                     const char *x_dirty_bitmap,
+                    uint32_t reconnect_delay,
                     Error **errp);
 void nbd_client_close(BlockDriverState *bs);
 
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 9f5c86eaa3..1359aff162 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -1218,6 +1218,7 @@ int nbd_client_init(BlockDriverState *bs,
                     QCryptoTLSCreds *tlscreds,
                     const char *hostname,
                     const char *x_dirty_bitmap,
+                    uint32_t reconnect_delay,
                     Error **errp)
 {
     int ret;
diff --git a/block/nbd.c b/block/nbd.c
index 208be59602..cffb2aee89 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -360,6 +360,18 @@ static QemuOptsList nbd_runtime_opts = {
             .help = "experimental: expose named dirty bitmap in place of "
                     "block status",
         },
+        {
+            .name = "reconnect-delay",
+            .type = QEMU_OPT_NUMBER,
+            .help = "Reconnect delay. On unexpected disconnect, nbd client "
+                    "tries to connect again, until success or serious error. "
+                    "During first @reconnect-delay seconds of reconnecting "
+                    "loop all requests are paused and have a chance to rerun, "
+                    "if successful connect occurs during this time. After"
+                    "@reconnect-delay seconds all delayed requests are failed"
+                    "and all following requests will be failed too (until"
+                    "successful reconnect). Default 0",
+        },
         { /* end of list */ }
     },
 };
@@ -411,7 +423,9 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
 
     /* NBD handshake */
     ret = nbd_client_init(bs, s->saddr, s->export, tlscreds, hostname,
-                          qemu_opt_get(opts, "x-dirty-bitmap"), errp);
+                          qemu_opt_get(opts, "x-dirty-bitmap"),
+                          qemu_opt_get_number(opts, "reconnect-delay", 0),
+                          errp);
 
  error:
     if (tlscreds) {
-- 
2.18.0

[Qemu-devel] [PATCH v6 4/7] block/nbd: add cmdline and qapi parameter reconnect-delay

[Vladimir Sementsov-Ogievskiy]

Reconnect will be implemented in the following commit, so for now,
in semantics below, disconnect itself is a "serious error".

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json | 12 +++++++++++-
 block/nbd-client.h   |  1 +
 block/nbd-client.c   |  1 +
 block/nbd.c          | 16 +++++++++++++++-
 4 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7ccbfff9d0..815258bd89 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3798,13 +3798,23 @@
 #                  traditional "base:allocation" block status (see
 #                  NBD_OPT_LIST_META_CONTEXT in the NBD protocol) (since 3.0)
 #
+# @reconnect-delay: Reconnect delay. On unexpected disconnect, nbd client tries
+#                   to connect again, until success or serious error. During
+#                   first @reconnect-delay seconds of reconnecting loop all
+#                   requests are paused and have a chance to rerun, if
+#                   successful connect occurs during this time. After
+#                   @reconnect-delay seconds all delayed requests are failed
+#                   and all following requests will be failed too (until
+#                   successful reconnect). Default 0 (Since 4.1)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsNbd',
   'data': { 'server': 'SocketAddress',
             '*export': 'str',
             '*tls-creds': 'str',
-            '*x-dirty-bitmap': 'str' } }
+            '*x-dirty-bitmap': 'str',
+            '*reconnect-delay': 'uint32' } }
 
 ##
 # @BlockdevOptionsRaw:
diff --git a/block/nbd-client.h b/block/nbd-client.h
index e45b7b0a14..91a6b32bdd 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -52,6 +52,7 @@ int nbd_client_init(BlockDriverState *bs,
                     QCryptoTLSCreds *tlscreds,
                     const char *hostname,
                     const char *x_dirty_bitmap,
+                    uint32_t reconnect_delay,
                     Error **errp);
 void nbd_client_close(BlockDriverState *bs);
 
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 9f5c86eaa3..1359aff162 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -1218,6 +1218,7 @@ int nbd_client_init(BlockDriverState *bs,
                     QCryptoTLSCreds *tlscreds,
                     const char *hostname,
                     const char *x_dirty_bitmap,
+                    uint32_t reconnect_delay,
                     Error **errp)
 {
     int ret;
diff --git a/block/nbd.c b/block/nbd.c
index 208be59602..cffb2aee89 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -360,6 +360,18 @@ static QemuOptsList nbd_runtime_opts = {
             .help = "experimental: expose named dirty bitmap in place of "
                     "block status",
         },
+        {
+            .name = "reconnect-delay",
+            .type = QEMU_OPT_NUMBER,
+            .help = "Reconnect delay. On unexpected disconnect, nbd client "
+                    "tries to connect again, until success or serious error. "
+                    "During first @reconnect-delay seconds of reconnecting "
+                    "loop all requests are paused and have a chance to rerun, "
+                    "if successful connect occurs during this time. After"
+                    "@reconnect-delay seconds all delayed requests are failed"
+                    "and all following requests will be failed too (until"
+                    "successful reconnect). Default 0",
+        },
         { /* end of list */ }
     },
 };
@@ -411,7 +423,9 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
 
     /* NBD handshake */
     ret = nbd_client_init(bs, s->saddr, s->export, tlscreds, hostname,
-                          qemu_opt_get(opts, "x-dirty-bitmap"), errp);
+                          qemu_opt_get(opts, "x-dirty-bitmap"),
+                          qemu_opt_get_number(opts, "reconnect-delay", 0),
+                          errp);
 
  error:
     if (tlscreds) {
-- 
2.18.0

Re: [Qemu-devel] [PATCH v6 4/7] block/nbd: add cmdline and qapi parameter reconnect-delay

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 3074 bytes --]

On 4/11/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
> Reconnect will be implemented in the following commit, so for now,
> in semantics below, disconnect itself is a "serious error".
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block-core.json | 12 +++++++++++-
>  block/nbd-client.h   |  1 +
>  block/nbd-client.c   |  1 +
>  block/nbd.c          | 16 +++++++++++++++-
>  4 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7ccbfff9d0..815258bd89 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3798,13 +3798,23 @@
>  #                  traditional "base:allocation" block status (see
>  #                  NBD_OPT_LIST_META_CONTEXT in the NBD protocol) (since 3.0)
>  #
> +# @reconnect-delay: Reconnect delay. On unexpected disconnect, nbd client tries
> +#                   to connect again, until success or serious error. During
> +#                   first @reconnect-delay seconds of reconnecting loop all
> +#                   requests are paused and have a chance to rerun, if
> +#                   successful connect occurs during this time. After
> +#                   @reconnect-delay seconds all delayed requests are failed
> +#                   and all following requests will be failed too (until
> +#                   successful reconnect). Default 0 (Since 4.1)

Maybe:

On an unexpected disconnect, the nbd client tries to connect again until
succeeding or encountering a serious error.  During the first
@reconnect-delay seconds, all requests are paused and will be rerun on a
successful reconnect. After that time, any delayed requests and all
future requests before a successful reconnect will immediately fail.


> +++ b/block/nbd.c
> @@ -360,6 +360,18 @@ static QemuOptsList nbd_runtime_opts = {
>              .help = "experimental: expose named dirty bitmap in place of "
>                      "block status",
>          },
> +        {
> +            .name = "reconnect-delay",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "Reconnect delay. On unexpected disconnect, nbd client "
> +                    "tries to connect again, until success or serious error. "
> +                    "During first @reconnect-delay seconds of reconnecting "
> +                    "loop all requests are paused and have a chance to rerun, "
> +                    "if successful connect occurs during this time. After"
> +                    "@reconnect-delay seconds all delayed requests are failed"
> +                    "and all following requests will be failed too (until"
> +                    "successful reconnect). Default 0",

And of course copy any changes to the QMP text to here.

I can touch up grammar, so if there's no other reason to respin,

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v6 5/7] qemu-coroutine-sleep: introduce qemu_co_sleep_wake

[Vladimir Sementsov-Ogievskiy]

Introduce a function to gracefully wake-up a coroutine, sleeping in
qemu_co_sleep_ns() sleep.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/qemu/coroutine.h    |  6 ++++++
 util/qemu-coroutine-sleep.c | 20 ++++++++++++++++----
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index 9801e7f5a4..ec765c26f0 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -278,6 +278,12 @@ void qemu_co_rwlock_unlock(CoRwlock *lock);
  */
 void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns);
 
+/*
+ * Wake a coroutine if it is sleeping by qemu_co_sleep_ns. Timer will be
+ * deleted.
+ */
+void qemu_co_sleep_wake(Coroutine *co);
+
 /**
  * Yield until a file descriptor becomes readable
  *
diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c
index 4bfdd30cbf..bcc6afca3e 100644
--- a/util/qemu-coroutine-sleep.c
+++ b/util/qemu-coroutine-sleep.c
@@ -17,13 +17,24 @@
 #include "qemu/timer.h"
 #include "block/aio.h"
 
+const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns";
+
+void qemu_co_sleep_wake(Coroutine *co)
+{
+    /* Write of schedule protected by barrier write in aio_co_schedule */
+    const char *scheduled = atomic_cmpxchg(&co->scheduled,
+                                           qemu_co_sleep_ns__scheduled, NULL);
+
+    if (scheduled == qemu_co_sleep_ns__scheduled) {
+        aio_co_wake(co);
+    }
+}
+
 static void co_sleep_cb(void *opaque)
 {
     Coroutine *co = opaque;
 
-    /* Write of schedule protected by barrier write in aio_co_schedule */
-    atomic_set(&co->scheduled, NULL);
-    aio_co_wake(co);
+    qemu_co_sleep_wake(co);
 }
 
 void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
@@ -32,7 +43,8 @@ void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
     QEMUTimer *ts;
     Coroutine *co = qemu_coroutine_self();
 
-    const char *scheduled = atomic_cmpxchg(&co->scheduled, NULL, __func__);
+    const char *scheduled = atomic_cmpxchg(&co->scheduled, NULL,
+                                           qemu_co_sleep_ns__scheduled);
     if (scheduled) {
         fprintf(stderr,
                 "%s: Co-routine was already scheduled in '%s'\n",
-- 
2.18.0

[Qemu-devel] [PATCH v6 5/7] qemu-coroutine-sleep: introduce qemu_co_sleep_wake

[Vladimir Sementsov-Ogievskiy]

Introduce a function to gracefully wake-up a coroutine, sleeping in
qemu_co_sleep_ns() sleep.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/qemu/coroutine.h    |  6 ++++++
 util/qemu-coroutine-sleep.c | 20 ++++++++++++++++----
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index 9801e7f5a4..ec765c26f0 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -278,6 +278,12 @@ void qemu_co_rwlock_unlock(CoRwlock *lock);
  */
 void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns);
 
+/*
+ * Wake a coroutine if it is sleeping by qemu_co_sleep_ns. Timer will be
+ * deleted.
+ */
+void qemu_co_sleep_wake(Coroutine *co);
+
 /**
  * Yield until a file descriptor becomes readable
  *
diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c
index 4bfdd30cbf..bcc6afca3e 100644
--- a/util/qemu-coroutine-sleep.c
+++ b/util/qemu-coroutine-sleep.c
@@ -17,13 +17,24 @@
 #include "qemu/timer.h"
 #include "block/aio.h"
 
+const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns";
+
+void qemu_co_sleep_wake(Coroutine *co)
+{
+    /* Write of schedule protected by barrier write in aio_co_schedule */
+    const char *scheduled = atomic_cmpxchg(&co->scheduled,
+                                           qemu_co_sleep_ns__scheduled, NULL);
+
+    if (scheduled == qemu_co_sleep_ns__scheduled) {
+        aio_co_wake(co);
+    }
+}
+
 static void co_sleep_cb(void *opaque)
 {
     Coroutine *co = opaque;
 
-    /* Write of schedule protected by barrier write in aio_co_schedule */
-    atomic_set(&co->scheduled, NULL);
-    aio_co_wake(co);
+    qemu_co_sleep_wake(co);
 }
 
 void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
@@ -32,7 +43,8 @@ void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
     QEMUTimer *ts;
     Coroutine *co = qemu_coroutine_self();
 
-    const char *scheduled = atomic_cmpxchg(&co->scheduled, NULL, __func__);
+    const char *scheduled = atomic_cmpxchg(&co->scheduled, NULL,
+                                           qemu_co_sleep_ns__scheduled);
     if (scheduled) {
         fprintf(stderr,
                 "%s: Co-routine was already scheduled in '%s'\n",
-- 
2.18.0

Re: [Qemu-devel] [PATCH v6 5/7] qemu-coroutine-sleep: introduce qemu_co_sleep_wake

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 2985 bytes --]

On 4/11/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
> Introduce a function to gracefully wake-up a coroutine, sleeping in
> qemu_co_sleep_ns() sleep.

Maybe:

Introduce a function to gracefully short-circuit the remainder of the
delay for a coroutine sleeping in qemu_co_sleep_ns().

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/qemu/coroutine.h    |  6 ++++++
>  util/qemu-coroutine-sleep.c | 20 ++++++++++++++++----
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> index 9801e7f5a4..ec765c26f0 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -278,6 +278,12 @@ void qemu_co_rwlock_unlock(CoRwlock *lock);
>   */
>  void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns);
>  
> +/*
> + * Wake a coroutine if it is sleeping by qemu_co_sleep_ns. Timer will be
> + * deleted.

Maybe:

Wake a coroutine if it is sleeping in qemu_co_sleep_ns, and delete the
timer.

> +++ b/util/qemu-coroutine-sleep.c
> @@ -17,13 +17,24 @@
>  #include "qemu/timer.h"
>  #include "block/aio.h"
>  
> +const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns";
> +
> +void qemu_co_sleep_wake(Coroutine *co)
> +{
> +    /* Write of schedule protected by barrier write in aio_co_schedule */
> +    const char *scheduled = atomic_cmpxchg(&co->scheduled,
> +                                           qemu_co_sleep_ns__scheduled, NULL);
> +
> +    if (scheduled == qemu_co_sleep_ns__scheduled) {
> +        aio_co_wake(co);
> +    }
> +}
> +
>  static void co_sleep_cb(void *opaque)
>  {
>      Coroutine *co = opaque;
>  
> -    /* Write of schedule protected by barrier write in aio_co_schedule */
> -    atomic_set(&co->scheduled, NULL);
> -    aio_co_wake(co);
> +    qemu_co_sleep_wake(co);
>  }
>  
>  void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
> @@ -32,7 +43,8 @@ void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
>      QEMUTimer *ts;
>      Coroutine *co = qemu_coroutine_self();
>  
> -    const char *scheduled = atomic_cmpxchg(&co->scheduled, NULL, __func__);
> +    const char *scheduled = atomic_cmpxchg(&co->scheduled, NULL,
> +                                           qemu_co_sleep_ns__scheduled);
>      if (scheduled) {
>          fprintf(stderr,
>                  "%s: Co-routine was already scheduled in '%s'\n",
> 

Here, I'd rather get an additional review from anyone more familiar with
coroutine sleeps.  I'm guessing that your intent is to request a maximum
timeout for a given operation to complete in, but to leave the sleep
loop early if the operation completes earlier.  I don't know if any
existing coroutine code can be used to express that same idea.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v6 5/7] qemu-coroutine-sleep: introduce qemu_co_sleep_wake

[Kevin Wolf]

Am 11.04.2019 um 19:27 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Introduce a function to gracefully wake-up a coroutine, sleeping in
> qemu_co_sleep_ns() sleep.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

You can simply reenter the coroutine while it has yielded in
qemu_co_sleep_ns(). This is supported.

I think what you add here is just the condition that you wake up the
coroutine only if it's currently sleeping, but not when it has yielded
for other reasons. This suggests that you're trying to reenter a
coroutine of which you don't know where exactly in its code it currently
is. This is wrong.

Just knowing that it's sleeping doesn't tell you where the coroutine is.
It could have called a function that sleeps internally and must not be
woken up early. If you reenter a coroutine, you always must know the
exact point where it yielded (or in exceptional cases, the exact points
(plural)). Just reentering because it sleeps will wake it up in
unexpected places, generally speaking.

So I don't think this function is a good idea. It's too easy to misuse,
and if you don't misuse it, you can directly call aio_co_wake().

Kevin

Re: [Qemu-devel] [PATCH v6 5/7] qemu-coroutine-sleep: introduce qemu_co_sleep_wake

[Vladimir Sementsov-Ogievskiy]

07.06.2019 10:57, Kevin Wolf wrote:
> Am 11.04.2019 um 19:27 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Introduce a function to gracefully wake-up a coroutine, sleeping in
>> qemu_co_sleep_ns() sleep.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> You can simply reenter the coroutine while it has yielded in
> qemu_co_sleep_ns(). This is supported.

No it doesn't. qemu_aio_coroutine_enter checks for scheduled field, and aborts
if it is set.

If I just use aio_co_enter instead of my new function, I get into

#1  0x00007f5d2514f8f8 in __GI_abort () at abort.c:90
#2  0x000055e9c8145278 in qemu_aio_coroutine_enter (ctx=0x55e9c9b12300, co=0x55e9c9b23cb0)
     at util/qemu-coroutine.c:132
#3  0x000055e9c8124f6d in aio_co_enter (ctx=0x55e9c9b12300, co=0x55e9c9b23cb0) at util/async.c:494
#4  0x000055e9c8124eb1 in aio_co_wake (co=0x55e9c9b23cb0) at util/async.c:478
#5  0x000055e9c808d5d4 in nbd_teardown_connection (bs=0x55e9c9b1bc50) at block/nbd-client.c:88
#6  0x000055e9c8090673 in nbd_client_close (bs=0x55e9c9b1bc50) at block/nbd-client.c:1289
#7  0x000055e9c808ca3f in nbd_close (bs=0x55e9c9b1bc50) at block/nbd.c:486
#8  0x000055e9c8006cd6 in bdrv_close (bs=0x55e9c9b1bc50) at block.c:3841

(gdb) fr 2
#2  0x000055e9c8145278 in qemu_aio_coroutine_enter (ctx=0x55e9c9b12300, co=0x55e9c9b23cb0)
     at util/qemu-coroutine.c:132
132                 abort();
(gdb) list
127              * been deleted */
128             if (scheduled) {
129                 fprintf(stderr,
130                         "%s: Co-routine was already scheduled in '%s'\n",
131                         __func__, scheduled);
132                 abort();
133             }
134
135             if (to->caller) {
136                 fprintf(stderr, "Co-routine re-entered recursively\n");
(gdb) p scheduled
$1 = 0x55e9c818e990 "qemu_co_sleep_ns"


> 
> I think what you add here is just the condition that you wake up the
> coroutine only if it's currently sleeping, but not when it has yielded
> for other reasons. This suggests that you're trying to reenter a
> coroutine of which you don't know where exactly in its code it currently
> is. This is wrong.
> 
> Just knowing that it's sleeping doesn't tell you where the coroutine is.
> It could have called a function that sleeps internally and must not be
> woken up early. If you reenter a coroutine, you always must know the
> exact point where it yielded (or in exceptional cases, the exact points
> (plural)). Just reentering because it sleeps will wake it up in
> unexpected places, generally speaking.
> 
> So I don't think this function is a good idea. It's too easy to misuse,
> and if you don't misuse it, you can directly call aio_co_wake().
> 
> Kevin
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v6 5/7] qemu-coroutine-sleep: introduce qemu_co_sleep_wake

[Kevin Wolf]

Am 07.06.2019 um 13:18 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 07.06.2019 10:57, Kevin Wolf wrote:
> > Am 11.04.2019 um 19:27 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> Introduce a function to gracefully wake-up a coroutine, sleeping in
> >> qemu_co_sleep_ns() sleep.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > 
> > You can simply reenter the coroutine while it has yielded in
> > qemu_co_sleep_ns(). This is supported.
> 
> No it doesn't. qemu_aio_coroutine_enter checks for scheduled field,
> and aborts if it is set.

Ah, yes, it has been broken since commit

I actually tried to fix it once, but it turned out more complicated and
I think we found a different solution for the problem at hand:

    Subject: [PATCH for-2.11 0/4] Fix qemu-iotests failures
    Message-Id: <20171128154350.21504-1-kwolf@redhat.com>

In this case, I guess your approach with a new function to interrupt
qemu_co_sleep_ns() is okay.

Do we need to timer_del() when taking the shortcut? We don't necessarily
reenter the coroutine immediately, but might only be scheduling it. In
this case, the timer could fire before qemu_co_sleep_ns() has run and
schedule the coroutine a second time (ignoring co->scheduled again -
maybe we should actually not do that in the timer callback path, but
instead let it run into the assertion because it would be a bug for the
timer callback to end up in this situation).

Kevin

Re: [Qemu-devel] [PATCH v6 5/7] qemu-coroutine-sleep: introduce qemu_co_sleep_wake

[Vladimir Sementsov-Ogievskiy]

07.06.2019 16:02, Kevin Wolf wrote:
> Am 07.06.2019 um 13:18 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 07.06.2019 10:57, Kevin Wolf wrote:
>>> Am 11.04.2019 um 19:27 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> Introduce a function to gracefully wake-up a coroutine, sleeping in
>>>> qemu_co_sleep_ns() sleep.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>> You can simply reenter the coroutine while it has yielded in
>>> qemu_co_sleep_ns(). This is supported.
>>
>> No it doesn't. qemu_aio_coroutine_enter checks for scheduled field,
>> and aborts if it is set.
> 
> Ah, yes, it has been broken since commit
> 
> I actually tried to fix it once, but it turned out more complicated and
> I think we found a different solution for the problem at hand:
> 
>      Subject: [PATCH for-2.11 0/4] Fix qemu-iotests failures
>      Message-Id: <20171128154350.21504-1-kwolf@redhat.com>
> 
> In this case, I guess your approach with a new function to interrupt
> qemu_co_sleep_ns() is okay.
> 
> Do we need to timer_del() when taking the shortcut? We don't necessarily
> reenter the coroutine immediately, but might only be scheduling it. In
> this case, the timer could fire before qemu_co_sleep_ns() has run and
> schedule the coroutine a second time (ignoring co->scheduled again -
> maybe we should actually not do that in the timer callback path, but
> instead let it run into the assertion because it would be a bug for the
> timer callback to end up in this situation).
> 

Ok, thanks, will try to improve it


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v6 5/7] qemu-coroutine-sleep: introduce qemu_co_sleep_wake

[Vladimir Sementsov-Ogievskiy]

07.06.2019 16:02, Kevin Wolf wrote:
> Am 07.06.2019 um 13:18 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 07.06.2019 10:57, Kevin Wolf wrote:
>>> Am 11.04.2019 um 19:27 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> Introduce a function to gracefully wake-up a coroutine, sleeping in
>>>> qemu_co_sleep_ns() sleep.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>> You can simply reenter the coroutine while it has yielded in
>>> qemu_co_sleep_ns(). This is supported.
>>
>> No it doesn't. qemu_aio_coroutine_enter checks for scheduled field,
>> and aborts if it is set.
> 
> Ah, yes, it has been broken since commit
> 
> I actually tried to fix it once, but it turned out more complicated and
> I think we found a different solution for the problem at hand:
> 
>      Subject: [PATCH for-2.11 0/4] Fix qemu-iotests failures
>      Message-Id: <20171128154350.21504-1-kwolf@redhat.com>
> 
> In this case, I guess your approach with a new function to interrupt
> qemu_co_sleep_ns() is okay.
> 
> Do we need to timer_del() when taking the shortcut? We don't necessarily
> reenter the coroutine immediately, but might only be scheduling it. In
> this case, the timer could fire before qemu_co_sleep_ns() has run and
> schedule the coroutine a second time

No it will not, as we do cmpxchg, scheduled to NULL, so second call will do
nothing..

But it seems unsafe, as even coroutine pointer may be stale when we call
qemu_co_sleep_wake second time. So, we possibly should remove timer, but ..

  (ignoring co->scheduled again -
> maybe we should actually not do that in the timer callback path, but
> instead let it run into the assertion because it would be a bug for the
> timer callback to end up in this situation).
> 
> Kevin
> 

Interesting, could there be a race condition, when we call qemu_co_sleep_wake,
but co_sleep_cb already scheduled in some queue and will run soon? Then removing
the timer will not help.


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v6 5/7] qemu-coroutine-sleep: introduce qemu_co_sleep_wake

[Vladimir Sementsov-Ogievskiy]

07.06.2019 18:52, Vladimir Sementsov-Ogievskiy wrote:
> 07.06.2019 16:02, Kevin Wolf wrote:
>> Am 07.06.2019 um 13:18 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>> 07.06.2019 10:57, Kevin Wolf wrote:
>>>> Am 11.04.2019 um 19:27 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>> Introduce a function to gracefully wake-up a coroutine, sleeping in
>>>>> qemu_co_sleep_ns() sleep.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>
>>>> You can simply reenter the coroutine while it has yielded in
>>>> qemu_co_sleep_ns(). This is supported.
>>>
>>> No it doesn't. qemu_aio_coroutine_enter checks for scheduled field,
>>> and aborts if it is set.
>>
>> Ah, yes, it has been broken since commit
>>
>> I actually tried to fix it once, but it turned out more complicated and
>> I think we found a different solution for the problem at hand:
>>
>>      Subject: [PATCH for-2.11 0/4] Fix qemu-iotests failures
>>      Message-Id: <20171128154350.21504-1-kwolf@redhat.com>
>>
>> In this case, I guess your approach with a new function to interrupt
>> qemu_co_sleep_ns() is okay.
>>
>> Do we need to timer_del() when taking the shortcut? We don't necessarily
>> reenter the coroutine immediately, but might only be scheduling it. In
>> this case, the timer could fire before qemu_co_sleep_ns() has run and
>> schedule the coroutine a second time
> 
> No it will not, as we do cmpxchg, scheduled to NULL, so second call will do
> nothing..
> 
> But it seems unsafe, as even coroutine pointer may be stale when we call
> qemu_co_sleep_wake second time. So, we possibly should remove timer, but ..
> 
>   (ignoring co->scheduled again -
>> maybe we should actually not do that in the timer callback path, but
>> instead let it run into the assertion because it would be a bug for the
>> timer callback to end up in this situation).
>>
>> Kevin
>>
> 
> Interesting, could there be a race condition, when we call qemu_co_sleep_wake,
> but co_sleep_cb already scheduled in some queue and will run soon? Then removing
> the timer will not help.
> 
> 

Hmm, it's commented that timer_del is thread-safe..

Hmm, so, if anyway want to return Timer pointer from qemu_co_sleep_ns, may be it's better
to just call timer_mod(ts, 0) to shorten waiting instead of cheating with .scheduled?


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v6 5/7] qemu-coroutine-sleep: introduce qemu_co_sleep_wake

[Kevin Wolf]

Am 07.06.2019 um 19:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 07.06.2019 18:52, Vladimir Sementsov-Ogievskiy wrote:
> > 07.06.2019 16:02, Kevin Wolf wrote:
> >> Am 07.06.2019 um 13:18 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >>> 07.06.2019 10:57, Kevin Wolf wrote:
> >>>> Am 11.04.2019 um 19:27 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >>>>> Introduce a function to gracefully wake-up a coroutine, sleeping in
> >>>>> qemu_co_sleep_ns() sleep.
> >>>>>
> >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >>>>
> >>>> You can simply reenter the coroutine while it has yielded in
> >>>> qemu_co_sleep_ns(). This is supported.
> >>>
> >>> No it doesn't. qemu_aio_coroutine_enter checks for scheduled field,
> >>> and aborts if it is set.
> >>
> >> Ah, yes, it has been broken since commit
> >>
> >> I actually tried to fix it once, but it turned out more complicated and
> >> I think we found a different solution for the problem at hand:
> >>
> >>      Subject: [PATCH for-2.11 0/4] Fix qemu-iotests failures
> >>      Message-Id: <20171128154350.21504-1-kwolf@redhat.com>
> >>
> >> In this case, I guess your approach with a new function to interrupt
> >> qemu_co_sleep_ns() is okay.
> >>
> >> Do we need to timer_del() when taking the shortcut? We don't necessarily
> >> reenter the coroutine immediately, but might only be scheduling it. In
> >> this case, the timer could fire before qemu_co_sleep_ns() has run and
> >> schedule the coroutine a second time
> > 
> > No it will not, as we do cmpxchg, scheduled to NULL, so second call will do
> > nothing..
> > 
> > But it seems unsafe, as even coroutine pointer may be stale when we call
> > qemu_co_sleep_wake second time. So, we possibly should remove timer, but ..
> > 
> >   (ignoring co->scheduled again -
> >> maybe we should actually not do that in the timer callback path, but
> >> instead let it run into the assertion because it would be a bug for the
> >> timer callback to end up in this situation).
> >>
> >> Kevin
> >>
> > 
> > Interesting, could there be a race condition, when we call qemu_co_sleep_wake,
> > but co_sleep_cb already scheduled in some queue and will run soon? Then removing
> > the timer will not help.
> > 
> > 
> 
> Hmm, it's commented that timer_del is thread-safe..
> 
> Hmm, so, if anyway want to return Timer pointer from qemu_co_sleep_ns, may be it's better
> to just call timer_mod(ts, 0) to shorten waiting instead of cheating with .scheduled?

This is probably slower than timer_del() and directly entering the
coroutine. Is there any advantage in using timer_mod()? I don't think
messing with .scheduled is too bad as it's set in the function just
below, so it pairs nicely enough.

Kevin

Re: [Qemu-devel] [PATCH v6 5/7] qemu-coroutine-sleep: introduce qemu_co_sleep_wake

[Vladimir Sementsov-Ogievskiy]

11.06.2019 11:53, Kevin Wolf wrote:
> Am 07.06.2019 um 19:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 07.06.2019 18:52, Vladimir Sementsov-Ogievskiy wrote:
>>> 07.06.2019 16:02, Kevin Wolf wrote:
>>>> Am 07.06.2019 um 13:18 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>> 07.06.2019 10:57, Kevin Wolf wrote:
>>>>>> Am 11.04.2019 um 19:27 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>>>> Introduce a function to gracefully wake-up a coroutine, sleeping in
>>>>>>> qemu_co_sleep_ns() sleep.
>>>>>>>
>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>>
>>>>>> You can simply reenter the coroutine while it has yielded in
>>>>>> qemu_co_sleep_ns(). This is supported.
>>>>>
>>>>> No it doesn't. qemu_aio_coroutine_enter checks for scheduled field,
>>>>> and aborts if it is set.
>>>>
>>>> Ah, yes, it has been broken since commit
>>>>
>>>> I actually tried to fix it once, but it turned out more complicated and
>>>> I think we found a different solution for the problem at hand:
>>>>
>>>>       Subject: [PATCH for-2.11 0/4] Fix qemu-iotests failures
>>>>       Message-Id: <20171128154350.21504-1-kwolf@redhat.com>
>>>>
>>>> In this case, I guess your approach with a new function to interrupt
>>>> qemu_co_sleep_ns() is okay.
>>>>
>>>> Do we need to timer_del() when taking the shortcut? We don't necessarily
>>>> reenter the coroutine immediately, but might only be scheduling it. In
>>>> this case, the timer could fire before qemu_co_sleep_ns() has run and
>>>> schedule the coroutine a second time
>>>
>>> No it will not, as we do cmpxchg, scheduled to NULL, so second call will do
>>> nothing..
>>>
>>> But it seems unsafe, as even coroutine pointer may be stale when we call
>>> qemu_co_sleep_wake second time. So, we possibly should remove timer, but ..
>>>
>>>    (ignoring co->scheduled again -
>>>> maybe we should actually not do that in the timer callback path, but
>>>> instead let it run into the assertion because it would be a bug for the
>>>> timer callback to end up in this situation).
>>>>
>>>> Kevin
>>>>
>>>
>>> Interesting, could there be a race condition, when we call qemu_co_sleep_wake,
>>> but co_sleep_cb already scheduled in some queue and will run soon? Then removing
>>> the timer will not help.
>>>
>>>
>>
>> Hmm, it's commented that timer_del is thread-safe..
>>
>> Hmm, so, if anyway want to return Timer pointer from qemu_co_sleep_ns, may be it's better
>> to just call timer_mod(ts, 0) to shorten waiting instead of cheating with .scheduled?
> 
> This is probably slower than timer_del() and directly entering the
> coroutine. Is there any advantage in using timer_mod()? I don't think
> messing with .scheduled is too bad as it's set in the function just
> below, so it pairs nicely enough.
> 

Ok, will try this variant too.


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect

[Vladimir Sementsov-Ogievskiy]

Implement reconnect. To achieve this:

1. add new modes:
   connecting-wait: means, that reconnecting is in progress, and there
     were small number of reconnect attempts, so all requests are
     waiting for the connection.
   connecting-nowait: reconnecting is in progress, there were a lot of
     attempts of reconnect, all requests will return errors.

   two old modes are used too:
   connected: normal state
   quit: exiting after fatal error or on close

Possible transitions are:

   * -> quit
   connecting-* -> connected
   connecting-wait -> connecting-nowait (transition is done after
                      reconnect-delay seconds in connecting-wait mode)
   connected -> connecting-wait

2. Implement reconnect in connection_co. So, in connecting-* mode,
    connection_co, tries to reconnect unlimited times.

3. Retry nbd queries on channel error, if we are in connecting-wait
    state.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.h |   7 +
 block/nbd-client.c | 336 +++++++++++++++++++++++++++++++++++----------
 2 files changed, 273 insertions(+), 70 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index 91a6b32bdd..f366c90e5e 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -24,6 +24,8 @@ typedef struct {
 } NBDClientRequest;
 
 typedef enum NBDClientState {
+    NBD_CLIENT_CONNECTING_WAIT,
+    NBD_CLIENT_CONNECTING_NOWAIT,
     NBD_CLIENT_CONNECTED,
     NBD_CLIENT_QUIT
 } NBDClientState;
@@ -38,10 +40,15 @@ typedef struct NBDClientSession {
     Coroutine *connection_co;
     int in_flight;
     NBDClientState state;
+    int connect_status;
+    Error *connect_err;
+    bool wait_in_flight;
 
     NBDClientRequest requests[MAX_NBD_REQUESTS];
     NBDReply reply;
     BlockDriverState *bs;
+
+    uint32_t reconnect_delay;
 } NBDClientSession;
 
 NBDClientSession *nbd_get_client_session(BlockDriverState *bs);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 1359aff162..b829a1a1bd 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -1,6 +1,7 @@
 /*
  * QEMU Block driver for  NBD
  *
+ * Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
  * Copyright (C) 2016 Red Hat, Inc.
  * Copyright (C) 2008 Bull S.A.S.
  *     Author: Laurent Vivier <Laurent.Vivier@bull.net>
@@ -36,10 +37,27 @@
 #define HANDLE_TO_INDEX(bs, handle) ((handle) ^ (uint64_t)(intptr_t)(bs))
 #define INDEX_TO_HANDLE(bs, index)  ((index)  ^ (uint64_t)(intptr_t)(bs))
 
-/* @ret will be used for reconnect in future */
+static int nbd_client_connect(BlockDriverState *bs,
+                              SocketAddress *saddr,
+                              const char *export,
+                              QCryptoTLSCreds *tlscreds,
+                              const char *hostname,
+                              const char *x_dirty_bitmap,
+                              Error **errp);
+
 static void nbd_channel_error(NBDClientSession *s, int ret)
 {
-    s->state = NBD_CLIENT_QUIT;
+    if (ret == -EIO) {
+        if (s->state == NBD_CLIENT_CONNECTED) {
+            s->state = s->reconnect_delay ? NBD_CLIENT_CONNECTING_WAIT :
+                                            NBD_CLIENT_CONNECTING_NOWAIT;
+        }
+    } else {
+        if (s->state == NBD_CLIENT_CONNECTED) {
+            qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+        }
+        s->state = NBD_CLIENT_QUIT;
+    }
 }
 
 static void nbd_recv_coroutines_wake_all(NBDClientSession *s)
@@ -59,24 +77,133 @@ static void nbd_teardown_connection(BlockDriverState *bs)
 {
     NBDClientSession *client = nbd_get_client_session(bs);
 
-    assert(client->ioc);
-
-    /* finish any pending coroutines */
-    qio_channel_shutdown(client->ioc,
-                         QIO_CHANNEL_SHUTDOWN_BOTH,
-                         NULL);
+    if (client->state == NBD_CLIENT_CONNECTED) {
+        /* finish any pending coroutines */
+        assert(client->ioc);
+        qio_channel_shutdown(client->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+    }
+    client->state = NBD_CLIENT_QUIT;
+    if (client->connection_co) {
+        qemu_co_sleep_wake(client->connection_co);
+    }
     BDRV_POLL_WHILE(bs, client->connection_co);
+}
+
+typedef struct NBDConnection {
+    BlockDriverState *bs;
+    SocketAddress *saddr;
+    const char *export;
+    QCryptoTLSCreds *tlscreds;
+    const char *hostname;
+    const char *x_dirty_bitmap;
+} NBDConnection;
+
+static bool nbd_client_connecting(NBDClientSession *client)
+{
+    return client->state == NBD_CLIENT_CONNECTING_WAIT ||
+           client->state == NBD_CLIENT_CONNECTING_NOWAIT;
+}
+
+static bool nbd_client_connecting_wait(NBDClientSession *client)
+{
+    return client->state == NBD_CLIENT_CONNECTING_WAIT;
+}
+
+static coroutine_fn void nbd_reconnect_attempt(NBDConnection *con)
+{
+    NBDClientSession *s = nbd_get_client_session(con->bs);
+    Error *local_err = NULL;
+
+    if (!nbd_client_connecting(s)) {
+        return;
+    }
+    assert(nbd_client_connecting(s));
+
+    /* Wait completion of all in-flight requests */
+
+    qemu_co_mutex_lock(&s->send_mutex);
 
-    nbd_client_detach_aio_context(bs);
-    object_unref(OBJECT(client->sioc));
-    client->sioc = NULL;
-    object_unref(OBJECT(client->ioc));
-    client->ioc = NULL;
+    while (s->in_flight > 0) {
+        qemu_co_mutex_unlock(&s->send_mutex);
+        nbd_recv_coroutines_wake_all(s);
+        s->wait_in_flight = true;
+        qemu_coroutine_yield();
+        s->wait_in_flight = false;
+        qemu_co_mutex_lock(&s->send_mutex);
+    }
+
+    qemu_co_mutex_unlock(&s->send_mutex);
+
+    if (!nbd_client_connecting(s)) {
+        return;
+    }
+
+    /*
+     * Now we are sure, that nobody accessing the channel now and nobody
+     * will try to access the channel, until we set state to CONNECTED
+     */
+
+    /* Finalize previous connection if any */
+    if (s->ioc) {
+        nbd_client_detach_aio_context(con->bs);
+        object_unref(OBJECT(s->sioc));
+        s->sioc = NULL;
+        object_unref(OBJECT(s->ioc));
+        s->ioc = NULL;
+    }
+
+    s->connect_status = nbd_client_connect(con->bs, con->saddr,
+                                           con->export, con->tlscreds,
+                                           con->hostname, con->x_dirty_bitmap,
+                                           &local_err);
+    error_free(s->connect_err);
+    s->connect_err = NULL;
+    error_propagate(&s->connect_err, local_err);
+    local_err = NULL;
+
+    if (s->connect_status < 0) {
+        /* failed attempt */
+        return;
+    }
+
+    /* successfully connected */
+    s->state = NBD_CLIENT_CONNECTED;
+    qemu_co_queue_restart_all(&s->free_sema);
+}
+
+static coroutine_fn void nbd_reconnect_loop(NBDConnection *con)
+{
+    NBDClientSession *s = nbd_get_client_session(con->bs);
+    uint64_t start_time_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+    uint64_t delay_ns = s->reconnect_delay * 1000000000UL;
+    uint64_t timeout = 1000000000UL; /* 1 sec */
+    uint64_t max_timeout = 16000000000UL; /* 16 sec */
+
+    nbd_reconnect_attempt(con);
+
+    while (nbd_client_connecting(s)) {
+        if (s->state == NBD_CLIENT_CONNECTING_WAIT &&
+            qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start_time_ns > delay_ns)
+        {
+            s->state = NBD_CLIENT_CONNECTING_NOWAIT;
+            qemu_co_queue_restart_all(&s->free_sema);
+        }
+
+        bdrv_dec_in_flight(con->bs);
+        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, timeout);
+        bdrv_inc_in_flight(con->bs);
+        if (timeout < max_timeout) {
+            timeout *= 2;
+        }
+
+        nbd_reconnect_attempt(con);
+    }
 }
 
 static coroutine_fn void nbd_connection_entry(void *opaque)
 {
-    NBDClientSession *s = opaque;
+    NBDConnection *con = opaque;
+    NBDClientSession *s = nbd_get_client_session(con->bs);
     uint64_t i;
     int ret = 0;
     Error *local_err = NULL;
@@ -91,16 +218,25 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
          * Therefore we keep an additional in_flight reference all the time and
          * only drop it temporarily here.
          */
+
+        if (nbd_client_connecting(s)) {
+            nbd_reconnect_loop(con);
+        }
+
+        if (s->state != NBD_CLIENT_CONNECTED) {
+            continue;
+        }
+
         assert(s->reply.handle == 0);
         ret = nbd_receive_reply(s->bs, s->ioc, &s->reply, &local_err);
-
         if (local_err) {
             trace_nbd_read_reply_entry_fail(ret, error_get_pretty(local_err));
             error_free(local_err);
+            local_err = NULL;
         }
         if (ret <= 0) {
             nbd_channel_error(s, ret ? ret : -EIO);
-            break;
+            continue;
         }
 
         /* There's no need for a mutex on the receive side, because the
@@ -114,7 +250,7 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
             (nbd_reply_is_structured(&s->reply) && !s->info.structured_reply))
         {
             nbd_channel_error(s, -EINVAL);
-            break;
+            continue;
         }
 
         /* We're woken up again by the request itself.  Note that there
@@ -132,10 +268,21 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
         qemu_coroutine_yield();
     }
 
+    qemu_co_queue_restart_all(&s->free_sema);
     nbd_recv_coroutines_wake_all(s);
     bdrv_dec_in_flight(s->bs);
 
     s->connection_co = NULL;
+    if (s->ioc) {
+        nbd_client_detach_aio_context(con->bs);
+        object_unref(OBJECT(s->sioc));
+        s->sioc = NULL;
+        object_unref(OBJECT(s->ioc));
+        s->ioc = NULL;
+    }
+
+    g_free(con);
+
     aio_wait_kick();
 }
 
@@ -147,7 +294,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
     int rc, i = -1;
 
     qemu_co_mutex_lock(&s->send_mutex);
-    while (s->in_flight == MAX_NBD_REQUESTS) {
+    while (s->in_flight == MAX_NBD_REQUESTS || nbd_client_connecting_wait(s)) {
         qemu_co_queue_wait(&s->free_sema, &s->send_mutex);
     }
 
@@ -198,7 +345,11 @@ err:
             s->requests[i].coroutine = NULL;
             s->in_flight--;
         }
-        qemu_co_queue_next(&s->free_sema);
+        if (s->in_flight == 0 && s->wait_in_flight) {
+            aio_co_wake(s->connection_co);
+        } else {
+            qemu_co_queue_next(&s->free_sema);
+        }
     }
     qemu_co_mutex_unlock(&s->send_mutex);
     return rc;
@@ -577,10 +728,15 @@ static coroutine_fn int nbd_co_receive_one_chunk(
         if (reply) {
             *reply = s->reply;
         }
-        s->reply.handle = 0;
     }
+    s->reply.handle = 0;
 
-    if (s->connection_co) {
+    if (s->connection_co && !s->wait_in_flight) {
+        /*
+         * We must check s->wait_in_flight, because we may entered by
+         * nbd_recv_coroutines_wake_all(), in this case we should not
+         * wake connection_co here, it will woken by last request.
+         */
         aio_co_wake(s->connection_co);
     }
 
@@ -688,7 +844,11 @@ break_loop:
 
     qemu_co_mutex_lock(&s->send_mutex);
     s->in_flight--;
-    qemu_co_queue_next(&s->free_sema);
+    if (s->in_flight == 0 && s->wait_in_flight) {
+        aio_co_wake(s->connection_co);
+    } else {
+        qemu_co_queue_next(&s->free_sema);
+    }
     qemu_co_mutex_unlock(&s->send_mutex);
 
     return false;
@@ -832,20 +992,26 @@ static int nbd_co_request(BlockDriverState *bs, NBDRequest *request,
     } else {
         assert(request->type != NBD_CMD_WRITE);
     }
-    ret = nbd_co_send_request(bs, request, write_qiov);
-    if (ret < 0) {
-        return ret;
-    }
 
-    ret = nbd_co_receive_return_code(client, request->handle,
-                                     &request_ret, &local_err);
-    if (local_err) {
-        trace_nbd_co_request_fail(request->from, request->len, request->handle,
-                                  request->flags, request->type,
-                                  nbd_cmd_lookup(request->type),
-                                  ret, error_get_pretty(local_err));
-        error_free(local_err);
-    }
+    do {
+        ret = nbd_co_send_request(bs, request, write_qiov);
+        if (ret < 0) {
+            continue;
+        }
+
+        ret = nbd_co_receive_return_code(client, request->handle,
+                                         &request_ret, &local_err);
+        if (local_err) {
+            trace_nbd_co_request_fail(request->from, request->len,
+                                      request->handle, request->flags,
+                                      request->type,
+                                      nbd_cmd_lookup(request->type),
+                                      ret, error_get_pretty(local_err));
+            error_free(local_err);
+            local_err = NULL;
+        }
+    } while (ret < 0 && nbd_client_connecting_wait(client));
+
     return ret ? ret : request_ret;
 }
 
@@ -886,20 +1052,24 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
         request.len -= slop;
     }
 
-    ret = nbd_co_send_request(bs, &request, NULL);
-    if (ret < 0) {
-        return ret;
-    }
+    do {
+        ret = nbd_co_send_request(bs, &request, NULL);
+        if (ret < 0) {
+            continue;
+        }
+
+        ret = nbd_co_receive_cmdread_reply(client, request.handle, offset, qiov,
+                                           &request_ret, &local_err);
+        if (local_err) {
+            trace_nbd_co_request_fail(request.from, request.len, request.handle,
+                                      request.flags, request.type,
+                                      nbd_cmd_lookup(request.type),
+                                      ret, error_get_pretty(local_err));
+            error_free(local_err);
+            local_err = NULL;
+        }
+    } while (ret < 0 && nbd_client_connecting_wait(client));
 
-    ret = nbd_co_receive_cmdread_reply(client, request.handle, offset, qiov,
-                                       &request_ret, &local_err);
-    if (local_err) {
-        trace_nbd_co_request_fail(request.from, request.len, request.handle,
-                                  request.flags, request.type,
-                                  nbd_cmd_lookup(request.type),
-                                  ret, error_get_pretty(local_err));
-        error_free(local_err);
-    }
     return ret ? ret : request_ret;
 }
 
@@ -1033,20 +1203,25 @@ int coroutine_fn nbd_client_co_block_status(BlockDriverState *bs,
     if (client->info.min_block) {
         assert(QEMU_IS_ALIGNED(request.len, client->info.min_block));
     }
-    ret = nbd_co_send_request(bs, &request, NULL);
-    if (ret < 0) {
-        return ret;
-    }
+    do {
+        ret = nbd_co_send_request(bs, &request, NULL);
+        if (ret < 0) {
+            continue;
+        }
+
+        ret = nbd_co_receive_blockstatus_reply(client, request.handle, bytes,
+                                               &extent, &request_ret,
+                                               &local_err);
+        if (local_err) {
+            trace_nbd_co_request_fail(request.from, request.len, request.handle,
+                                      request.flags, request.type,
+                                      nbd_cmd_lookup(request.type),
+                                      ret, error_get_pretty(local_err));
+            error_free(local_err);
+            local_err = NULL;
+        }
+    } while (ret < 0 && nbd_client_connecting_wait(client));
 
-    ret = nbd_co_receive_blockstatus_reply(client, request.handle, bytes,
-                                           &extent, &request_ret, &local_err);
-    if (local_err) {
-        trace_nbd_co_request_fail(request.from, request.len, request.handle,
-                                  request.flags, request.type,
-                                  nbd_cmd_lookup(request.type),
-                                  ret, error_get_pretty(local_err));
-        error_free(local_err);
-    }
     if (ret < 0 || request_ret < 0) {
         return ret ? ret : request_ret;
     }
@@ -1083,13 +1258,22 @@ void nbd_client_attach_aio_context(BlockDriverState *bs,
                                    AioContext *new_context)
 {
     NBDClientSession *client = nbd_get_client_session(bs);
-    qio_channel_attach_aio_context(QIO_CHANNEL(client->ioc), new_context);
 
-    bdrv_inc_in_flight(bs);
+    /*
+     * client->connection_co is either yielded from nbd_receive_reply or from
+     * nbd_reconnect_loop(), in latter case we do nothing
+     */
+    if (client->state == NBD_CLIENT_CONNECTED) {
+        qio_channel_attach_aio_context(QIO_CHANNEL(client->ioc), new_context);
 
-    /* Need to wait here for the BH to run because the BH must run while the
-     * node is still drained. */
-    aio_wait_bh_oneshot(new_context, nbd_client_attach_aio_context_bh, bs);
+        bdrv_inc_in_flight(bs);
+
+        /*
+         * Need to wait here for the BH to run because the BH must run while the
+         * node is still drained.
+         */
+        aio_wait_bh_oneshot(new_context, nbd_client_attach_aio_context_bh, bs);
+    }
 }
 
 void nbd_client_close(BlockDriverState *bs)
@@ -1097,9 +1281,9 @@ void nbd_client_close(BlockDriverState *bs)
     NBDClientSession *client = nbd_get_client_session(bs);
     NBDRequest request = { .type = NBD_CMD_DISC };
 
-    assert(client->ioc);
-
-    nbd_send_request(client->ioc, &request);
+    if (client->ioc) {
+        nbd_send_request(client->ioc, &request);
+    }
 
     nbd_teardown_connection(bs);
 }
@@ -1223,6 +1407,7 @@ int nbd_client_init(BlockDriverState *bs,
 {
     int ret;
     NBDClientSession *client = nbd_get_client_session(bs);
+    NBDConnection *con;
 
     client->bs = bs;
     qemu_co_mutex_init(&client->send_mutex);
@@ -1233,8 +1418,19 @@ int nbd_client_init(BlockDriverState *bs,
     if (ret < 0) {
         return ret;
     }
+    /* successfully connected */
+    client->state = NBD_CLIENT_CONNECTED;
+    client->reconnect_delay = reconnect_delay;
+
+    con = g_new(NBDConnection, 1);
+    con->bs = bs;
+    con->saddr = saddr;
+    con->export = export;
+    con->tlscreds = tlscreds;
+    con->hostname = hostname;
+    con->x_dirty_bitmap = x_dirty_bitmap;
 
-    client->connection_co = qemu_coroutine_create(nbd_connection_entry, client);
+    client->connection_co = qemu_coroutine_create(nbd_connection_entry, con);
     bdrv_inc_in_flight(bs);
     aio_co_schedule(bdrv_get_aio_context(bs), client->connection_co);
 
-- 
2.18.0

[Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect

[Vladimir Sementsov-Ogievskiy]

Implement reconnect. To achieve this:

1. add new modes:
   connecting-wait: means, that reconnecting is in progress, and there
     were small number of reconnect attempts, so all requests are
     waiting for the connection.
   connecting-nowait: reconnecting is in progress, there were a lot of
     attempts of reconnect, all requests will return errors.

   two old modes are used too:
   connected: normal state
   quit: exiting after fatal error or on close

Possible transitions are:

   * -> quit
   connecting-* -> connected
   connecting-wait -> connecting-nowait (transition is done after
                      reconnect-delay seconds in connecting-wait mode)
   connected -> connecting-wait

2. Implement reconnect in connection_co. So, in connecting-* mode,
    connection_co, tries to reconnect unlimited times.

3. Retry nbd queries on channel error, if we are in connecting-wait
    state.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.h |   7 +
 block/nbd-client.c | 336 +++++++++++++++++++++++++++++++++++----------
 2 files changed, 273 insertions(+), 70 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index 91a6b32bdd..f366c90e5e 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -24,6 +24,8 @@ typedef struct {
 } NBDClientRequest;
 
 typedef enum NBDClientState {
+    NBD_CLIENT_CONNECTING_WAIT,
+    NBD_CLIENT_CONNECTING_NOWAIT,
     NBD_CLIENT_CONNECTED,
     NBD_CLIENT_QUIT
 } NBDClientState;
@@ -38,10 +40,15 @@ typedef struct NBDClientSession {
     Coroutine *connection_co;
     int in_flight;
     NBDClientState state;
+    int connect_status;
+    Error *connect_err;
+    bool wait_in_flight;
 
     NBDClientRequest requests[MAX_NBD_REQUESTS];
     NBDReply reply;
     BlockDriverState *bs;
+
+    uint32_t reconnect_delay;
 } NBDClientSession;
 
 NBDClientSession *nbd_get_client_session(BlockDriverState *bs);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 1359aff162..b829a1a1bd 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -1,6 +1,7 @@
 /*
  * QEMU Block driver for  NBD
  *
+ * Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
  * Copyright (C) 2016 Red Hat, Inc.
  * Copyright (C) 2008 Bull S.A.S.
  *     Author: Laurent Vivier <Laurent.Vivier@bull.net>
@@ -36,10 +37,27 @@
 #define HANDLE_TO_INDEX(bs, handle) ((handle) ^ (uint64_t)(intptr_t)(bs))
 #define INDEX_TO_HANDLE(bs, index)  ((index)  ^ (uint64_t)(intptr_t)(bs))
 
-/* @ret will be used for reconnect in future */
+static int nbd_client_connect(BlockDriverState *bs,
+                              SocketAddress *saddr,
+                              const char *export,
+                              QCryptoTLSCreds *tlscreds,
+                              const char *hostname,
+                              const char *x_dirty_bitmap,
+                              Error **errp);
+
 static void nbd_channel_error(NBDClientSession *s, int ret)
 {
-    s->state = NBD_CLIENT_QUIT;
+    if (ret == -EIO) {
+        if (s->state == NBD_CLIENT_CONNECTED) {
+            s->state = s->reconnect_delay ? NBD_CLIENT_CONNECTING_WAIT :
+                                            NBD_CLIENT_CONNECTING_NOWAIT;
+        }
+    } else {
+        if (s->state == NBD_CLIENT_CONNECTED) {
+            qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+        }
+        s->state = NBD_CLIENT_QUIT;
+    }
 }
 
 static void nbd_recv_coroutines_wake_all(NBDClientSession *s)
@@ -59,24 +77,133 @@ static void nbd_teardown_connection(BlockDriverState *bs)
 {
     NBDClientSession *client = nbd_get_client_session(bs);
 
-    assert(client->ioc);
-
-    /* finish any pending coroutines */
-    qio_channel_shutdown(client->ioc,
-                         QIO_CHANNEL_SHUTDOWN_BOTH,
-                         NULL);
+    if (client->state == NBD_CLIENT_CONNECTED) {
+        /* finish any pending coroutines */
+        assert(client->ioc);
+        qio_channel_shutdown(client->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+    }
+    client->state = NBD_CLIENT_QUIT;
+    if (client->connection_co) {
+        qemu_co_sleep_wake(client->connection_co);
+    }
     BDRV_POLL_WHILE(bs, client->connection_co);
+}
+
+typedef struct NBDConnection {
+    BlockDriverState *bs;
+    SocketAddress *saddr;
+    const char *export;
+    QCryptoTLSCreds *tlscreds;
+    const char *hostname;
+    const char *x_dirty_bitmap;
+} NBDConnection;
+
+static bool nbd_client_connecting(NBDClientSession *client)
+{
+    return client->state == NBD_CLIENT_CONNECTING_WAIT ||
+           client->state == NBD_CLIENT_CONNECTING_NOWAIT;
+}
+
+static bool nbd_client_connecting_wait(NBDClientSession *client)
+{
+    return client->state == NBD_CLIENT_CONNECTING_WAIT;
+}
+
+static coroutine_fn void nbd_reconnect_attempt(NBDConnection *con)
+{
+    NBDClientSession *s = nbd_get_client_session(con->bs);
+    Error *local_err = NULL;
+
+    if (!nbd_client_connecting(s)) {
+        return;
+    }
+    assert(nbd_client_connecting(s));
+
+    /* Wait completion of all in-flight requests */
+
+    qemu_co_mutex_lock(&s->send_mutex);
 
-    nbd_client_detach_aio_context(bs);
-    object_unref(OBJECT(client->sioc));
-    client->sioc = NULL;
-    object_unref(OBJECT(client->ioc));
-    client->ioc = NULL;
+    while (s->in_flight > 0) {
+        qemu_co_mutex_unlock(&s->send_mutex);
+        nbd_recv_coroutines_wake_all(s);
+        s->wait_in_flight = true;
+        qemu_coroutine_yield();
+        s->wait_in_flight = false;
+        qemu_co_mutex_lock(&s->send_mutex);
+    }
+
+    qemu_co_mutex_unlock(&s->send_mutex);
+
+    if (!nbd_client_connecting(s)) {
+        return;
+    }
+
+    /*
+     * Now we are sure, that nobody accessing the channel now and nobody
+     * will try to access the channel, until we set state to CONNECTED
+     */
+
+    /* Finalize previous connection if any */
+    if (s->ioc) {
+        nbd_client_detach_aio_context(con->bs);
+        object_unref(OBJECT(s->sioc));
+        s->sioc = NULL;
+        object_unref(OBJECT(s->ioc));
+        s->ioc = NULL;
+    }
+
+    s->connect_status = nbd_client_connect(con->bs, con->saddr,
+                                           con->export, con->tlscreds,
+                                           con->hostname, con->x_dirty_bitmap,
+                                           &local_err);
+    error_free(s->connect_err);
+    s->connect_err = NULL;
+    error_propagate(&s->connect_err, local_err);
+    local_err = NULL;
+
+    if (s->connect_status < 0) {
+        /* failed attempt */
+        return;
+    }
+
+    /* successfully connected */
+    s->state = NBD_CLIENT_CONNECTED;
+    qemu_co_queue_restart_all(&s->free_sema);
+}
+
+static coroutine_fn void nbd_reconnect_loop(NBDConnection *con)
+{
+    NBDClientSession *s = nbd_get_client_session(con->bs);
+    uint64_t start_time_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+    uint64_t delay_ns = s->reconnect_delay * 1000000000UL;
+    uint64_t timeout = 1000000000UL; /* 1 sec */
+    uint64_t max_timeout = 16000000000UL; /* 16 sec */
+
+    nbd_reconnect_attempt(con);
+
+    while (nbd_client_connecting(s)) {
+        if (s->state == NBD_CLIENT_CONNECTING_WAIT &&
+            qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start_time_ns > delay_ns)
+        {
+            s->state = NBD_CLIENT_CONNECTING_NOWAIT;
+            qemu_co_queue_restart_all(&s->free_sema);
+        }
+
+        bdrv_dec_in_flight(con->bs);
+        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, timeout);
+        bdrv_inc_in_flight(con->bs);
+        if (timeout < max_timeout) {
+            timeout *= 2;
+        }
+
+        nbd_reconnect_attempt(con);
+    }
 }
 
 static coroutine_fn void nbd_connection_entry(void *opaque)
 {
-    NBDClientSession *s = opaque;
+    NBDConnection *con = opaque;
+    NBDClientSession *s = nbd_get_client_session(con->bs);
     uint64_t i;
     int ret = 0;
     Error *local_err = NULL;
@@ -91,16 +218,25 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
          * Therefore we keep an additional in_flight reference all the time and
          * only drop it temporarily here.
          */
+
+        if (nbd_client_connecting(s)) {
+            nbd_reconnect_loop(con);
+        }
+
+        if (s->state != NBD_CLIENT_CONNECTED) {
+            continue;
+        }
+
         assert(s->reply.handle == 0);
         ret = nbd_receive_reply(s->bs, s->ioc, &s->reply, &local_err);
-
         if (local_err) {
             trace_nbd_read_reply_entry_fail(ret, error_get_pretty(local_err));
             error_free(local_err);
+            local_err = NULL;
         }
         if (ret <= 0) {
             nbd_channel_error(s, ret ? ret : -EIO);
-            break;
+            continue;
         }
 
         /* There's no need for a mutex on the receive side, because the
@@ -114,7 +250,7 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
             (nbd_reply_is_structured(&s->reply) && !s->info.structured_reply))
         {
             nbd_channel_error(s, -EINVAL);
-            break;
+            continue;
         }
 
         /* We're woken up again by the request itself.  Note that there
@@ -132,10 +268,21 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
         qemu_coroutine_yield();
     }
 
+    qemu_co_queue_restart_all(&s->free_sema);
     nbd_recv_coroutines_wake_all(s);
     bdrv_dec_in_flight(s->bs);
 
     s->connection_co = NULL;
+    if (s->ioc) {
+        nbd_client_detach_aio_context(con->bs);
+        object_unref(OBJECT(s->sioc));
+        s->sioc = NULL;
+        object_unref(OBJECT(s->ioc));
+        s->ioc = NULL;
+    }
+
+    g_free(con);
+
     aio_wait_kick();
 }
 
@@ -147,7 +294,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
     int rc, i = -1;
 
     qemu_co_mutex_lock(&s->send_mutex);
-    while (s->in_flight == MAX_NBD_REQUESTS) {
+    while (s->in_flight == MAX_NBD_REQUESTS || nbd_client_connecting_wait(s)) {
         qemu_co_queue_wait(&s->free_sema, &s->send_mutex);
     }
 
@@ -198,7 +345,11 @@ err:
             s->requests[i].coroutine = NULL;
             s->in_flight--;
         }
-        qemu_co_queue_next(&s->free_sema);
+        if (s->in_flight == 0 && s->wait_in_flight) {
+            aio_co_wake(s->connection_co);
+        } else {
+            qemu_co_queue_next(&s->free_sema);
+        }
     }
     qemu_co_mutex_unlock(&s->send_mutex);
     return rc;
@@ -577,10 +728,15 @@ static coroutine_fn int nbd_co_receive_one_chunk(
         if (reply) {
             *reply = s->reply;
         }
-        s->reply.handle = 0;
     }
+    s->reply.handle = 0;
 
-    if (s->connection_co) {
+    if (s->connection_co && !s->wait_in_flight) {
+        /*
+         * We must check s->wait_in_flight, because we may entered by
+         * nbd_recv_coroutines_wake_all(), in this case we should not
+         * wake connection_co here, it will woken by last request.
+         */
         aio_co_wake(s->connection_co);
     }
 
@@ -688,7 +844,11 @@ break_loop:
 
     qemu_co_mutex_lock(&s->send_mutex);
     s->in_flight--;
-    qemu_co_queue_next(&s->free_sema);
+    if (s->in_flight == 0 && s->wait_in_flight) {
+        aio_co_wake(s->connection_co);
+    } else {
+        qemu_co_queue_next(&s->free_sema);
+    }
     qemu_co_mutex_unlock(&s->send_mutex);
 
     return false;
@@ -832,20 +992,26 @@ static int nbd_co_request(BlockDriverState *bs, NBDRequest *request,
     } else {
         assert(request->type != NBD_CMD_WRITE);
     }
-    ret = nbd_co_send_request(bs, request, write_qiov);
-    if (ret < 0) {
-        return ret;
-    }
 
-    ret = nbd_co_receive_return_code(client, request->handle,
-                                     &request_ret, &local_err);
-    if (local_err) {
-        trace_nbd_co_request_fail(request->from, request->len, request->handle,
-                                  request->flags, request->type,
-                                  nbd_cmd_lookup(request->type),
-                                  ret, error_get_pretty(local_err));
-        error_free(local_err);
-    }
+    do {
+        ret = nbd_co_send_request(bs, request, write_qiov);
+        if (ret < 0) {
+            continue;
+        }
+
+        ret = nbd_co_receive_return_code(client, request->handle,
+                                         &request_ret, &local_err);
+        if (local_err) {
+            trace_nbd_co_request_fail(request->from, request->len,
+                                      request->handle, request->flags,
+                                      request->type,
+                                      nbd_cmd_lookup(request->type),
+                                      ret, error_get_pretty(local_err));
+            error_free(local_err);
+            local_err = NULL;
+        }
+    } while (ret < 0 && nbd_client_connecting_wait(client));
+
     return ret ? ret : request_ret;
 }
 
@@ -886,20 +1052,24 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
         request.len -= slop;
     }
 
-    ret = nbd_co_send_request(bs, &request, NULL);
-    if (ret < 0) {
-        return ret;
-    }
+    do {
+        ret = nbd_co_send_request(bs, &request, NULL);
+        if (ret < 0) {
+            continue;
+        }
+
+        ret = nbd_co_receive_cmdread_reply(client, request.handle, offset, qiov,
+                                           &request_ret, &local_err);
+        if (local_err) {
+            trace_nbd_co_request_fail(request.from, request.len, request.handle,
+                                      request.flags, request.type,
+                                      nbd_cmd_lookup(request.type),
+                                      ret, error_get_pretty(local_err));
+            error_free(local_err);
+            local_err = NULL;
+        }
+    } while (ret < 0 && nbd_client_connecting_wait(client));
 
-    ret = nbd_co_receive_cmdread_reply(client, request.handle, offset, qiov,
-                                       &request_ret, &local_err);
-    if (local_err) {
-        trace_nbd_co_request_fail(request.from, request.len, request.handle,
-                                  request.flags, request.type,
-                                  nbd_cmd_lookup(request.type),
-                                  ret, error_get_pretty(local_err));
-        error_free(local_err);
-    }
     return ret ? ret : request_ret;
 }
 
@@ -1033,20 +1203,25 @@ int coroutine_fn nbd_client_co_block_status(BlockDriverState *bs,
     if (client->info.min_block) {
         assert(QEMU_IS_ALIGNED(request.len, client->info.min_block));
     }
-    ret = nbd_co_send_request(bs, &request, NULL);
-    if (ret < 0) {
-        return ret;
-    }
+    do {
+        ret = nbd_co_send_request(bs, &request, NULL);
+        if (ret < 0) {
+            continue;
+        }
+
+        ret = nbd_co_receive_blockstatus_reply(client, request.handle, bytes,
+                                               &extent, &request_ret,
+                                               &local_err);
+        if (local_err) {
+            trace_nbd_co_request_fail(request.from, request.len, request.handle,
+                                      request.flags, request.type,
+                                      nbd_cmd_lookup(request.type),
+                                      ret, error_get_pretty(local_err));
+            error_free(local_err);
+            local_err = NULL;
+        }
+    } while (ret < 0 && nbd_client_connecting_wait(client));
 
-    ret = nbd_co_receive_blockstatus_reply(client, request.handle, bytes,
-                                           &extent, &request_ret, &local_err);
-    if (local_err) {
-        trace_nbd_co_request_fail(request.from, request.len, request.handle,
-                                  request.flags, request.type,
-                                  nbd_cmd_lookup(request.type),
-                                  ret, error_get_pretty(local_err));
-        error_free(local_err);
-    }
     if (ret < 0 || request_ret < 0) {
         return ret ? ret : request_ret;
     }
@@ -1083,13 +1258,22 @@ void nbd_client_attach_aio_context(BlockDriverState *bs,
                                    AioContext *new_context)
 {
     NBDClientSession *client = nbd_get_client_session(bs);
-    qio_channel_attach_aio_context(QIO_CHANNEL(client->ioc), new_context);
 
-    bdrv_inc_in_flight(bs);
+    /*
+     * client->connection_co is either yielded from nbd_receive_reply or from
+     * nbd_reconnect_loop(), in latter case we do nothing
+     */
+    if (client->state == NBD_CLIENT_CONNECTED) {
+        qio_channel_attach_aio_context(QIO_CHANNEL(client->ioc), new_context);
 
-    /* Need to wait here for the BH to run because the BH must run while the
-     * node is still drained. */
-    aio_wait_bh_oneshot(new_context, nbd_client_attach_aio_context_bh, bs);
+        bdrv_inc_in_flight(bs);
+
+        /*
+         * Need to wait here for the BH to run because the BH must run while the
+         * node is still drained.
+         */
+        aio_wait_bh_oneshot(new_context, nbd_client_attach_aio_context_bh, bs);
+    }
 }
 
 void nbd_client_close(BlockDriverState *bs)
@@ -1097,9 +1281,9 @@ void nbd_client_close(BlockDriverState *bs)
     NBDClientSession *client = nbd_get_client_session(bs);
     NBDRequest request = { .type = NBD_CMD_DISC };
 
-    assert(client->ioc);
-
-    nbd_send_request(client->ioc, &request);
+    if (client->ioc) {
+        nbd_send_request(client->ioc, &request);
+    }
 
     nbd_teardown_connection(bs);
 }
@@ -1223,6 +1407,7 @@ int nbd_client_init(BlockDriverState *bs,
 {
     int ret;
     NBDClientSession *client = nbd_get_client_session(bs);
+    NBDConnection *con;
 
     client->bs = bs;
     qemu_co_mutex_init(&client->send_mutex);
@@ -1233,8 +1418,19 @@ int nbd_client_init(BlockDriverState *bs,
     if (ret < 0) {
         return ret;
     }
+    /* successfully connected */
+    client->state = NBD_CLIENT_CONNECTED;
+    client->reconnect_delay = reconnect_delay;
+
+    con = g_new(NBDConnection, 1);
+    con->bs = bs;
+    con->saddr = saddr;
+    con->export = export;
+    con->tlscreds = tlscreds;
+    con->hostname = hostname;
+    con->x_dirty_bitmap = x_dirty_bitmap;
 
-    client->connection_co = qemu_coroutine_create(nbd_connection_entry, client);
+    client->connection_co = qemu_coroutine_create(nbd_connection_entry, con);
     bdrv_inc_in_flight(bs);
     aio_co_schedule(bdrv_get_aio_context(bs), client->connection_co);
 
-- 
2.18.0

Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 9179 bytes --]

On 4/11/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
> Implement reconnect. To achieve this:
> 
> 1. add new modes:
>    connecting-wait: means, that reconnecting is in progress, and there
>      were small number of reconnect attempts, so all requests are
>      waiting for the connection.
>    connecting-nowait: reconnecting is in progress, there were a lot of
>      attempts of reconnect, all requests will return errors.
> 
>    two old modes are used too:
>    connected: normal state
>    quit: exiting after fatal error or on close
> 
> Possible transitions are:
> 
>    * -> quit
>    connecting-* -> connected
>    connecting-wait -> connecting-nowait (transition is done after
>                       reconnect-delay seconds in connecting-wait mode)
>    connected -> connecting-wait
> 
> 2. Implement reconnect in connection_co. So, in connecting-* mode,
>     connection_co, tries to reconnect unlimited times.
> 
> 3. Retry nbd queries on channel error, if we are in connecting-wait
>     state.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

Does this also mean that we can start queuing up guest I/O even before
the first time connected is reached?

>  block/nbd-client.h |   7 +
>  block/nbd-client.c | 336 +++++++++++++++++++++++++++++++++++----------
>  2 files changed, 273 insertions(+), 70 deletions(-)
> 

> +++ b/block/nbd-client.c
> @@ -1,6 +1,7 @@
>  /*
>   * QEMU Block driver for  NBD
>   *
> + * Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.

Adding copyright is fine - you are indeed adding a non-trivial amount of
code to this file. Adding "All rights reserved" is questionable, in part
because it no longer has legal status (see this recent nbdkit patch
https://github.com/libguestfs/nbdkit/commit/952ffe0fc7 for example).

Furthermore, I really cringe when I see it mixed with GPL, because the
GPL works by explicitly stating that you are NOT reserving all rights,
but are rather granting specific permissions to recipients. However, as
this file is BSD licensed, and the various family tree of BSD licenses
have (often due to copy-and-paste) used this phrase in the past, I'm not
going to reject the patch because of the phrase, even though I can
definitely ask if you can remove it.

> @@ -59,24 +77,133 @@ static void nbd_teardown_connection(BlockDriverState *bs)
>  {
>      NBDClientSession *client = nbd_get_client_session(bs);
>  
> -    assert(client->ioc);
> -
> -    /* finish any pending coroutines */
> -    qio_channel_shutdown(client->ioc,
> -                         QIO_CHANNEL_SHUTDOWN_BOTH,
> -                         NULL);
> +    if (client->state == NBD_CLIENT_CONNECTED) {
> +        /* finish any pending coroutines */
> +        assert(client->ioc);
> +        qio_channel_shutdown(client->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> +    }
> +    client->state = NBD_CLIENT_QUIT;
> +    if (client->connection_co) {
> +        qemu_co_sleep_wake(client->connection_co);
> +    }
>      BDRV_POLL_WHILE(bs, client->connection_co);

So you are using the qemu_co_sleep_wake code as a way to in effect
cancel any ongoing sleep. I'm still not sure if there is already another
way to achieve the same effect, perhaps by re-entering the coroutine?

> +typedef struct NBDConnection {
> +    BlockDriverState *bs;
> +    SocketAddress *saddr;
> +    const char *export;
> +    QCryptoTLSCreds *tlscreds;
> +    const char *hostname;
> +    const char *x_dirty_bitmap;
> +} NBDConnection;

Can we put this type in a header, and use it instead of passing lots of
individual parameters to nbd_client_connect()?  Probably as a separate
pre-requisite cleanup patch.

> +
> +static bool nbd_client_connecting(NBDClientSession *client)
> +{
> +    return client->state == NBD_CLIENT_CONNECTING_WAIT ||
> +           client->state == NBD_CLIENT_CONNECTING_NOWAIT;
> +}

I don't know what style we prefer to use here. If my returns split
across lines, I tend to go with either 4-space indent instead of 7, or
to use () so that the second line is indented to the column after (; but
this is aesthetics and so I'm not going to change what you have.

> +
> +static bool nbd_client_connecting_wait(NBDClientSession *client)
> +{
> +    return client->state == NBD_CLIENT_CONNECTING_WAIT;
> +}
> +
> +static coroutine_fn void nbd_reconnect_attempt(NBDConnection *con)
> +{
> +    NBDClientSession *s = nbd_get_client_session(con->bs);
> +    Error *local_err = NULL;
> +
> +    if (!nbd_client_connecting(s)) {
> +        return;
> +    }
> +    assert(nbd_client_connecting(s));
> +
> +    /* Wait completion of all in-flight requests */

Wait for completion

> +
> +    qemu_co_mutex_lock(&s->send_mutex);
>  
> -    nbd_client_detach_aio_context(bs);
> -    object_unref(OBJECT(client->sioc));
> -    client->sioc = NULL;
> -    object_unref(OBJECT(client->ioc));
> -    client->ioc = NULL;
> +    while (s->in_flight > 0) {
> +        qemu_co_mutex_unlock(&s->send_mutex);
> +        nbd_recv_coroutines_wake_all(s);
> +        s->wait_in_flight = true;
> +        qemu_coroutine_yield();
> +        s->wait_in_flight = false;
> +        qemu_co_mutex_lock(&s->send_mutex);
> +    }
> +
> +    qemu_co_mutex_unlock(&s->send_mutex);
> +
> +    if (!nbd_client_connecting(s)) {
> +        return;
> +    }
> +
> +    /*
> +     * Now we are sure, that nobody accessing the channel now and nobody
> +     * will try to access the channel, until we set state to CONNECTED

Now we are sure that nobody is accessing the channel, and no one will
try until we set the state to CONNECTED

> +     */
> +
> +    /* Finalize previous connection if any */
> +    if (s->ioc) {
> +        nbd_client_detach_aio_context(con->bs);
> +        object_unref(OBJECT(s->sioc));
> +        s->sioc = NULL;
> +        object_unref(OBJECT(s->ioc));
> +        s->ioc = NULL;
> +    }
> +
> +    s->connect_status = nbd_client_connect(con->bs, con->saddr,
> +                                           con->export, con->tlscreds,
> +                                           con->hostname, con->x_dirty_bitmap,
> +                                           &local_err);
> +    error_free(s->connect_err);
> +    s->connect_err = NULL;
> +    error_propagate(&s->connect_err, local_err);
> +    local_err = NULL;
> +
> +    if (s->connect_status < 0) {
> +        /* failed attempt */
> +        return;
> +    }
> +
> +    /* successfully connected */
> +    s->state = NBD_CLIENT_CONNECTED;
> +    qemu_co_queue_restart_all(&s->free_sema);
> +}
> +
> +static coroutine_fn void nbd_reconnect_loop(NBDConnection *con)
> +{
> +    NBDClientSession *s = nbd_get_client_session(con->bs);
> +    uint64_t start_time_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> +    uint64_t delay_ns = s->reconnect_delay * 1000000000UL;

Do we have a #define constant for nanoseconds in a second to make this
more legible than counting '0's?

> +    uint64_t timeout = 1000000000UL; /* 1 sec */
> +    uint64_t max_timeout = 16000000000UL; /* 16 sec */

1 * constant, 16 * constant

> +
> +    nbd_reconnect_attempt(con);
> +
> +    while (nbd_client_connecting(s)) {
> +        if (s->state == NBD_CLIENT_CONNECTING_WAIT &&
> +            qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start_time_ns > delay_ns)
> +        {
> +            s->state = NBD_CLIENT_CONNECTING_NOWAIT;
> +            qemu_co_queue_restart_all(&s->free_sema);
> +        }
> +
> +        bdrv_dec_in_flight(con->bs);
> +        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, timeout);

Another place where I'd like someone more familiar with coroutines to
also have a look.

> +        bdrv_inc_in_flight(con->bs);
> +        if (timeout < max_timeout) {
> +            timeout *= 2;
> +        }
> +
> +        nbd_reconnect_attempt(con);
> +    }
>  }
>  
>  static coroutine_fn void nbd_connection_entry(void *opaque)
>  {
> -    NBDClientSession *s = opaque;
> +    NBDConnection *con = opaque;
> +    NBDClientSession *s = nbd_get_client_session(con->bs);
>      uint64_t i;
>      int ret = 0;
>      Error *local_err = NULL;
> @@ -91,16 +218,25 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
>           * Therefore we keep an additional in_flight reference all the time and
>           * only drop it temporarily here.
>           */
> +
> +        if (nbd_client_connecting(s)) {
> +            nbd_reconnect_loop(con);
> +        }
> +
> +        if (s->state != NBD_CLIENT_CONNECTED) {
> +            continue;
> +        }
> +

If I understand, this is not a busy loop because nbd_reconnect_loop()
will pause the coroutine until something interesting happens. Correct?

It's late enough at night for me that I don't trust this to be a full
review; I'll try and look again soon in more details, as well as play
with this against iotests.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1851 bytes --]

Am 07.06.2019 um 05:17 hat Eric Blake geschrieben:
> On 4/11/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
> > +static coroutine_fn void nbd_reconnect_loop(NBDConnection *con)
> > +{
> > +    NBDClientSession *s = nbd_get_client_session(con->bs);
> > +    uint64_t start_time_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> > +    uint64_t delay_ns = s->reconnect_delay * 1000000000UL;
> 
> Do we have a #define constant for nanoseconds in a second to make this
> more legible than counting '0's?
> 
> > +    uint64_t timeout = 1000000000UL; /* 1 sec */
> > +    uint64_t max_timeout = 16000000000UL; /* 16 sec */
> 
> 1 * constant, 16 * constant
> 
> > +
> > +    nbd_reconnect_attempt(con);
> > +
> > +    while (nbd_client_connecting(s)) {
> > +        if (s->state == NBD_CLIENT_CONNECTING_WAIT &&
> > +            qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start_time_ns > delay_ns)
> > +        {
> > +            s->state = NBD_CLIENT_CONNECTING_NOWAIT;
> > +            qemu_co_queue_restart_all(&s->free_sema);
> > +        }
> > +
> > +        bdrv_dec_in_flight(con->bs);
> > +        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, timeout);
> 
> Another place where I'd like someone more familiar with coroutines to
> also have a look.

What's the exact question you'd like me to answer?

But anyway, bdrv_dec/inc_in_flight() around the sleep looks wrong to me.
Either the operation must be waited for in drain, then you can't
decrease the counter even during the sleep. Or drain doesn't have to
consider it, then why is the counter even increased in the first place?

The way it currently is, drain can return assuming that there are no
requests, but after the timeout the request automatically comes back
while the drain caller assumes that there is no more activity. This
doesn't look right.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect

[Vladimir Sementsov-Ogievskiy]

07.06.2019 11:06, Kevin Wolf wrote:
> Am 07.06.2019 um 05:17 hat Eric Blake geschrieben:
>> On 4/11/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> +static coroutine_fn void nbd_reconnect_loop(NBDConnection *con)
>>> +{
>>> +    NBDClientSession *s = nbd_get_client_session(con->bs);
>>> +    uint64_t start_time_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>>> +    uint64_t delay_ns = s->reconnect_delay * 1000000000UL;
>>
>> Do we have a #define constant for nanoseconds in a second to make this
>> more legible than counting '0's?
>>
>>> +    uint64_t timeout = 1000000000UL; /* 1 sec */
>>> +    uint64_t max_timeout = 16000000000UL; /* 16 sec */
>>
>> 1 * constant, 16 * constant
>>
>>> +
>>> +    nbd_reconnect_attempt(con);
>>> +
>>> +    while (nbd_client_connecting(s)) {
>>> +        if (s->state == NBD_CLIENT_CONNECTING_WAIT &&
>>> +            qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start_time_ns > delay_ns)
>>> +        {
>>> +            s->state = NBD_CLIENT_CONNECTING_NOWAIT;
>>> +            qemu_co_queue_restart_all(&s->free_sema);
>>> +        }
>>> +
>>> +        bdrv_dec_in_flight(con->bs);
>>> +        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, timeout);
>>
>> Another place where I'd like someone more familiar with coroutines to
>> also have a look.
> 
> What's the exact question you'd like me to answer?
> 
> But anyway, bdrv_dec/inc_in_flight() around the sleep looks wrong to me.
> Either the operation must be waited for in drain, then you can't
> decrease the counter even during the sleep. Or drain doesn't have to
> consider it, then why is the counter even increased in the first place?
> 
> The way it currently is, drain can return assuming that there are no
> requests, but after the timeout the request automatically comes back
> while the drain caller assumes that there is no more activity. This
> doesn't look right.
> 

Hmm.

This ind/dec around all lifetime of connection coroutine you added in

commit 5ad81b4946baf948c65cf7e1436d9b74803c1280
Author: Kevin Wolf <kwolf@redhat.com>
Date:   Fri Feb 15 16:53:51 2019 +0100

     nbd: Restrict connection_co reentrance


And now I try to connect in endless loop, with qemu_co_sleep_ns() inside.
I need to get a change to bdrv_drain to complete, so I have to sometimes
drop this in_flight request. But I understand your point.

Will the following work better?

bdrv_dec_in_flight(con->bs);
qemu_co_sleep_ns(...);
while (s->drained) {
   s->wait_drained_end = true;
   qemu_coroutine_yield();
}
bdrv_inc_in_flight(con->bs);

...
.drained_begin() {
    s->drained = true;
}

.drained_end() {
    if (s->wait_drained_end) {
       s->wait_drained_end = false;
       aio_co_wake(s->connection_co);
    }
}




-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect

[Kevin Wolf]

Am 07.06.2019 um 14:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 07.06.2019 11:06, Kevin Wolf wrote:
> > Am 07.06.2019 um 05:17 hat Eric Blake geschrieben:
> >> On 4/11/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
> >>> +static coroutine_fn void nbd_reconnect_loop(NBDConnection *con)
> >>> +{
> >>> +    NBDClientSession *s = nbd_get_client_session(con->bs);
> >>> +    uint64_t start_time_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> >>> +    uint64_t delay_ns = s->reconnect_delay * 1000000000UL;
> >>
> >> Do we have a #define constant for nanoseconds in a second to make this
> >> more legible than counting '0's?
> >>
> >>> +    uint64_t timeout = 1000000000UL; /* 1 sec */
> >>> +    uint64_t max_timeout = 16000000000UL; /* 16 sec */
> >>
> >> 1 * constant, 16 * constant
> >>
> >>> +
> >>> +    nbd_reconnect_attempt(con);
> >>> +
> >>> +    while (nbd_client_connecting(s)) {
> >>> +        if (s->state == NBD_CLIENT_CONNECTING_WAIT &&
> >>> +            qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start_time_ns > delay_ns)
> >>> +        {
> >>> +            s->state = NBD_CLIENT_CONNECTING_NOWAIT;
> >>> +            qemu_co_queue_restart_all(&s->free_sema);
> >>> +        }
> >>> +
> >>> +        bdrv_dec_in_flight(con->bs);
> >>> +        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, timeout);
> >>
> >> Another place where I'd like someone more familiar with coroutines to
> >> also have a look.
> > 
> > What's the exact question you'd like me to answer?
> > 
> > But anyway, bdrv_dec/inc_in_flight() around the sleep looks wrong to me.
> > Either the operation must be waited for in drain, then you can't
> > decrease the counter even during the sleep. Or drain doesn't have to
> > consider it, then why is the counter even increased in the first place?
> > 
> > The way it currently is, drain can return assuming that there are no
> > requests, but after the timeout the request automatically comes back
> > while the drain caller assumes that there is no more activity. This
> > doesn't look right.
> 
> Hmm.
> 
> This ind/dec around all lifetime of connection coroutine you added in
> 
> commit 5ad81b4946baf948c65cf7e1436d9b74803c1280
> Author: Kevin Wolf <kwolf@redhat.com>
> Date:   Fri Feb 15 16:53:51 2019 +0100
> 
>      nbd: Restrict connection_co reentrance
> 
> 
> And now I try to connect in endless loop, with qemu_co_sleep_ns() inside.
> I need to get a change to bdrv_drain to complete, so I have to sometimes
> drop this in_flight request. But I understand your point.

Ah, right, I see. I think it's fine to add a second point where we
decrease the counter (though I would add a comment telling why we do
this) as long as the right conditions are met.

The right conditions are probably something like: Once drained, we
guarantee that we don't call any callbacks until the drained section
ends. In nbd_read_eof() this is true because we can't get an answer if
we had no request running.

Or actually... This assumes a nice compliant server that doesn't just
arbitrarily send unexpected messages. Is the existing code broken if we
connect to a malicious server?

> Will the following work better?
> 
> bdrv_dec_in_flight(con->bs);
> qemu_co_sleep_ns(...);
> while (s->drained) {
>    s->wait_drained_end = true;
>    qemu_coroutine_yield();
> }
> bdrv_inc_in_flight(con->bs);
> 
> ...
> .drained_begin() {
>     s->drained = true;
> }
> 
> .drained_end() {
>     if (s->wait_drained_end) {
>        s->wait_drained_end = false;
>        aio_co_wake(s->connection_co);
>     }
> }

This should make sure that we don't call any callbacks before the drain
section has ended.

We probably still have a problem because nbd_client_attach_aio_context()
reenters the coroutine with qemu_aio_coroutine_enter(), which will cause
an assertion failure if co->scheduled is set. So this needs to use a
version that can cancel a sleep.

I see that your patch currently simply ignores attaching a new context,
but then the coroutine stays in the old AioContext. Did I miss some
additional code that moves it to the new context somehow or will it just
stay where it was if the coroutine happens to be reconnecting when the
AioContext was supposed to change?

Kevin

Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect

[Vladimir Sementsov-Ogievskiy]

07.06.2019 16:26, Kevin Wolf wrote:
> Am 07.06.2019 um 14:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 07.06.2019 11:06, Kevin Wolf wrote:
>>> Am 07.06.2019 um 05:17 hat Eric Blake geschrieben:
>>>> On 4/11/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> +static coroutine_fn void nbd_reconnect_loop(NBDConnection *con)
>>>>> +{
>>>>> +    NBDClientSession *s = nbd_get_client_session(con->bs);
>>>>> +    uint64_t start_time_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>>>>> +    uint64_t delay_ns = s->reconnect_delay * 1000000000UL;
>>>>
>>>> Do we have a #define constant for nanoseconds in a second to make this
>>>> more legible than counting '0's?
>>>>
>>>>> +    uint64_t timeout = 1000000000UL; /* 1 sec */
>>>>> +    uint64_t max_timeout = 16000000000UL; /* 16 sec */
>>>>
>>>> 1 * constant, 16 * constant
>>>>
>>>>> +
>>>>> +    nbd_reconnect_attempt(con);
>>>>> +
>>>>> +    while (nbd_client_connecting(s)) {
>>>>> +        if (s->state == NBD_CLIENT_CONNECTING_WAIT &&
>>>>> +            qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start_time_ns > delay_ns)
>>>>> +        {
>>>>> +            s->state = NBD_CLIENT_CONNECTING_NOWAIT;
>>>>> +            qemu_co_queue_restart_all(&s->free_sema);
>>>>> +        }
>>>>> +
>>>>> +        bdrv_dec_in_flight(con->bs);
>>>>> +        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, timeout);
>>>>
>>>> Another place where I'd like someone more familiar with coroutines to
>>>> also have a look.
>>>
>>> What's the exact question you'd like me to answer?
>>>
>>> But anyway, bdrv_dec/inc_in_flight() around the sleep looks wrong to me.
>>> Either the operation must be waited for in drain, then you can't
>>> decrease the counter even during the sleep. Or drain doesn't have to
>>> consider it, then why is the counter even increased in the first place?
>>>
>>> The way it currently is, drain can return assuming that there are no
>>> requests, but after the timeout the request automatically comes back
>>> while the drain caller assumes that there is no more activity. This
>>> doesn't look right.
>>
>> Hmm.
>>
>> This ind/dec around all lifetime of connection coroutine you added in
>>
>> commit 5ad81b4946baf948c65cf7e1436d9b74803c1280
>> Author: Kevin Wolf <kwolf@redhat.com>
>> Date:   Fri Feb 15 16:53:51 2019 +0100
>>
>>       nbd: Restrict connection_co reentrance
>>
>>
>> And now I try to connect in endless loop, with qemu_co_sleep_ns() inside.
>> I need to get a change to bdrv_drain to complete, so I have to sometimes
>> drop this in_flight request. But I understand your point.
> 
> Ah, right, I see. I think it's fine to add a second point where we
> decrease the counter (though I would add a comment telling why we do
> this) as long as the right conditions are met.
> 
> The right conditions are probably something like: Once drained, we
> guarantee that we don't call any callbacks until the drained section
> ends. In nbd_read_eof() this is true because we can't get an answer if
> we had no request running.
> 
> Or actually... This assumes a nice compliant server that doesn't just
> arbitrarily send unexpected messages. Is the existing code broken if we
> connect to a malicious server?
> 
>> Will the following work better?
>>
>> bdrv_dec_in_flight(con->bs);
>> qemu_co_sleep_ns(...);
>> while (s->drained) {
>>     s->wait_drained_end = true;
>>     qemu_coroutine_yield();
>> }
>> bdrv_inc_in_flight(con->bs);
>>
>> ...
>> .drained_begin() {
>>      s->drained = true;
>> }
>>
>> .drained_end() {
>>      if (s->wait_drained_end) {
>>         s->wait_drained_end = false;
>>         aio_co_wake(s->connection_co);
>>      }
>> }
> 
> This should make sure that we don't call any callbacks before the drain
> section has ended.
> 
> We probably still have a problem because nbd_client_attach_aio_context()
> reenters the coroutine with qemu_aio_coroutine_enter(), which will cause
> an assertion failure if co->scheduled is set. So this needs to use a
> version that can cancel a sleep.
> 
> I see that your patch currently simply ignores attaching a new context,
> but then the coroutine stays in the old AioContext. Did I miss some
> additional code that moves it to the new context somehow or will it just
> stay where it was if the coroutine happens to be reconnecting when the
> AioContext was supposed to change?


Hmm. Do you mean "in latter case we do nothing" in

   void nbd_client_attach_aio_context(BlockDriverState *bs,
                                      AioContext *new_context)
   {
       NBDClientSession *client = nbd_get_client_session(bs);

       /*
        * client->connection_co is either yielded from nbd_receive_reply or from
        * nbd_reconnect_loop(), in latter case we do nothing
        */
       if (client->state == NBD_CLIENT_CONNECTED) {
           qio_channel_attach_aio_context(QIO_CHANNEL(client->ioc), new_context);

           bdrv_inc_in_flight(bs);

           /*
            * Need to wait here for the BH to run because the BH must run while the
            * node is still drained.
            */
           aio_wait_bh_oneshot(new_context, nbd_client_attach_aio_context_bh, bs);
       }
   }

?
Not sure why I ignored this case. So, I should run if() body unconditionally here and support
interrupting timer-sleeping coroutine in nbd_client_attach_aio_context_bh, yes?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect

[Kevin Wolf]

Am 07.06.2019 um 16:27 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 07.06.2019 16:26, Kevin Wolf wrote:
> > Am 07.06.2019 um 14:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> 07.06.2019 11:06, Kevin Wolf wrote:
> >>> Am 07.06.2019 um 05:17 hat Eric Blake geschrieben:
> >>>> On 4/11/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
> >>>>> +static coroutine_fn void nbd_reconnect_loop(NBDConnection *con)
> >>>>> +{
> >>>>> +    NBDClientSession *s = nbd_get_client_session(con->bs);
> >>>>> +    uint64_t start_time_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> >>>>> +    uint64_t delay_ns = s->reconnect_delay * 1000000000UL;
> >>>>
> >>>> Do we have a #define constant for nanoseconds in a second to make this
> >>>> more legible than counting '0's?
> >>>>
> >>>>> +    uint64_t timeout = 1000000000UL; /* 1 sec */
> >>>>> +    uint64_t max_timeout = 16000000000UL; /* 16 sec */
> >>>>
> >>>> 1 * constant, 16 * constant
> >>>>
> >>>>> +
> >>>>> +    nbd_reconnect_attempt(con);
> >>>>> +
> >>>>> +    while (nbd_client_connecting(s)) {
> >>>>> +        if (s->state == NBD_CLIENT_CONNECTING_WAIT &&
> >>>>> +            qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start_time_ns > delay_ns)
> >>>>> +        {
> >>>>> +            s->state = NBD_CLIENT_CONNECTING_NOWAIT;
> >>>>> +            qemu_co_queue_restart_all(&s->free_sema);
> >>>>> +        }
> >>>>> +
> >>>>> +        bdrv_dec_in_flight(con->bs);
> >>>>> +        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, timeout);
> >>>>
> >>>> Another place where I'd like someone more familiar with coroutines to
> >>>> also have a look.
> >>>
> >>> What's the exact question you'd like me to answer?
> >>>
> >>> But anyway, bdrv_dec/inc_in_flight() around the sleep looks wrong to me.
> >>> Either the operation must be waited for in drain, then you can't
> >>> decrease the counter even during the sleep. Or drain doesn't have to
> >>> consider it, then why is the counter even increased in the first place?
> >>>
> >>> The way it currently is, drain can return assuming that there are no
> >>> requests, but after the timeout the request automatically comes back
> >>> while the drain caller assumes that there is no more activity. This
> >>> doesn't look right.
> >>
> >> Hmm.
> >>
> >> This ind/dec around all lifetime of connection coroutine you added in
> >>
> >> commit 5ad81b4946baf948c65cf7e1436d9b74803c1280
> >> Author: Kevin Wolf <kwolf@redhat.com>
> >> Date:   Fri Feb 15 16:53:51 2019 +0100
> >>
> >>       nbd: Restrict connection_co reentrance
> >>
> >>
> >> And now I try to connect in endless loop, with qemu_co_sleep_ns() inside.
> >> I need to get a change to bdrv_drain to complete, so I have to sometimes
> >> drop this in_flight request. But I understand your point.
> > 
> > Ah, right, I see. I think it's fine to add a second point where we
> > decrease the counter (though I would add a comment telling why we do
> > this) as long as the right conditions are met.
> > 
> > The right conditions are probably something like: Once drained, we
> > guarantee that we don't call any callbacks until the drained section
> > ends. In nbd_read_eof() this is true because we can't get an answer if
> > we had no request running.
> > 
> > Or actually... This assumes a nice compliant server that doesn't just
> > arbitrarily send unexpected messages. Is the existing code broken if we
> > connect to a malicious server?
> > 
> >> Will the following work better?
> >>
> >> bdrv_dec_in_flight(con->bs);
> >> qemu_co_sleep_ns(...);
> >> while (s->drained) {
> >>     s->wait_drained_end = true;
> >>     qemu_coroutine_yield();
> >> }
> >> bdrv_inc_in_flight(con->bs);
> >>
> >> ...
> >> .drained_begin() {
> >>      s->drained = true;
> >> }
> >>
> >> .drained_end() {
> >>      if (s->wait_drained_end) {
> >>         s->wait_drained_end = false;
> >>         aio_co_wake(s->connection_co);
> >>      }
> >> }
> > 
> > This should make sure that we don't call any callbacks before the drain
> > section has ended.
> > 
> > We probably still have a problem because nbd_client_attach_aio_context()
> > reenters the coroutine with qemu_aio_coroutine_enter(), which will cause
> > an assertion failure if co->scheduled is set. So this needs to use a
> > version that can cancel a sleep.
> > 
> > I see that your patch currently simply ignores attaching a new context,
> > but then the coroutine stays in the old AioContext. Did I miss some
> > additional code that moves it to the new context somehow or will it just
> > stay where it was if the coroutine happens to be reconnecting when the
> > AioContext was supposed to change?
> 
> 
> Hmm. Do you mean "in latter case we do nothing" in
> 
>    void nbd_client_attach_aio_context(BlockDriverState *bs,
>                                       AioContext *new_context)
>    {
>        NBDClientSession *client = nbd_get_client_session(bs);
> 
>        /*
>         * client->connection_co is either yielded from nbd_receive_reply or from
>         * nbd_reconnect_loop(), in latter case we do nothing
>         */
>        if (client->state == NBD_CLIENT_CONNECTED) {
>            qio_channel_attach_aio_context(QIO_CHANNEL(client->ioc), new_context);
> 
>            bdrv_inc_in_flight(bs);
> 
>            /*
>             * Need to wait here for the BH to run because the BH must run while the
>             * node is still drained.
>             */
>            aio_wait_bh_oneshot(new_context, nbd_client_attach_aio_context_bh, bs);
>        }
>    }
> 
> ?
> Not sure why I ignored this case. So, I should run if() body unconditionally here and support
> interrupting timer-sleeping coroutine in nbd_client_attach_aio_context_bh, yes?

Yes, I think so. We have now two places where the coroutine could be
yielded, the old place that simply yielded in a loop and can be
reentered without a problem and the new one in a sleep. Both need to be
supported when we switch to coroutine to a new AioContext.

Kevin

Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect

[Vladimir Sementsov-Ogievskiy]

07.06.2019 6:17, Eric Blake wrote:
> On 4/11/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Implement reconnect. To achieve this:
>>
>> 1. add new modes:
>>     connecting-wait: means, that reconnecting is in progress, and there
>>       were small number of reconnect attempts, so all requests are
>>       waiting for the connection.
>>     connecting-nowait: reconnecting is in progress, there were a lot of
>>       attempts of reconnect, all requests will return errors.
>>
>>     two old modes are used too:
>>     connected: normal state
>>     quit: exiting after fatal error or on close
>>
>> Possible transitions are:
>>
>>     * -> quit
>>     connecting-* -> connected
>>     connecting-wait -> connecting-nowait (transition is done after
>>                        reconnect-delay seconds in connecting-wait mode)
>>     connected -> connecting-wait
>>
>> 2. Implement reconnect in connection_co. So, in connecting-* mode,
>>      connection_co, tries to reconnect unlimited times.
>>
>> 3. Retry nbd queries on channel error, if we are in connecting-wait
>>      state.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
> 
> Does this also mean that we can start queuing up guest I/O even before
> the first time connected is reached?

No, we decided that it's simpler and clearer way to keep first connect to
be synchronous in nbd_open.

> 
>>   block/nbd-client.h |   7 +
>>   block/nbd-client.c | 336 +++++++++++++++++++++++++++++++++++----------
>>   2 files changed, 273 insertions(+), 70 deletions(-)
>>
> 
>> +++ b/block/nbd-client.c
>> @@ -1,6 +1,7 @@
>>   /*
>>    * QEMU Block driver for  NBD
>>    *
>> + * Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
> 
> Adding copyright is fine - you are indeed adding a non-trivial amount of
> code to this file. Adding "All rights reserved" is questionable, in part
> because it no longer has legal status (see this recent nbdkit patch
> https://github.com/libguestfs/nbdkit/commit/952ffe0fc7 for example).
> 
> Furthermore, I really cringe when I see it mixed with GPL, because the
> GPL works by explicitly stating that you are NOT reserving all rights,
> but are rather granting specific permissions to recipients. However, as
> this file is BSD licensed, and the various family tree of BSD licenses
> have (often due to copy-and-paste) used this phrase in the past, I'm not
> going to reject the patch because of the phrase, even though I can
> definitely ask if you can remove it.

Hmm, I think it's not a problem to drop "All rights reserved".

> 
>> @@ -59,24 +77,133 @@ static void nbd_teardown_connection(BlockDriverState *bs)
>>   {
>>       NBDClientSession *client = nbd_get_client_session(bs);
>>   
>> -    assert(client->ioc);
>> -
>> -    /* finish any pending coroutines */
>> -    qio_channel_shutdown(client->ioc,
>> -                         QIO_CHANNEL_SHUTDOWN_BOTH,
>> -                         NULL);
>> +    if (client->state == NBD_CLIENT_CONNECTED) {
>> +        /* finish any pending coroutines */
>> +        assert(client->ioc);
>> +        qio_channel_shutdown(client->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
>> +    }
>> +    client->state = NBD_CLIENT_QUIT;
>> +    if (client->connection_co) {
>> +        qemu_co_sleep_wake(client->connection_co);
>> +    }
>>       BDRV_POLL_WHILE(bs, client->connection_co);
> 
> So you are using the qemu_co_sleep_wake code as a way to in effect
> cancel any ongoing sleep. I'm still not sure if there is already another
> way to achieve the same effect, perhaps by re-entering the coroutine?

It marked as scheduled by qemu_co_sleep_ns and can't be entered, due to this code
in qemu_aio_coroutine_enter:

128             if (scheduled) {
129                 fprintf(stderr,
130                         "%s: Co-routine was already scheduled in '%s'\n",
131                         __func__, scheduled);
132                 abort();
133             }

> 
>> +typedef struct NBDConnection {
>> +    BlockDriverState *bs;
>> +    SocketAddress *saddr;
>> +    const char *export;
>> +    QCryptoTLSCreds *tlscreds;
>> +    const char *hostname;
>> +    const char *x_dirty_bitmap;
>> +} NBDConnection;
> 
> Can we put this type in a header, and use it instead of passing lots of
> individual parameters to nbd_client_connect()?  Probably as a separate
> pre-requisite cleanup patch.

Will try

> 
>> +
>> +static bool nbd_client_connecting(NBDClientSession *client)
>> +{
>> +    return client->state == NBD_CLIENT_CONNECTING_WAIT ||
>> +           client->state == NBD_CLIENT_CONNECTING_NOWAIT;
>> +}
> 
> I don't know what style we prefer to use here. If my returns split
> across lines, I tend to go with either 4-space indent instead of 7, or
> to use () so that the second line is indented to the column after (; but
> this is aesthetics and so I'm not going to change what you have.

Not a problem for me to change, if you dislike)

> 
>> +
>> +static bool nbd_client_connecting_wait(NBDClientSession *client)
>> +{
>> +    return client->state == NBD_CLIENT_CONNECTING_WAIT;
>> +}
>> +
>> +static coroutine_fn void nbd_reconnect_attempt(NBDConnection *con)
>> +{
>> +    NBDClientSession *s = nbd_get_client_session(con->bs);
>> +    Error *local_err = NULL;
>> +
>> +    if (!nbd_client_connecting(s)) {
>> +        return;
>> +    }
>> +    assert(nbd_client_connecting(s));
>> +
>> +    /* Wait completion of all in-flight requests */
> 
> Wait for completion
> 
>> +
>> +    qemu_co_mutex_lock(&s->send_mutex);
>>   
>> -    nbd_client_detach_aio_context(bs);
>> -    object_unref(OBJECT(client->sioc));
>> -    client->sioc = NULL;
>> -    object_unref(OBJECT(client->ioc));
>> -    client->ioc = NULL;
>> +    while (s->in_flight > 0) {
>> +        qemu_co_mutex_unlock(&s->send_mutex);
>> +        nbd_recv_coroutines_wake_all(s);
>> +        s->wait_in_flight = true;
>> +        qemu_coroutine_yield();
>> +        s->wait_in_flight = false;
>> +        qemu_co_mutex_lock(&s->send_mutex);
>> +    }
>> +
>> +    qemu_co_mutex_unlock(&s->send_mutex);
>> +
>> +    if (!nbd_client_connecting(s)) {
>> +        return;
>> +    }
>> +
>> +    /*
>> +     * Now we are sure, that nobody accessing the channel now and nobody
>> +     * will try to access the channel, until we set state to CONNECTED
> 
> Now we are sure that nobody is accessing the channel, and no one will
> try until we set the state to CONNECTED
> 
>> +     */
>> +
>> +    /* Finalize previous connection if any */
>> +    if (s->ioc) {
>> +        nbd_client_detach_aio_context(con->bs);
>> +        object_unref(OBJECT(s->sioc));
>> +        s->sioc = NULL;
>> +        object_unref(OBJECT(s->ioc));
>> +        s->ioc = NULL;
>> +    }
>> +
>> +    s->connect_status = nbd_client_connect(con->bs, con->saddr,
>> +                                           con->export, con->tlscreds,
>> +                                           con->hostname, con->x_dirty_bitmap,
>> +                                           &local_err);
>> +    error_free(s->connect_err);
>> +    s->connect_err = NULL;
>> +    error_propagate(&s->connect_err, local_err);
>> +    local_err = NULL;
>> +
>> +    if (s->connect_status < 0) {
>> +        /* failed attempt */
>> +        return;
>> +    }
>> +
>> +    /* successfully connected */
>> +    s->state = NBD_CLIENT_CONNECTED;
>> +    qemu_co_queue_restart_all(&s->free_sema);
>> +}
>> +
>> +static coroutine_fn void nbd_reconnect_loop(NBDConnection *con)
>> +{
>> +    NBDClientSession *s = nbd_get_client_session(con->bs);
>> +    uint64_t start_time_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>> +    uint64_t delay_ns = s->reconnect_delay * 1000000000UL;
> 
> Do we have a #define constant for nanoseconds in a second to make this
> more legible than counting '0's?
> 
>> +    uint64_t timeout = 1000000000UL; /* 1 sec */
>> +    uint64_t max_timeout = 16000000000UL; /* 16 sec */
> 
> 1 * constant, 16 * constant

OK, if we have not one, I can at least add it locally

> 
>> +
>> +    nbd_reconnect_attempt(con);
>> +
>> +    while (nbd_client_connecting(s)) {
>> +        if (s->state == NBD_CLIENT_CONNECTING_WAIT &&
>> +            qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start_time_ns > delay_ns)
>> +        {
>> +            s->state = NBD_CLIENT_CONNECTING_NOWAIT;
>> +            qemu_co_queue_restart_all(&s->free_sema);
>> +        }
>> +
>> +        bdrv_dec_in_flight(con->bs);
>> +        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, timeout);
> 
> Another place where I'd like someone more familiar with coroutines to
> also have a look.
> 
>> +        bdrv_inc_in_flight(con->bs);
>> +        if (timeout < max_timeout) {
>> +            timeout *= 2;
>> +        }
>> +
>> +        nbd_reconnect_attempt(con);
>> +    }
>>   }
>>   
>>   static coroutine_fn void nbd_connection_entry(void *opaque)
>>   {
>> -    NBDClientSession *s = opaque;
>> +    NBDConnection *con = opaque;
>> +    NBDClientSession *s = nbd_get_client_session(con->bs);
>>       uint64_t i;
>>       int ret = 0;
>>       Error *local_err = NULL;
>> @@ -91,16 +218,25 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
>>            * Therefore we keep an additional in_flight reference all the time and
>>            * only drop it temporarily here.
>>            */
>> +
>> +        if (nbd_client_connecting(s)) {
>> +            nbd_reconnect_loop(con);
>> +        }
>> +
>> +        if (s->state != NBD_CLIENT_CONNECTED) {
>> +            continue;
>> +        }
>> +
> 
> If I understand, this is not a busy loop because nbd_reconnect_loop()
> will pause the coroutine until something interesting happens. Correct?

At least it will do qemu_co_sleep_ns.

> 
> It's late enough at night for me that I don't trust this to be a full
> review; I'll try and look again soon in more details, as well as play
> with this against iotests.
> 

Thank you!

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect

[Vladimir Sementsov-Ogievskiy]

07.06.2019 6:17, Eric Blake wrote:
>> +typedef struct NBDConnection {
>> +    BlockDriverState *bs;
>> +    SocketAddress *saddr;
>> +    const char *export;
>> +    QCryptoTLSCreds *tlscreds;
>> +    const char *hostname;
>> +    const char *x_dirty_bitmap;
>> +} NBDConnection;
> Can we put this type in a header, and use it instead of passing lots of
> individual parameters to nbd_client_connect()?  Probably as a separate
> pre-requisite cleanup patch.
> 


Hmm, and then, include it into BDRVNBDState? I don't remember why didn't do
it, and now I don't see any reason for it. We store this information anyway
for the whole life of the driver..

So, if I'm going to refactor it, I have a question: is there a reason for
layered BDRVNBDState:

typedef struct BDRVNBDState {
     NBDClientSession client;

     /* For nbd_refresh_filename() */
     SocketAddress *saddr;
     char *export, *tlscredsid;
} BDRVNBDState;

and use nbd_get_client_session everywhere instead of simple converting void *opaque
to State pointer like in qcow2 for example?

The only reason I can imagine is not to share the whole BDRVNBDState, and keep
things we are using only in nbd.c to be available only in nbd.c.. But I've put
saddr and export to NBDConnection to be used in nbd-client.c anyway. So, I think
it's a good moment to flatten and share BDRVNBDState and drop nbd_get_client_session.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect

[Vladimir Sementsov-Ogievskiy]

10.06.2019 15:38, Vladimir Sementsov-Ogievskiy wrote:
> 07.06.2019 6:17, Eric Blake wrote:
>>> +typedef struct NBDConnection {
>>> +    BlockDriverState *bs;
>>> +    SocketAddress *saddr;
>>> +    const char *export;
>>> +    QCryptoTLSCreds *tlscreds;
>>> +    const char *hostname;
>>> +    const char *x_dirty_bitmap;
>>> +} NBDConnection;
>> Can we put this type in a header, and use it instead of passing lots of
>> individual parameters to nbd_client_connect()?  Probably as a separate
>> pre-requisite cleanup patch.
>>
> 
> 
> Hmm, and then, include it into BDRVNBDState? I don't remember why didn't do
> it, and now I don't see any reason for it. We store this information anyway
> for the whole life of the driver..
> 
> So, if I'm going to refactor it, I have a question: is there a reason for
> layered BDRVNBDState:
> 
> typedef struct BDRVNBDState {
>      NBDClientSession client;
> 
>      /* For nbd_refresh_filename() */
>      SocketAddress *saddr;
>      char *export, *tlscredsid;
> } BDRVNBDState;
> 
> and use nbd_get_client_session everywhere instead of simple converting void *opaque
> to State pointer like in qcow2 for example?
> 
> The only reason I can imagine is not to share the whole BDRVNBDState, and keep
> things we are using only in nbd.c to be available only in nbd.c.. But I've put
> saddr and export to NBDConnection to be used in nbd-client.c anyway. So, I think
> it's a good moment to flatten and share BDRVNBDState and drop nbd_get_client_session.
> 

And if we are here, what is exact purpose of splitting  nbd-client.c from nbd.c?

I see, it was long ago:

commit 2302c1cafb13df23938b098d9c6595de52ec2577
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Sun Dec 1 22:23:41 2013 +0100

     Split nbd block client code


But still, it doesn't explain..

And all this leads to noop wrappers like this

static int nbd_co_flush(BlockDriverState *bs)
{
     return nbd_client_co_flush(bs);
}

static void nbd_detach_aio_context(BlockDriverState *bs)
{
     nbd_client_detach_aio_context(bs);
}

static void nbd_attach_aio_context(BlockDriverState *bs,
                                    AioContext *new_context)
{
     nbd_client_attach_aio_context(bs, new_context);
}


or need of defining driver callbacks in header file, instead of keeping them together
with driver struct definition and static (like other block drivers)...


And names of these two files always confused me.. nbd.c is nbd client block driver, and
driver is defined here.. But we have nbd-client.c which defines nbd client driver too.
And we also have nbd/client.c (OK, this split I understand of course, but it increases
confusion anyway).

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 2193 bytes --]

On 6/10/19 8:29 AM, Vladimir Sementsov-Ogievskiy wrote:

>> Hmm, and then, include it into BDRVNBDState? I don't remember why didn't do
>> it, and now I don't see any reason for it. We store this information anyway
>> for the whole life of the driver..
>>
>> So, if I'm going to refactor it, I have a question: is there a reason for
>> layered BDRVNBDState:
>>
>> typedef struct BDRVNBDState {
>>      NBDClientSession client;
>>
>>      /* For nbd_refresh_filename() */
>>      SocketAddress *saddr;
>>      char *export, *tlscredsid;
>> } BDRVNBDState;
>>
>> and use nbd_get_client_session everywhere instead of simple converting void *opaque
>> to State pointer like in qcow2 for example?
>>
>> The only reason I can imagine is not to share the whole BDRVNBDState, and keep
>> things we are using only in nbd.c to be available only in nbd.c.. But I've put
>> saddr and export to NBDConnection to be used in nbd-client.c anyway. So, I think
>> it's a good moment to flatten and share BDRVNBDState and drop nbd_get_client_session.
>>
> 
> And if we are here, what is exact purpose of splitting  nbd-client.c from nbd.c?

I see no strong reasons to keep the separation. If a single large file
is easier to maintain than an arbitrary split between two files, I'm
open to the idea of a patch that undoes the split.


> or need of defining driver callbacks in header file, instead of keeping them together
> with driver struct definition and static (like other block drivers)...
> 
> 
> And names of these two files always confused me.. nbd.c is nbd client block driver, and
> driver is defined here.. But we have nbd-client.c which defines nbd client driver too.
> And we also have nbd/client.c (OK, this split I understand of course, but it increases
> confusion anyway).

I'm also toying with the idea of writing block/nbd.c to utilize the
relatively-new libnbd library, to see if there are any differences in
behavior by offloading NBD access to a 3rd-party library.  But that's
further down the road.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v6 7/7] iotests: test nbd reconnect

[Vladimir Sementsov-Ogievskiy]

Add test, which starts backup to nbd target and restarts nbd server
during backup.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/249        | 63 +++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/249.out    | 10 ++++++
 tests/qemu-iotests/group      |  1 +
 tests/qemu-iotests/iotests.py |  4 +++
 4 files changed, 78 insertions(+)
 create mode 100755 tests/qemu-iotests/249
 create mode 100644 tests/qemu-iotests/249.out

diff --git a/tests/qemu-iotests/249 b/tests/qemu-iotests/249
new file mode 100755
index 0000000000..a29a276207
--- /dev/null
+++ b/tests/qemu-iotests/249
@@ -0,0 +1,63 @@
+#!/usr/bin/env python
+#
+# Test nbd reconnect
+#
+# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import time
+
+import iotests
+from iotests import qemu_img_create, file_path, qemu_nbd_popen, log
+
+disk_a, disk_b, nbd_sock = file_path('disk_a', 'disk_b', 'nbd-sock')
+
+qemu_img_create('-f', iotests.imgfmt, disk_a, '5M')
+qemu_img_create('-f', iotests.imgfmt, disk_b, '5M')
+srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
+time.sleep(1)
+
+vm = iotests.VM().add_drive(disk_a)
+vm.launch()
+vm.hmp_qemu_io('drive0', 'write 0 5M')
+
+vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
+           **{'node_name': 'backup0',
+              'driver': 'raw',
+              'file': {'driver': 'nbd',
+                       'server': {'type': 'unix', 'path': nbd_sock},
+                       'reconnect-delay': 10}})
+vm.qmp_log('blockdev-backup', device='drive0', sync='full', target='backup0')
+
+time.sleep(1)
+log('Kill NBD server')
+srv.kill()
+
+jobs = vm.qmp('query-block-jobs')['return']
+if jobs and jobs[0]['offset'] < jobs[0]['len']:
+    log('Backup job is still in progress')
+
+time.sleep(1)
+
+log('Start NBD server')
+srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
+
+e = vm.event_wait('BLOCK_JOB_COMPLETED')
+log('Backup completed: {}'.format(e['data']['offset']))
+
+vm.qmp_log('blockdev-del', node_name='backup0')
+srv.kill()
+vm.shutdown()
diff --git a/tests/qemu-iotests/249.out b/tests/qemu-iotests/249.out
new file mode 100644
index 0000000000..15a82ddefb
--- /dev/null
+++ b/tests/qemu-iotests/249.out
@@ -0,0 +1,10 @@
+{"execute": "blockdev-add", "arguments": {"driver": "raw", "file": {"driver": "nbd", "reconnect-delay": 10, "server": {"path": "TEST_DIR/PID-nbd-sock", "type": "unix"}}, "node-name": "backup0"}}
+{"return": {}}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "sync": "full", "target": "backup0"}}
+{"return": {}}
+Kill NBD server
+Backup job is still in progress
+Start NBD server
+Backup completed: 5242880
+{"execute": "blockdev-del", "arguments": {"node-name": "backup0"}}
+{"return": {}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index bae7718380..7ac9a5ea4a 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -248,3 +248,4 @@
 246 rw auto quick
 247 rw auto quick
 248 rw auto quick
+249 rw auto quick
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 997dc910cb..3a1f16dbfd 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -218,6 +218,10 @@ def qemu_nbd_pipe(*args):
                           ' '.join(qemu_nbd_args + ['--fork'] + list(args))))
     return exitcode, subp.communicate()[0]
 
+def qemu_nbd_popen(*args):
+    '''Run qemu-nbd in daemon mode and return the parent's exit code'''
+    return subprocess.Popen(qemu_nbd_args + ['--persistent'] + list(args))
+
 def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
     '''Return True if two image files are identical'''
     return qemu_img('compare', '-f', fmt1,
-- 
2.18.0

[Qemu-devel] [PATCH v6 7/7] iotests: test nbd reconnect

[Vladimir Sementsov-Ogievskiy]

Add test, which starts backup to nbd target and restarts nbd server
during backup.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/249        | 63 +++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/249.out    | 10 ++++++
 tests/qemu-iotests/group      |  1 +
 tests/qemu-iotests/iotests.py |  4 +++
 4 files changed, 78 insertions(+)
 create mode 100755 tests/qemu-iotests/249
 create mode 100644 tests/qemu-iotests/249.out

diff --git a/tests/qemu-iotests/249 b/tests/qemu-iotests/249
new file mode 100755
index 0000000000..a29a276207
--- /dev/null
+++ b/tests/qemu-iotests/249
@@ -0,0 +1,63 @@
+#!/usr/bin/env python
+#
+# Test nbd reconnect
+#
+# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import time
+
+import iotests
+from iotests import qemu_img_create, file_path, qemu_nbd_popen, log
+
+disk_a, disk_b, nbd_sock = file_path('disk_a', 'disk_b', 'nbd-sock')
+
+qemu_img_create('-f', iotests.imgfmt, disk_a, '5M')
+qemu_img_create('-f', iotests.imgfmt, disk_b, '5M')
+srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
+time.sleep(1)
+
+vm = iotests.VM().add_drive(disk_a)
+vm.launch()
+vm.hmp_qemu_io('drive0', 'write 0 5M')
+
+vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
+           **{'node_name': 'backup0',
+              'driver': 'raw',
+              'file': {'driver': 'nbd',
+                       'server': {'type': 'unix', 'path': nbd_sock},
+                       'reconnect-delay': 10}})
+vm.qmp_log('blockdev-backup', device='drive0', sync='full', target='backup0')
+
+time.sleep(1)
+log('Kill NBD server')
+srv.kill()
+
+jobs = vm.qmp('query-block-jobs')['return']
+if jobs and jobs[0]['offset'] < jobs[0]['len']:
+    log('Backup job is still in progress')
+
+time.sleep(1)
+
+log('Start NBD server')
+srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
+
+e = vm.event_wait('BLOCK_JOB_COMPLETED')
+log('Backup completed: {}'.format(e['data']['offset']))
+
+vm.qmp_log('blockdev-del', node_name='backup0')
+srv.kill()
+vm.shutdown()
diff --git a/tests/qemu-iotests/249.out b/tests/qemu-iotests/249.out
new file mode 100644
index 0000000000..15a82ddefb
--- /dev/null
+++ b/tests/qemu-iotests/249.out
@@ -0,0 +1,10 @@
+{"execute": "blockdev-add", "arguments": {"driver": "raw", "file": {"driver": "nbd", "reconnect-delay": 10, "server": {"path": "TEST_DIR/PID-nbd-sock", "type": "unix"}}, "node-name": "backup0"}}
+{"return": {}}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "sync": "full", "target": "backup0"}}
+{"return": {}}
+Kill NBD server
+Backup job is still in progress
+Start NBD server
+Backup completed: 5242880
+{"execute": "blockdev-del", "arguments": {"node-name": "backup0"}}
+{"return": {}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index bae7718380..7ac9a5ea4a 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -248,3 +248,4 @@
 246 rw auto quick
 247 rw auto quick
 248 rw auto quick
+249 rw auto quick
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 997dc910cb..3a1f16dbfd 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -218,6 +218,10 @@ def qemu_nbd_pipe(*args):
                           ' '.join(qemu_nbd_args + ['--fork'] + list(args))))
     return exitcode, subp.communicate()[0]
 
+def qemu_nbd_popen(*args):
+    '''Run qemu-nbd in daemon mode and return the parent's exit code'''
+    return subprocess.Popen(qemu_nbd_args + ['--persistent'] + list(args))
+
 def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
     '''Return True if two image files are identical'''
     return qemu_img('compare', '-f', fmt1,
-- 
2.18.0

Re: [Qemu-devel] [PATCH v6 0/7] NBD reconnect

[Vladimir Sementsov-Ogievskiy]

Ping

Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v6 0/7] NBD reconnect

[Vladimir Sementsov-Ogievskiy]

Ping

Best regards,
Vladimir

[Qemu-devel] ping Re: [PATCH v6 0/7] NBD reconnect

[Vladimir Sementsov-Ogievskiy]

ping

11.04.2019 20:27, Vladimir Sementsov-Ogievskiy wrote:
> Here is NBD reconnect. Previously, if connection failed all current
> and future requests will fail. After the series, nbd-client driver
> will try to reconnect unlimited times. During first @reconnect-delay
> seconds of reconnecting all requests will wait for the connection,
> and if it is established requests will be resent. After
> @reconnect-delay period all requests will be failed (until successful
> reconnect).
> 
> v4->v6:
>   (some preparing patches from v4 was merged as v5)
> 01: new
> 02: new
> 03: - drop unused things
>         - future states
>         - future s/break/continue/
> 04: - fix typos
>      - s/3.1/4.1/
>      - set 0 as default
> 05: new
> 06: - new states and s/break/continue/ moved here from 03
>      - drop NBDClientSession.receiving, as now in_flight
>        requests used instread
>      - add copyright
>      - go to NBD_CLIENT_CONNECTING_NOWAIT immediately if
>        reconnect_delay is 0 (so, reconnect_delay moved to
>        NBDClientSession)
>      - on close, do qemu_co_sleep_wake(client->connection_co),
>        to not wait for reconnect loop iteration
>      - handle state transition to QUIT during reconnect loop
>        (assert(nbd_client_connecting(s)) was bad idea)
>      - don't try to fail on protocol errors after
>        nbd_client_connect, as we can't distinguish them
>      - decrement in_flight around reconnect sleep to make
>        it possible to drain and exit during it
>        (v4 was based on something before in_flight logic
>         introduced into nbd-client)
>      - changed logic in nbd_client_attach_aio_context
> 07: - refactor, using log and qmp_log
>      - drop export name
>      - drop strange try/except
>      - add reconnect-delay option (as 0 is a default now)
> 
> 
> Vladimir Sementsov-Ogievskiy (7):
>    block/nbd-client: split connection_co start out of nbd_client_connect
>    block/nbd-client: use non-blocking io channel for nbd negotiation
>    block/nbd-client: move from quit to state
>    block/nbd: add cmdline and qapi parameter reconnect-delay
>    qemu-coroutine-sleep: introduce qemu_co_sleep_wake
>    block/nbd-client: nbd reconnect
>    iotests: test nbd reconnect
> 
>   qapi/block-core.json          |  12 +-
>   block/nbd-client.h            |  15 +-
>   include/block/nbd.h           |   3 +-
>   include/qemu/coroutine.h      |   6 +
>   block/nbd-client.c            | 416 +++++++++++++++++++++++++---------
>   block/nbd.c                   |  16 +-
>   nbd/client.c                  |  16 +-
>   qemu-nbd.c                    |   2 +-
>   util/qemu-coroutine-sleep.c   |  20 +-
>   tests/qemu-iotests/249        |  63 +++++
>   tests/qemu-iotests/249.out    |  10 +
>   tests/qemu-iotests/group      |   1 +
>   tests/qemu-iotests/iotests.py |   4 +
>   13 files changed, 468 insertions(+), 116 deletions(-)
>   create mode 100755 tests/qemu-iotests/249
>   create mode 100644 tests/qemu-iotests/249.out
> 


-- 
Best regards,
Vladimir

[Qemu-devel] ping Re: [PATCH v6 0/7] NBD reconnect

[Vladimir Sementsov-Ogievskiy]

ping

11.04.2019 20:27, Vladimir Sementsov-Ogievskiy wrote:
> Here is NBD reconnect. Previously, if connection failed all current
> and future requests will fail. After the series, nbd-client driver
> will try to reconnect unlimited times. During first @reconnect-delay
> seconds of reconnecting all requests will wait for the connection,
> and if it is established requests will be resent. After
> @reconnect-delay period all requests will be failed (until successful
> reconnect).
> 
> v4->v6:
>   (some preparing patches from v4 was merged as v5)
> 01: new
> 02: new
> 03: - drop unused things
>         - future states
>         - future s/break/continue/
> 04: - fix typos
>      - s/3.1/4.1/
>      - set 0 as default
> 05: new
> 06: - new states and s/break/continue/ moved here from 03
>      - drop NBDClientSession.receiving, as now in_flight
>        requests used instread
>      - add copyright
>      - go to NBD_CLIENT_CONNECTING_NOWAIT immediately if
>        reconnect_delay is 0 (so, reconnect_delay moved to
>        NBDClientSession)
>      - on close, do qemu_co_sleep_wake(client->connection_co),
>        to not wait for reconnect loop iteration
>      - handle state transition to QUIT during reconnect loop
>        (assert(nbd_client_connecting(s)) was bad idea)
>      - don't try to fail on protocol errors after
>        nbd_client_connect, as we can't distinguish them
>      - decrement in_flight around reconnect sleep to make
>        it possible to drain and exit during it
>        (v4 was based on something before in_flight logic
>         introduced into nbd-client)
>      - changed logic in nbd_client_attach_aio_context
> 07: - refactor, using log and qmp_log
>      - drop export name
>      - drop strange try/except
>      - add reconnect-delay option (as 0 is a default now)
> 
> 
> Vladimir Sementsov-Ogievskiy (7):
>    block/nbd-client: split connection_co start out of nbd_client_connect
>    block/nbd-client: use non-blocking io channel for nbd negotiation
>    block/nbd-client: move from quit to state
>    block/nbd: add cmdline and qapi parameter reconnect-delay
>    qemu-coroutine-sleep: introduce qemu_co_sleep_wake
>    block/nbd-client: nbd reconnect
>    iotests: test nbd reconnect
> 
>   qapi/block-core.json          |  12 +-
>   block/nbd-client.h            |  15 +-
>   include/block/nbd.h           |   3 +-
>   include/qemu/coroutine.h      |   6 +
>   block/nbd-client.c            | 416 +++++++++++++++++++++++++---------
>   block/nbd.c                   |  16 +-
>   nbd/client.c                  |  16 +-
>   qemu-nbd.c                    |   2 +-
>   util/qemu-coroutine-sleep.c   |  20 +-
>   tests/qemu-iotests/249        |  63 +++++
>   tests/qemu-iotests/249.out    |  10 +
>   tests/qemu-iotests/group      |   1 +
>   tests/qemu-iotests/iotests.py |   4 +
>   13 files changed, 468 insertions(+), 116 deletions(-)
>   create mode 100755 tests/qemu-iotests/249
>   create mode 100644 tests/qemu-iotests/249.out
> 


-- 
Best regards,
Vladimir