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

[Vladimir Sementsov-Ogievskiy]

03.06.2021 19:17, Eric Blake 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
>
> 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?

Yes, that looks strange :\. Will check when prepare new version and fix or 
leave a comment here.

> > +
> > +        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?

We are in a separate, by-hand created thread, which knows nothing about 
coroutines, iothreads, aio contexts etc.. I think bare sleep is what should be 
here.

> > +                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?

wull drop

--
Best regards,
Vladimir