Re: [PATCH v3 17/33] nbd/client-connection: implement connection retry

[Eric Blake]

On Fri, Apr 16, 2021 at 11:08:55AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Add an option for thread to retry connection until success. We'll use

for a thread to retry connection until it succeeds.

> nbd/client-connection both for reconnect and for initial connection in
> nbd_open(), so we need a possibility to use same NBDClientConnection
> instance to connect once in nbd_open() and then use retry semantics for
> reconnect.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/nbd.h     |  2 ++
>  nbd/client-connection.c | 55 +++++++++++++++++++++++++++++------------
>  2 files changed, 41 insertions(+), 16 deletions(-)
> 
> +++ b/nbd/client-connection.c
> @@ -36,6 +36,8 @@ struct NBDClientConnection {
>      NBDExportInfo initial_info;
>      bool do_negotiation;
>  
> +    bool do_retry;
> +
>      /*
>       * Result of last attempt. Valid in FAIL and SUCCESS states.
>       * If you want to steal error, don't forget to set pointer to NULL.
> @@ -52,6 +54,15 @@ struct NBDClientConnection {
>      Coroutine *wait_co; /* nbd_co_establish_connection() wait in yield() */
>  };
>  
> +/*
> + * The function isn't protected by any mutex, so call it when thread is not

so only call it when the thread is not yet running

or maybe even

only call it when the client connection attempt has not yet started

> + * running.
> + */
> +void nbd_client_connection_enable_retry(NBDClientConnection *conn)
> +{
> +    conn->do_retry = true;
> +}
> +
>  NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr,
>                                                 bool do_negotiation,
>                                                 const char *export_name,
> @@ -144,24 +155,37 @@ static void *connect_thread_func(void *opaque)
>      NBDClientConnection *conn = opaque;
>      bool do_free;
>      int ret;
> +    uint64_t timeout = 1;
> +    uint64_t max_timeout = 16;
> +
> +    while (true) {
> +        conn->sioc = qio_channel_socket_new();
> +
> +        error_free(conn->err);
> +        conn->err = NULL;
> +        conn->updated_info = conn->initial_info;
> +
> +        ret = nbd_connect(conn->sioc, conn->saddr,
> +                          conn->do_negotiation ? &conn->updated_info : NULL,
> +                          conn->tlscreds, &conn->ioc, &conn->err);
> +        conn->updated_info.x_dirty_bitmap = NULL;
> +        conn->updated_info.name = NULL;

I'm not quite sure I follow the allocation here: if we passed in
&conn->updated_info which got modified in-place by nbd_connect, then
are we risking a memory leak by ignoring the x_dirty_bitmap and name
set by that call?

> +
> +        if (ret < 0) {
> +            object_unref(OBJECT(conn->sioc));
> +            conn->sioc = NULL;
> +            if (conn->do_retry) {
> +                sleep(timeout);

This is a bare sleep in a function not marked as coroutine_fn.  Do we
need to instead use coroutine sleep for better response to an early
exit if initialization is taking too long?

> +                if (timeout < max_timeout) {
> +                    timeout *= 2;
> +                }
> +                continue;
> +            }
> +        }
>  
> -    conn->sioc = qio_channel_socket_new();
> -
> -    error_free(conn->err);
> -    conn->err = NULL;
> -    conn->updated_info = conn->initial_info;
> -
> -    ret = nbd_connect(conn->sioc, conn->saddr,
> -                      conn->do_negotiation ? &conn->updated_info : NULL,
> -                      conn->tlscreds, &conn->ioc, &conn->err);
> -    if (ret < 0) {
> -        object_unref(OBJECT(conn->sioc));
> -        conn->sioc = NULL;
> +        break;
>      }
>  
> -    conn->updated_info.x_dirty_bitmap = NULL;
> -    conn->updated_info.name = NULL;
> -
>      WITH_QEMU_LOCK_GUARD(&conn->mutex) {
>          assert(conn->running);
>          conn->running = false;
> @@ -172,7 +196,6 @@ static void *connect_thread_func(void *opaque)
>          do_free = conn->detached;
>      }
>  
> -
>      if (do_free) {
>          nbd_client_connection_do_free(conn);

Spurious hunk?

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