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

[Vladimir Sementsov-Ogievskiy]

11.05.2021 23:54, Roman Kagan wrote:
> 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
> > 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(-)
> >
> > diff --git a/include/block/nbd.h b/include/block/nbd.h
> > index 5d86e6a393..5bb54d831c 100644
> > --- a/include/block/nbd.h
> > +++ b/include/block/nbd.h
> > @@ -409,6 +409,8 @@ const char *nbd_err_lookup(int err);
> >   /* nbd/client-connection.c */
> >   typedef struct NBDClientConnection NBDClientConnection;
> >   
> > +void nbd_client_connection_enable_retry(NBDClientConnection *conn);
> > +
> >   NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr,
> >                                                  bool do_negotiation,
> >                                                  const char *export_name,
> > diff --git a/nbd/client-connection.c b/nbd/client-connection.c
> > index ae4a77f826..002bd91f42 100644
> > --- a/nbd/client-connection.c
> > +++ 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
> > + * 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;
> > +
> > +        if (ret < 0) {
> > +            object_unref(OBJECT(conn->sioc));
> > +            conn->sioc = NULL;
> > +            if (conn->do_retry) {
> > +                sleep(timeout);
> > +                if (timeout < max_timeout) {
> > +                    timeout *= 2;
> > +                }
> > +                continue;
> > +            }
> > +        }
>
> How is it supposed to get canceled?

Next commit does it
 
> > -    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);
> >       }
> > --
> > 2.29.2


--
Best regards,
Vladimir