[Qemu-devel] [RFC 0/3] NBD reconnect

[Qemu-devel] [RFC 0/3] NBD reconnect

[Vladimir Sementsov-Ogievskiy]

Hi all.

Here is a draft of NBD reconnect, to negotiate overall design. It is done
through implementing new error action: reconnect.

New action works as follows:

Firstly, not stopping the vm, it tries to bdrv_reconnect several times
with given pause. Then, if we failed to reconnect fallthrough to 'stop'
error action.

My RFC doubts:
1. introducing new .bdrv_reconnect(). Is bdrv_reopen() appropriate for
   the feature? I'm not sure, because it is not recursive, and don't
   really reopen files..
2. introducing new error action. Anyway, I'll have to add some configuration
   parameters (see 03), so, the feature may be just an option to 'stop'
   action, rather than separate error action.

Vladimir Sementsov-Ogievskiy (3):
  block: add bdrv_reconnect
  nbd: add .bdrv_reconnect handler
  blk: add 'reconnect' error action

 qapi/block-core.json      |  4 ++--
 include/block/block.h     |  2 ++
 include/block/block_int.h |  3 +++
 block.c                   | 22 +++++++++++++++++
 block/block-backend.c     | 48 ++++++++++++++++++++++++++++++++++++-
 block/nbd.c               | 60 ++++++++++++++++++++++++++++++++++-------------
 hw/scsi/scsi-disk.c       |  4 +++-
 7 files changed, 123 insertions(+), 20 deletions(-)

-- 
2.11.1

[Qemu-devel] [RFC 1/3] block: add bdrv_reconnect

[Vladimir Sementsov-Ogievskiy]

It will be used to reconnect NBD connection.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block.h     |  2 ++
 include/block/block_int.h |  3 +++
 block.c                   | 22 ++++++++++++++++++++++
 3 files changed, 27 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index cdec3639a3..912e3f3dcc 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -604,4 +604,6 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
  */
 void bdrv_register_buf(BlockDriverState *bs, void *host, size_t size);
 void bdrv_unregister_buf(BlockDriverState *bs, void *host);
+
+int bdrv_reconnect(BlockDriverState *bs, Error **errp);
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c4dd1d4bb8..ab9018f1c4 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -475,6 +475,9 @@ struct BlockDriver {
      */
     void (*bdrv_register_buf)(BlockDriverState *bs, void *host, size_t size);
     void (*bdrv_unregister_buf)(BlockDriverState *bs, void *host);
+
+    int (*bdrv_reconnect)(BlockDriverState *bs, Error **errp);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
diff --git a/block.c b/block.c
index a2caadf0a0..fab4413d59 100644
--- a/block.c
+++ b/block.c
@@ -4095,6 +4095,28 @@ int bdrv_has_zero_init(BlockDriverState *bs)
     return 0;
 }
 
+int bdrv_reconnect(BlockDriverState *bs, Error **errp)
+{
+    int ret;
+
+    if (bs->drv && bs->drv->bdrv_reconnect) {
+        return bs->drv->bdrv_reconnect(bs, errp);
+    }
+
+    if (bs->backing) {
+        ret = bdrv_reconnect(bs->backing->bs, errp);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
+    if (bs->file) {
+        return bdrv_reconnect(bs->file->bs, errp);
+    }
+
+    return 0;
+}
+
 bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs)
 {
     BlockDriverInfo bdi;
-- 
2.11.1

[Qemu-devel] [RFC 2/3] nbd: add .bdrv_reconnect handler

[Vladimir Sementsov-Ogievskiy]

Add handler, which reconnects to NBD server. For it:

 - separate connection code to nbd_reconnect()
 - store tlscreds and hostname in BDRVNBDState

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

diff --git a/block/nbd.c b/block/nbd.c
index 1e2b3ba2d3..371341dbcf 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -48,6 +48,10 @@ typedef struct BDRVNBDState {
     /* For nbd_refresh_filename() */
     SocketAddress *saddr;
     char *export, *tlscredsid;
+
+    /* For nbd_reconnect() */
+    QCryptoTLSCreds *tlscreds;
+    const char *hostname;
 } BDRVNBDState;
 
 static int nbd_parse_uri(const char *filename, QDict *options)
@@ -392,6 +396,33 @@ static QemuOptsList nbd_runtime_opts = {
     },
 };
 
+static int nbd_reconnect(BlockDriverState *bs, Error **errp)
+{
+    int ret;
+    BDRVNBDState *s = bs->opaque;
+    QIOChannelSocket *sioc;
+
+    /* close current connection if any */
+    nbd_client_close(bs);
+
+    memset(&s->client, 0, sizeof(s->client));
+    s->client.quit = true;
+
+    sioc = nbd_establish_connection(s->saddr, errp);
+    if (!sioc) {
+        return -ECONNREFUSED;
+    }
+
+    ret = nbd_client_init(bs, sioc, s->export, s->tlscreds, s->hostname, errp);
+    object_unref(OBJECT(sioc));
+    if (ret < 0) {
+        return ret;
+    }
+
+    s->client.quit = false;
+    return ret;
+}
+
 static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
@@ -399,8 +430,6 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
     QemuOpts *opts = NULL;
     Error *local_err = NULL;
     QIOChannelSocket *sioc = NULL;
-    QCryptoTLSCreds *tlscreds = NULL;
-    const char *hostname = NULL;
     int ret = -EINVAL;
 
     opts = qemu_opts_create(&nbd_runtime_opts, NULL, 0, &error_abort);
@@ -425,8 +454,8 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
 
     s->tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds"));
     if (s->tlscredsid) {
-        tlscreds = nbd_get_tls_creds(s->tlscredsid, errp);
-        if (!tlscreds) {
+        s->tlscreds = nbd_get_tls_creds(s->tlscredsid, errp);
+        if (!s->tlscreds) {
             goto error;
         }
 
@@ -435,29 +464,22 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
             error_setg(errp, "TLS only supported over IP sockets");
             goto error;
         }
-        hostname = s->saddr->u.inet.host;
+        s->hostname = s->saddr->u.inet.host;
     }
 
     /* establish TCP connection, return error if it fails
      * TODO: Configurable retry-until-timeout behaviour.
      */
-    sioc = nbd_establish_connection(s->saddr, errp);
-    if (!sioc) {
-        ret = -ECONNREFUSED;
-        goto error;
-    }
+    ret = nbd_reconnect(bs, errp);
 
-    /* NBD handshake */
-    ret = nbd_client_init(bs, sioc, s->export,
-                          tlscreds, hostname, errp);
  error:
     if (sioc) {
         object_unref(OBJECT(sioc));
     }
-    if (tlscreds) {
-        object_unref(OBJECT(tlscreds));
-    }
     if (ret < 0) {
+        if (s->tlscreds) {
+            object_unref(OBJECT(s->tlscreds));
+        }
         qapi_free_SocketAddress(s->saddr);
         g_free(s->export);
         g_free(s->tlscredsid);
@@ -494,6 +516,9 @@ static void nbd_close(BlockDriverState *bs)
 
     nbd_client_close(bs);
 
+    if (s->tlscreds) {
+        object_unref(OBJECT(s->tlscreds));
+    }
     qapi_free_SocketAddress(s->saddr);
     g_free(s->export);
     g_free(s->tlscredsid);
@@ -574,6 +599,7 @@ static BlockDriver bdrv_nbd = {
     .instance_size              = sizeof(BDRVNBDState),
     .bdrv_parse_filename        = nbd_parse_filename,
     .bdrv_file_open             = nbd_open,
+    .bdrv_reconnect             = nbd_reconnect,
     .bdrv_co_preadv             = nbd_client_co_preadv,
     .bdrv_co_pwritev            = nbd_client_co_pwritev,
     .bdrv_co_pwrite_zeroes      = nbd_client_co_pwrite_zeroes,
@@ -594,6 +620,7 @@ static BlockDriver bdrv_nbd_tcp = {
     .instance_size              = sizeof(BDRVNBDState),
     .bdrv_parse_filename        = nbd_parse_filename,
     .bdrv_file_open             = nbd_open,
+    .bdrv_reconnect             = nbd_reconnect,
     .bdrv_co_preadv             = nbd_client_co_preadv,
     .bdrv_co_pwritev            = nbd_client_co_pwritev,
     .bdrv_co_pwrite_zeroes      = nbd_client_co_pwrite_zeroes,
@@ -614,6 +641,7 @@ static BlockDriver bdrv_nbd_unix = {
     .instance_size              = sizeof(BDRVNBDState),
     .bdrv_parse_filename        = nbd_parse_filename,
     .bdrv_file_open             = nbd_open,
+    .bdrv_reconnect             = nbd_reconnect,
     .bdrv_co_preadv             = nbd_client_co_preadv,
     .bdrv_co_pwritev            = nbd_client_co_pwritev,
     .bdrv_co_pwrite_zeroes      = nbd_client_co_pwrite_zeroes,
-- 
2.11.1

[Qemu-devel] [RFC 3/3] blk: add 'reconnect' error action

[Vladimir Sementsov-Ogievskiy]

New action works as follows:

Firstly, not stopping the vm, it tries to bdrv_reconnect several times
with given pause. Then, if we failed to reconnect fallthrough to 'stop'
error action.

TODO:
 - qapi docs
 - support other disks (only scsi here)
 - support block jobs
 - add configuration of timeout and tries count parameters

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json  |  4 ++--
 block/block-backend.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 hw/scsi/scsi-disk.c   |  4 +++-
 3 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index c50517bff3..d4d87dbd4f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1028,7 +1028,7 @@
 # Since: 1.3
 ##
 { 'enum': 'BlockdevOnError',
-  'data': ['report', 'ignore', 'enospc', 'stop', 'auto'] }
+  'data': ['report', 'ignore', 'enospc', 'stop', 'auto', 'reconnect'] }
 
 ##
 # @MirrorSyncMode:
@@ -4351,7 +4351,7 @@
 # Since: 2.1
 ##
 { 'enum': 'BlockErrorAction',
-  'data': [ 'ignore', 'report', 'stop' ] }
+  'data': [ 'ignore', 'report', 'stop', 'reconnect' ] }
 
 
 ##
diff --git a/block/block-backend.c b/block/block-backend.c
index 681b240b12..81eb9a7bd0 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -89,6 +89,11 @@ struct BlockBackend {
      */
     unsigned int in_flight;
     AioWait wait;
+
+    bool reconnect_failed; /* TODO: worth tri-state variable? */
+    bool reconnecting;
+    unsigned int reconnect_max;
+    uint64_t reconnect_ns;
 };
 
 typedef struct BlockBackendAIOCB {
@@ -322,6 +327,8 @@ BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm)
     blk->refcnt = 1;
     blk->perm = perm;
     blk->shared_perm = shared_perm;
+    blk->reconnect_max = 10; /* TODO configure */
+    blk->reconnect_ns = 5000000000; /* 5 seconds, TODO configure */
     blk_set_enable_write_cache(blk, true);
 
     block_acct_init(&blk->stats);
@@ -1079,6 +1086,7 @@ void blk_iostatus_disable(BlockBackend *blk)
 
 void blk_iostatus_reset(BlockBackend *blk)
 {
+    blk->reconnect_failed = false;
     if (blk_iostatus_is_enabled(blk)) {
         BlockDriverState *bs = blk_bs(blk);
         blk->iostatus = BLOCK_DEVICE_IO_STATUS_OK;
@@ -1635,6 +1643,9 @@ BlockErrorAction blk_get_error_action(BlockBackend *blk, bool is_read,
     BlockdevOnError on_err = blk_get_on_error(blk, is_read);
 
     switch (on_err) {
+    case BLOCKDEV_ON_ERROR_RECONNECT:
+        return blk->reconnect_failed ? BLOCK_ERROR_ACTION_STOP :
+                                       BLOCK_ERROR_ACTION_RECONNECT;
     case BLOCKDEV_ON_ERROR_ENOSPC:
         return (error == ENOSPC) ?
                BLOCK_ERROR_ACTION_STOP : BLOCK_ERROR_ACTION_REPORT;
@@ -1665,6 +1676,29 @@ static void send_qmp_error_event(BlockBackend *blk,
                                    &error_abort);
 }
 
+
+static void coroutine_fn blk_reconnect_co(void *opaque)
+{
+    BlockBackend *blk = opaque;
+    int i;
+
+    for (i = 0; i < blk->reconnect_max; i++) {
+        int ret;
+
+        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, blk->reconnect_ns);
+
+        ret = bdrv_reconnect(blk_bs(blk), NULL);
+        if (ret == 0) {
+            blk->reconnecting = false;
+            blk_iostatus_reset(blk);
+            return;
+        }
+    }
+
+    blk->reconnecting = false;
+    blk->reconnect_failed = true;
+}
+
 /* This is done by device models because, while the block layer knows
  * about the error, it does not know whether an operation comes from
  * the device or the block layer (from a job, for example).
@@ -1674,7 +1708,19 @@ void blk_error_action(BlockBackend *blk, BlockErrorAction action,
 {
     assert(error >= 0);
 
-    if (action == BLOCK_ERROR_ACTION_STOP) {
+    if (action == BLOCK_ERROR_ACTION_RECONNECT) {
+        Coroutine *co;
+        blk_iostatus_set_err(blk, error);
+
+        if (blk->reconnecting || blk->reconnect_failed) {
+            return;
+        }
+
+        blk->reconnecting = true;
+
+        co = qemu_coroutine_create(blk_reconnect_co, blk);
+        aio_co_enter(blk_get_aio_context(blk), co);
+    } else if (action == BLOCK_ERROR_ACTION_STOP) {
         /* First set the iostatus, so that "info block" returns an iostatus
          * that matches the events raised so far (an additional error iostatus
          * is fine, but not a lost one).
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index ded23d36ca..f1c166dfda 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -474,7 +474,9 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
     }
 
     blk_error_action(s->qdev.conf.blk, action, is_read, error);
-    if (action == BLOCK_ERROR_ACTION_STOP) {
+    if (action == BLOCK_ERROR_ACTION_STOP ||
+        action == BLOCK_ERROR_ACTION_RECONNECT)
+    {
         scsi_req_retry(&r->req);
     }
     return action != BLOCK_ERROR_ACTION_IGNORE;
-- 
2.11.1

Re: [Qemu-devel] [RFC 0/3] NBD reconnect

[Vladimir Sementsov-Ogievskiy]

24.04.2018 16:08, Vladimir Sementsov-Ogievskiy wrote:
> Hi all.
>
> Here is a draft of NBD reconnect, to negotiate overall design. It is done
> through implementing new error action: reconnect.
>
> New action works as follows:
>
> Firstly, not stopping the vm, it tries to bdrv_reconnect several times
> with given pause. Then, if we failed to reconnect fallthrough to 'stop'
> error action.
>
> My RFC doubts:
> 1. introducing new .bdrv_reconnect(). Is bdrv_reopen() appropriate for
>     the feature? I'm not sure, because it is not recursive, and don't
>     really reopen files..
> 2. introducing new error action. Anyway, I'll have to add some configuration
>     parameters (see 03), so, the feature may be just an option to 'stop'
>     action, rather than separate error action.
>
> Vladimir Sementsov-Ogievskiy (3):
>    block: add bdrv_reconnect
>    nbd: add .bdrv_reconnect handler
>    blk: add 'reconnect' error action
>
>   qapi/block-core.json      |  4 ++--
>   include/block/block.h     |  2 ++
>   include/block/block_int.h |  3 +++
>   block.c                   | 22 +++++++++++++++++
>   block/block-backend.c     | 48 ++++++++++++++++++++++++++++++++++++-
>   block/nbd.c               | 60 ++++++++++++++++++++++++++++++++++-------------
>   hw/scsi/scsi-disk.c       |  4 +++-
>   7 files changed, 123 insertions(+), 20 deletions(-)
>

Hm, answering myself:

it's a bad design:

1. Assume we have a tree of nodes. So, if one node was disconnected, we 
will reconnect all nodes. It's better to reconnect only one node.
2. Possibility to reconnect looks more like option, rather than separate 
error_action, as we may want reconnect before stop in _STOP action, and 
reconnect if disconnected with _NOSPC error action.

So, I'll try to rewrite it to be a simple nbd-driver option.

-- 
Best regards,
Vladimir