[PATCH 00/14] nbd: move reconnect-thread to separate file

[PATCH 00/14] nbd: move reconnect-thread to separate file

[Vladimir Sementsov-Ogievskiy]

Hi all!

There are problems with nbd driver:

 - nbd reconnect is cancelled on drain, which is bad as Roman describes
   in his "[PATCH 0/7] block/nbd: decouple reconnect from drain"
 - nbd driver is too complicated around drained sections and aio context
   switch. It's nearly impossible to follow all the logic, including
   abuse of bs->in_flight, which is temporary decreased in some places
   (like nbd_read_eof()). Additional reconnect thread and two different
   state machines (we have BDRVNBDState::state and
   BDRVNBDState::connect_thread->state) doesn't make things simpler :)

So, I have a plan:

1. Move nbd negotiation to connect_thread

2. Do receive NBD replies in request coroutines, not in connection_co
  
   At this point we can drop connection_co, and when we don't have
   endless running coroutine, NBD driver becomes a usual block driver,
   and we can drop abuse of bs->in_flight, and probably drop most of
   complicated logic around drained section and aio context switch in
   nbd driver.

3. Still, as Roman describes, with [2] we loose a possibility to
   reconnect immediately when connection breaks (with current code we
   have endless read in reconnect_co, but actually for this to work
   keep-alive should be setup correctly). So, we'll need to reinvent it,
   checking connection periodically by timeout, with help of getsockopt
   or just sending a kind of PING request (zero-length READ or something
   like this).

And this series a kind of preparation. The main point of it is moving
connect-thread to a separate file.

This series may crash on iotest 277. So, it's based on corresponding
fix: "[PATCH 1/7] block/nbd: avoid touching freed connect_thread":

Based-on: <20210315060611.2989049-2-rvkagan@yandex-team.ru>

Vladimir Sementsov-Ogievskiy (14):
  block/nbd: BDRVNBDState: drop unused connect_err
  block/nbd: nbd_co_establish_connection(): drop unused errp
  block/nbd: drop unused NBDConnectThread::err field
  block/nbd: split connect_thread_cb() out of connect_thread_func()
  block/nbd: rename NBDConnectThread to NBDConnectCB
  block/nbd: further segregation of connect-thread
  block/nbd: drop nbd_free_connect_thread()
  block/nbd: move nbd connect-thread to nbd/client-connect.c
  block/nbd: NBDConnectCB: drop bh_* fields
  block/nbd: move wait_connect field under mutex protection
  block/nbd: refactor connect_bh()
  block/nbd: refactor nbd_co_establish_connection
  block/nbd: nbd_co_establish_connection_cancel(): rename wake to
    do_wake
  block/nbd: drop thr->state

 include/block/nbd.h  |   6 +
 block/nbd.c          | 266 ++++++++++++++-----------------------------
 nbd/client-connect.c |  72 ++++++++++++
 nbd/meson.build      |   1 +
 4 files changed, 162 insertions(+), 183 deletions(-)
 create mode 100644 nbd/client-connect.c

-- 
2.29.2

[PATCH 01/14] block/nbd: BDRVNBDState: drop unused connect_err

[Vladimir Sementsov-Ogievskiy]

The field is actually unused. Let's make things a bit simpler dropping
it and corresponding logic.

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

diff --git a/block/nbd.c b/block/nbd.c
index c26dc5a54f..a47d6cfea3 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -122,7 +122,6 @@ typedef struct BDRVNBDState {
     int in_flight;
     NBDClientState state;
     int connect_status;
-    Error *connect_err;
     bool wait_in_flight;
 
     QEMUTimer *reconnect_delay_timer;
@@ -578,7 +577,6 @@ static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
 static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
 {
     int ret;
-    Error *local_err = NULL;
 
     if (!nbd_client_connecting(s)) {
         return;
@@ -619,14 +617,14 @@ static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
         s->ioc = NULL;
     }
 
-    if (nbd_co_establish_connection(s->bs, &local_err) < 0) {
+    if (nbd_co_establish_connection(s->bs, NULL) < 0) {
         ret = -ECONNREFUSED;
         goto out;
     }
 
     bdrv_dec_in_flight(s->bs);
 
-    ret = nbd_client_handshake(s->bs, &local_err);
+    ret = nbd_client_handshake(s->bs, NULL);
 
     if (s->drained) {
         s->wait_drained_end = true;
@@ -642,9 +640,6 @@ static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
 
 out:
     s->connect_status = ret;
-    error_free(s->connect_err);
-    s->connect_err = NULL;
-    error_propagate(&s->connect_err, local_err);
 
     if (ret >= 0) {
         /* successfully connected */
-- 
2.29.2

Re: [PATCH 01/14] block/nbd: BDRVNBDState: drop unused connect_err

[Roman Kagan]

On Wed, Apr 07, 2021 at 01:46:24PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> The field is actually unused. Let's make things a bit simpler dropping
> it and corresponding logic.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/nbd.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index c26dc5a54f..a47d6cfea3 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -122,7 +122,6 @@ typedef struct BDRVNBDState {
>      int in_flight;
>      NBDClientState state;
>      int connect_status;

This field is write-only too.  Do you have any plans on using it later?
Otherwise you may want to nuke it in this patch too.

Roman.

[PATCH 02/14] block/nbd: nbd_co_establish_connection(): drop unused errp

[Vladimir Sementsov-Ogievskiy]

We are going to refactor connection logic to make it more
understandable. Every bit that we can simplify in advance will help.
Drop errp for now, it's unused anyway. We'll probably reimplement it in
future.

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

diff --git a/block/nbd.c b/block/nbd.c
index a47d6cfea3..29c33338bf 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -145,7 +145,7 @@ typedef struct BDRVNBDState {
 
 static int nbd_establish_connection(BlockDriverState *bs, SocketAddress *saddr,
                                     Error **errp);
-static int nbd_co_establish_connection(BlockDriverState *bs, Error **errp);
+static int nbd_co_establish_connection(BlockDriverState *bs);
 static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
                                                bool detach);
 static int nbd_client_handshake(BlockDriverState *bs, Error **errp);
@@ -435,7 +435,7 @@ static void *connect_thread_func(void *opaque)
 }
 
 static int coroutine_fn
-nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
+nbd_co_establish_connection(BlockDriverState *bs)
 {
     int ret;
     QemuThread thread;
@@ -491,7 +491,7 @@ nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
     case CONNECT_THREAD_SUCCESS:
     case CONNECT_THREAD_FAIL:
         thr->state = CONNECT_THREAD_NONE;
-        error_propagate(errp, thr->err);
+        error_free(thr->err);
         thr->err = NULL;
         s->sioc = thr->sioc;
         thr->sioc = NULL;
@@ -509,7 +509,6 @@ nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
          * result may be used for next connection attempt.
          */
         ret = -1;
-        error_setg(errp, "Connection attempt cancelled by other operation");
         break;
 
     case CONNECT_THREAD_NONE:
@@ -617,7 +616,7 @@ static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
         s->ioc = NULL;
     }
 
-    if (nbd_co_establish_connection(s->bs, NULL) < 0) {
+    if (nbd_co_establish_connection(s->bs) < 0) {
         ret = -ECONNREFUSED;
         goto out;
     }
-- 
2.29.2

Re: [PATCH 02/14] block/nbd: nbd_co_establish_connection(): drop unused errp

[Roman Kagan]

On Wed, Apr 07, 2021 at 01:46:25PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We are going to refactor connection logic to make it more
> understandable. Every bit that we can simplify in advance will help.
> Drop errp for now, it's unused anyway. We'll probably reimplement it in
> future.

Although I agree that this passing errors around is a bit of an
overkill, my problem with NBD client is that it's notoriously silent
about problems it expeirences, and those errors never pop up in logs.

Given that these errors are not guest-triggerable, and probably indicate
serious problems at the infrastructure level, instead of endlessly
passing them around (as in the code ATM) or dropping them on the floor
(as you propose in the patch) I'd much rather log them immediately when
encountering.

I have a patch to that end, I'll try to port it on top of your series.

Thanks,
Roman.

[PATCH 03/14] block/nbd: drop unused NBDConnectThread::err field

[Vladimir Sementsov-Ogievskiy]

The field is used only to free it. Let's drop it for now for
simplicity.

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

diff --git a/block/nbd.c b/block/nbd.c
index 29c33338bf..fbf5154048 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -94,12 +94,8 @@ typedef struct NBDConnectThread {
     QEMUBHFunc *bh_func;
     void *bh_opaque;
 
-    /*
-     * Result of last attempt. Valid in FAIL and SUCCESS states.
-     * If you want to steal error, don't forget to set pointer to NULL.
-     */
+    /* Result of last attempt. Valid in FAIL and SUCCESS states. */
     QIOChannelSocket *sioc;
-    Error *err;
 
     /* state and bh_ctx are protected by mutex */
     QemuMutex mutex;
@@ -385,7 +381,6 @@ static void nbd_free_connect_thread(NBDConnectThread *thr)
     if (thr->sioc) {
         qio_channel_close(QIO_CHANNEL(thr->sioc), NULL);
     }
-    error_free(thr->err);
     qapi_free_SocketAddress(thr->saddr);
     g_free(thr);
 }
@@ -398,9 +393,7 @@ static void *connect_thread_func(void *opaque)
 
     thr->sioc = qio_channel_socket_new();
 
-    error_free(thr->err);
-    thr->err = NULL;
-    ret = qio_channel_socket_connect_sync(thr->sioc, thr->saddr, &thr->err);
+    ret = qio_channel_socket_connect_sync(thr->sioc, thr->saddr, NULL);
     if (ret < 0) {
         object_unref(OBJECT(thr->sioc));
         thr->sioc = NULL;
@@ -447,8 +440,6 @@ nbd_co_establish_connection(BlockDriverState *bs)
     switch (thr->state) {
     case CONNECT_THREAD_FAIL:
     case CONNECT_THREAD_NONE:
-        error_free(thr->err);
-        thr->err = NULL;
         thr->state = CONNECT_THREAD_RUNNING;
         qemu_thread_create(&thread, "nbd-connect",
                            connect_thread_func, thr, QEMU_THREAD_DETACHED);
@@ -491,8 +482,6 @@ nbd_co_establish_connection(BlockDriverState *bs)
     case CONNECT_THREAD_SUCCESS:
     case CONNECT_THREAD_FAIL:
         thr->state = CONNECT_THREAD_NONE;
-        error_free(thr->err);
-        thr->err = NULL;
         s->sioc = thr->sioc;
         thr->sioc = NULL;
         if (s->sioc) {
-- 
2.29.2

Re: [PATCH 03/14] block/nbd: drop unused NBDConnectThread::err field

[Roman Kagan]

On Wed, Apr 07, 2021 at 01:46:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> The field is used only to free it. Let's drop it for now for
> simplicity.

Well, it's *now* (after your patch 2) only used to free it.  This makes
the reconnect process even further concealed from the user: the client
may be struggling to re-establish the connection but you'll never know
when looking at the logs.

As I wrote in my comment to patch 2 I believe these errors need to be
logged.  Whether to do it directly in the reconnection thread or punt it
to the spawner of it is another matter; I'd personally prefer to do
logging in the original bdrv context as it allows to (easier) tag the
log messages with the indication of which connection is suffering.

Thanks,
Roman.

Re: [PATCH 03/14] block/nbd: drop unused NBDConnectThread::err field

[Vladimir Sementsov-Ogievskiy]

07.04.2021 14:42, Roman Kagan wrote:
> On Wed, Apr 07, 2021 at 01:46:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> The field is used only to free it. Let's drop it for now for
>> simplicity.
> 
> Well, it's *now* (after your patch 2) only used to free it.  This makes
> the reconnect process even further concealed from the user: the client
> may be struggling to re-establish the connection but you'll never know
> when looking at the logs.
> 
> As I wrote in my comment to patch 2 I believe these errors need to be
> logged.  Whether to do it directly in the reconnection thread or punt it
> to the spawner of it is another matter; I'd personally prefer to do
> logging in the original bdrv context as it allows to (easier) tag the
> log messages with the indication of which connection is suffering.
> 

I see your point. It may be more reasonable to rebase my series on your patch and just save the err. Could you send your patch based on master?


-- 
Best regards,
Vladimir

[PATCH 04/14] block/nbd: split connect_thread_cb() out of connect_thread_func()

[Vladimir Sementsov-Ogievskiy]

We are going to split connect-thread to separate file. Start from
splitting the callback.

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

diff --git a/block/nbd.c b/block/nbd.c
index fbf5154048..a9d351cbbc 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -385,20 +385,11 @@ static void nbd_free_connect_thread(NBDConnectThread *thr)
     g_free(thr);
 }
 
-static void *connect_thread_func(void *opaque)
+static void connect_thread_cb(int ret, void *opaque)
 {
     NBDConnectThread *thr = opaque;
-    int ret;
     bool do_free = false;
 
-    thr->sioc = qio_channel_socket_new();
-
-    ret = qio_channel_socket_connect_sync(thr->sioc, thr->saddr, NULL);
-    if (ret < 0) {
-        object_unref(OBJECT(thr->sioc));
-        thr->sioc = NULL;
-    }
-
     qemu_mutex_lock(&thr->mutex);
 
     switch (thr->state) {
@@ -423,6 +414,22 @@ static void *connect_thread_func(void *opaque)
     if (do_free) {
         nbd_free_connect_thread(thr);
     }
+}
+
+static void *connect_thread_func(void *opaque)
+{
+    NBDConnectThread *thr = opaque;
+    int ret;
+
+    thr->sioc = qio_channel_socket_new();
+
+    ret = qio_channel_socket_connect_sync(thr->sioc, thr->saddr, NULL);
+    if (ret < 0) {
+        object_unref(OBJECT(thr->sioc));
+        thr->sioc = NULL;
+    }
+
+    connect_thread_cb(ret, thr);
 
     return NULL;
 }
-- 
2.29.2

[PATCH 05/14] block/nbd: rename NBDConnectThread to NBDConnectCB

[Vladimir Sementsov-Ogievskiy]

Now it's mostly a state of call-back, so rename the structure, cleaning
the way for new small NBDConnectThread.

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

diff --git a/block/nbd.c b/block/nbd.c
index a9d351cbbc..e16c6d636a 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -84,7 +84,7 @@ typedef enum NBDConnectThreadState {
     CONNECT_THREAD_SUCCESS
 } NBDConnectThreadState;
 
-typedef struct NBDConnectThread {
+typedef struct NBDConnectCB {
     /* Initialization constants */
     SocketAddress *saddr; /* address to connect to */
     /*
@@ -101,7 +101,7 @@ typedef struct NBDConnectThread {
     QemuMutex mutex;
     NBDConnectThreadState state; /* current state of the thread */
     AioContext *bh_ctx; /* where to schedule bh (NULL means don't schedule) */
-} NBDConnectThread;
+} NBDConnectCB;
 
 typedef struct BDRVNBDState {
     QIOChannelSocket *sioc; /* The master data channel */
@@ -136,7 +136,7 @@ typedef struct BDRVNBDState {
     bool alloc_depth;
 
     bool wait_connect;
-    NBDConnectThread *connect_thread;
+    NBDConnectCB *connect_thread;
 } BDRVNBDState;
 
 static int nbd_establish_connection(BlockDriverState *bs, SocketAddress *saddr,
@@ -364,9 +364,9 @@ static void connect_bh(void *opaque)
 
 static void nbd_init_connect_thread(BDRVNBDState *s)
 {
-    s->connect_thread = g_new(NBDConnectThread, 1);
+    s->connect_thread = g_new(NBDConnectCB, 1);
 
-    *s->connect_thread = (NBDConnectThread) {
+    *s->connect_thread = (NBDConnectCB) {
         .saddr = QAPI_CLONE(SocketAddress, s->saddr),
         .state = CONNECT_THREAD_NONE,
         .bh_func = connect_bh,
@@ -376,7 +376,7 @@ static void nbd_init_connect_thread(BDRVNBDState *s)
     qemu_mutex_init(&s->connect_thread->mutex);
 }
 
-static void nbd_free_connect_thread(NBDConnectThread *thr)
+static void nbd_free_connect_thread(NBDConnectCB *thr)
 {
     if (thr->sioc) {
         qio_channel_close(QIO_CHANNEL(thr->sioc), NULL);
@@ -387,7 +387,7 @@ static void nbd_free_connect_thread(NBDConnectThread *thr)
 
 static void connect_thread_cb(int ret, void *opaque)
 {
-    NBDConnectThread *thr = opaque;
+    NBDConnectCB *thr = opaque;
     bool do_free = false;
 
     qemu_mutex_lock(&thr->mutex);
@@ -418,7 +418,7 @@ static void connect_thread_cb(int ret, void *opaque)
 
 static void *connect_thread_func(void *opaque)
 {
-    NBDConnectThread *thr = opaque;
+    NBDConnectCB *thr = opaque;
     int ret;
 
     thr->sioc = qio_channel_socket_new();
@@ -440,7 +440,7 @@ nbd_co_establish_connection(BlockDriverState *bs)
     int ret;
     QemuThread thread;
     BDRVNBDState *s = bs->opaque;
-    NBDConnectThread *thr = s->connect_thread;
+    NBDConnectCB *thr = s->connect_thread;
 
     qemu_mutex_lock(&thr->mutex);
 
@@ -536,7 +536,7 @@ static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
                                                bool detach)
 {
     BDRVNBDState *s = bs->opaque;
-    NBDConnectThread *thr = s->connect_thread;
+    NBDConnectCB *thr = s->connect_thread;
     bool wake = false;
     bool do_free = false;
 
-- 
2.29.2

[PATCH 06/14] block/nbd: further segregation of connect-thread

[Vladimir Sementsov-Ogievskiy]

Add personal state NBDConnectThread for connect-thread and
nbd_connect_thread_start() function. Next step would be moving
connect-thread to a separate file.

Note that we stop publishing thr->sioc during
qio_channel_socket_connect_sync(). Which probably means that we can't
early-cancel qio_channel_socket_connect_sync() call in
nbd_free_connect_thread(). Still, when thread is detached it doesn't
make big sense, and we have no guarantee anyway.

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

diff --git a/block/nbd.c b/block/nbd.c
index e16c6d636a..23eb8adab1 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -85,8 +85,6 @@ typedef enum NBDConnectThreadState {
 } NBDConnectThreadState;
 
 typedef struct NBDConnectCB {
-    /* Initialization constants */
-    SocketAddress *saddr; /* address to connect to */
     /*
      * Bottom half to schedule on completion. Scheduled only if bh_ctx is not
      * NULL
@@ -103,6 +101,15 @@ typedef struct NBDConnectCB {
     AioContext *bh_ctx; /* where to schedule bh (NULL means don't schedule) */
 } NBDConnectCB;
 
+typedef void (*NBDConnectThreadCallback)(QIOChannelSocket *sioc, int ret,
+                                         void *opaque);
+
+typedef struct NBDConnectThread {
+    SocketAddress *saddr; /* address to connect to */
+    NBDConnectThreadCallback cb;
+    void *cb_opaque;
+} NBDConnectThread;
+
 typedef struct BDRVNBDState {
     QIOChannelSocket *sioc; /* The master data channel */
     QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
@@ -367,7 +374,6 @@ static void nbd_init_connect_thread(BDRVNBDState *s)
     s->connect_thread = g_new(NBDConnectCB, 1);
 
     *s->connect_thread = (NBDConnectCB) {
-        .saddr = QAPI_CLONE(SocketAddress, s->saddr),
         .state = CONNECT_THREAD_NONE,
         .bh_func = connect_bh,
         .bh_opaque = s,
@@ -378,20 +384,18 @@ static void nbd_init_connect_thread(BDRVNBDState *s)
 
 static void nbd_free_connect_thread(NBDConnectCB *thr)
 {
-    if (thr->sioc) {
-        qio_channel_close(QIO_CHANNEL(thr->sioc), NULL);
-    }
-    qapi_free_SocketAddress(thr->saddr);
     g_free(thr);
 }
 
-static void connect_thread_cb(int ret, void *opaque)
+static void connect_thread_cb(QIOChannelSocket *sioc, int ret, void *opaque)
 {
     NBDConnectCB *thr = opaque;
     bool do_free = false;
 
     qemu_mutex_lock(&thr->mutex);
 
+    thr->sioc = sioc;
+
     switch (thr->state) {
     case CONNECT_THREAD_RUNNING:
         thr->state = ret < 0 ? CONNECT_THREAD_FAIL : CONNECT_THREAD_SUCCESS;
@@ -418,27 +422,45 @@ static void connect_thread_cb(int ret, void *opaque)
 
 static void *connect_thread_func(void *opaque)
 {
-    NBDConnectCB *thr = opaque;
+    NBDConnectThread *thr = opaque;
     int ret;
+    QIOChannelSocket *sioc = qio_channel_socket_new();
 
-    thr->sioc = qio_channel_socket_new();
-
-    ret = qio_channel_socket_connect_sync(thr->sioc, thr->saddr, NULL);
+    ret = qio_channel_socket_connect_sync(sioc, thr->saddr, NULL);
     if (ret < 0) {
-        object_unref(OBJECT(thr->sioc));
-        thr->sioc = NULL;
+        object_unref(OBJECT(sioc));
+        sioc = NULL;
     }
 
-    connect_thread_cb(ret, thr);
+    thr->cb(sioc, ret, thr->cb_opaque);
+
+    qapi_free_SocketAddress(thr->saddr);
+    g_free(thr);
 
     return NULL;
 }
 
+static void nbd_connect_thread_start(const SocketAddress *saddr,
+                                     NBDConnectThreadCallback cb,
+                                     void *cb_opaque)
+{
+    QemuThread thread;
+    NBDConnectThread *thr = g_new(NBDConnectThread, 1);
+
+    *thr = (NBDConnectThread) {
+        .saddr = QAPI_CLONE(SocketAddress, saddr),
+        .cb = cb,
+        .cb_opaque = cb_opaque,
+    };
+
+    qemu_thread_create(&thread, "nbd-connect",
+                       connect_thread_func, thr, QEMU_THREAD_DETACHED);
+}
+
 static int coroutine_fn
 nbd_co_establish_connection(BlockDriverState *bs)
 {
     int ret;
-    QemuThread thread;
     BDRVNBDState *s = bs->opaque;
     NBDConnectCB *thr = s->connect_thread;
 
@@ -448,8 +470,7 @@ nbd_co_establish_connection(BlockDriverState *bs)
     case CONNECT_THREAD_FAIL:
     case CONNECT_THREAD_NONE:
         thr->state = CONNECT_THREAD_RUNNING;
-        qemu_thread_create(&thread, "nbd-connect",
-                           connect_thread_func, thr, QEMU_THREAD_DETACHED);
+        nbd_connect_thread_start(s->saddr, connect_thread_cb, thr);
         break;
     case CONNECT_THREAD_SUCCESS:
         /* Previous attempt finally succeeded in background */
-- 
2.29.2

Re: [PATCH 06/14] block/nbd: further segregation of connect-thread

[Roman Kagan]

On Wed, Apr 07, 2021 at 01:46:29PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Add personal state NBDConnectThread for connect-thread and
> nbd_connect_thread_start() function. Next step would be moving
> connect-thread to a separate file.
> 
> Note that we stop publishing thr->sioc during
> qio_channel_socket_connect_sync(). Which probably means that we can't
> early-cancel qio_channel_socket_connect_sync() call in
> nbd_free_connect_thread(). Still, when thread is detached it doesn't
> make big sense, and we have no guarantee anyway.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/nbd.c | 57 ++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 39 insertions(+), 18 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index e16c6d636a..23eb8adab1 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -85,8 +85,6 @@ typedef enum NBDConnectThreadState {
>  } NBDConnectThreadState;
>  
>  typedef struct NBDConnectCB {
> -    /* Initialization constants */
> -    SocketAddress *saddr; /* address to connect to */
>      /*
>       * Bottom half to schedule on completion. Scheduled only if bh_ctx is not
>       * NULL
> @@ -103,6 +101,15 @@ typedef struct NBDConnectCB {
>      AioContext *bh_ctx; /* where to schedule bh (NULL means don't schedule) */
>  } NBDConnectCB;
>  
> +typedef void (*NBDConnectThreadCallback)(QIOChannelSocket *sioc, int ret,
> +                                         void *opaque);
> +
> +typedef struct NBDConnectThread {
> +    SocketAddress *saddr; /* address to connect to */
> +    NBDConnectThreadCallback cb;
> +    void *cb_opaque;
> +} NBDConnectThread;
> +
>  typedef struct BDRVNBDState {
>      QIOChannelSocket *sioc; /* The master data channel */
>      QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
> @@ -367,7 +374,6 @@ static void nbd_init_connect_thread(BDRVNBDState *s)
>      s->connect_thread = g_new(NBDConnectCB, 1);
>  
>      *s->connect_thread = (NBDConnectCB) {
> -        .saddr = QAPI_CLONE(SocketAddress, s->saddr),
>          .state = CONNECT_THREAD_NONE,
>          .bh_func = connect_bh,
>          .bh_opaque = s,
> @@ -378,20 +384,18 @@ static void nbd_init_connect_thread(BDRVNBDState *s)
>  
>  static void nbd_free_connect_thread(NBDConnectCB *thr)
>  {
> -    if (thr->sioc) {
> -        qio_channel_close(QIO_CHANNEL(thr->sioc), NULL);
> -    }

Doesn't this result in an open channel leak?  (The original code here
seems to be missing an unref, too.)

> -    qapi_free_SocketAddress(thr->saddr);
>      g_free(thr);
>  }
>  
> -static void connect_thread_cb(int ret, void *opaque)
> +static void connect_thread_cb(QIOChannelSocket *sioc, int ret, void *opaque)
>  {
>      NBDConnectCB *thr = opaque;
>      bool do_free = false;
>  
>      qemu_mutex_lock(&thr->mutex);
>  
> +    thr->sioc = sioc;
> +
>      switch (thr->state) {
>      case CONNECT_THREAD_RUNNING:
>          thr->state = ret < 0 ? CONNECT_THREAD_FAIL : CONNECT_THREAD_SUCCESS;
> @@ -418,27 +422,45 @@ static void connect_thread_cb(int ret, void *opaque)
>  
>  static void *connect_thread_func(void *opaque)
>  {
> -    NBDConnectCB *thr = opaque;
> +    NBDConnectThread *thr = opaque;
>      int ret;
> +    QIOChannelSocket *sioc = qio_channel_socket_new();
>  
> -    thr->sioc = qio_channel_socket_new();
> -
> -    ret = qio_channel_socket_connect_sync(thr->sioc, thr->saddr, NULL);
> +    ret = qio_channel_socket_connect_sync(sioc, thr->saddr, NULL);
>      if (ret < 0) {
> -        object_unref(OBJECT(thr->sioc));
> -        thr->sioc = NULL;
> +        object_unref(OBJECT(sioc));
> +        sioc = NULL;
>      }
>  
> -    connect_thread_cb(ret, thr);
> +    thr->cb(sioc, ret, thr->cb_opaque);
> +
> +    qapi_free_SocketAddress(thr->saddr);
> +    g_free(thr);
>  
>      return NULL;
>  }
>  
> +static void nbd_connect_thread_start(const SocketAddress *saddr,
> +                                     NBDConnectThreadCallback cb,
> +                                     void *cb_opaque)
> +{
> +    QemuThread thread;
> +    NBDConnectThread *thr = g_new(NBDConnectThread, 1);
> +
> +    *thr = (NBDConnectThread) {
> +        .saddr = QAPI_CLONE(SocketAddress, saddr),
> +        .cb = cb,
> +        .cb_opaque = cb_opaque,
> +    };
> +
> +    qemu_thread_create(&thread, "nbd-connect",
> +                       connect_thread_func, thr, QEMU_THREAD_DETACHED);
> +}
> +
>  static int coroutine_fn
>  nbd_co_establish_connection(BlockDriverState *bs)
>  {
>      int ret;
> -    QemuThread thread;
>      BDRVNBDState *s = bs->opaque;
>      NBDConnectCB *thr = s->connect_thread;
>  
> @@ -448,8 +470,7 @@ nbd_co_establish_connection(BlockDriverState *bs)
>      case CONNECT_THREAD_FAIL:
>      case CONNECT_THREAD_NONE:
>          thr->state = CONNECT_THREAD_RUNNING;
> -        qemu_thread_create(&thread, "nbd-connect",
> -                           connect_thread_func, thr, QEMU_THREAD_DETACHED);
> +        nbd_connect_thread_start(s->saddr, connect_thread_cb, thr);
>          break;
>      case CONNECT_THREAD_SUCCESS:
>          /* Previous attempt finally succeeded in background */
> -- 
> 2.29.2
> 

[PATCH 07/14] block/nbd: drop nbd_free_connect_thread()

[Vladimir Sementsov-Ogievskiy]

Now it's only a wrapper on g_free, we don't need it. Also, it's
clearing the state of nbd connect thread callback, not thread itself,
so name is not very good. Drop it.

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

diff --git a/block/nbd.c b/block/nbd.c
index 23eb8adab1..4e669d762a 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -382,11 +382,6 @@ static void nbd_init_connect_thread(BDRVNBDState *s)
     qemu_mutex_init(&s->connect_thread->mutex);
 }
 
-static void nbd_free_connect_thread(NBDConnectCB *thr)
-{
-    g_free(thr);
-}
-
 static void connect_thread_cb(QIOChannelSocket *sioc, int ret, void *opaque)
 {
     NBDConnectCB *thr = opaque;
@@ -416,7 +411,7 @@ static void connect_thread_cb(QIOChannelSocket *sioc, int ret, void *opaque)
     qemu_mutex_unlock(&thr->mutex);
 
     if (do_free) {
-        nbd_free_connect_thread(thr);
+        g_free(thr);
     }
 }
 
@@ -581,7 +576,7 @@ static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
     qemu_mutex_unlock(&thr->mutex);
 
     if (do_free) {
-        nbd_free_connect_thread(thr);
+        g_free(thr);
         s->connect_thread = NULL;
     }
 
-- 
2.29.2

[PATCH 08/14] block/nbd: move nbd connect-thread to nbd/client-connect.c

[Vladimir Sementsov-Ogievskiy]

connect-thread part is not directly connected to  block-layer. Good
place for it is nbd/ directory.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/nbd.h  |  6 ++++
 block/nbd.c          | 46 ----------------------------
 nbd/client-connect.c | 72 ++++++++++++++++++++++++++++++++++++++++++++
 nbd/meson.build      |  1 +
 4 files changed, 79 insertions(+), 46 deletions(-)
 create mode 100644 nbd/client-connect.c

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 5f34d23bb0..660ab4c266 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -406,4 +406,10 @@ const char *nbd_info_lookup(uint16_t info);
 const char *nbd_cmd_lookup(uint16_t info);
 const char *nbd_err_lookup(int err);
 
+
+typedef void (*NBDConnectThreadCallback)(QIOChannelSocket *sioc, int ret,
+                                         void *opaque);
+void nbd_connect_thread_start(const SocketAddress *saddr,
+                              NBDConnectThreadCallback cb, void *cb_opaque);
+
 #endif
diff --git a/block/nbd.c b/block/nbd.c
index 4e669d762a..ba281e2d5a 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -101,15 +101,6 @@ typedef struct NBDConnectCB {
     AioContext *bh_ctx; /* where to schedule bh (NULL means don't schedule) */
 } NBDConnectCB;
 
-typedef void (*NBDConnectThreadCallback)(QIOChannelSocket *sioc, int ret,
-                                         void *opaque);
-
-typedef struct NBDConnectThread {
-    SocketAddress *saddr; /* address to connect to */
-    NBDConnectThreadCallback cb;
-    void *cb_opaque;
-} NBDConnectThread;
-
 typedef struct BDRVNBDState {
     QIOChannelSocket *sioc; /* The master data channel */
     QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
@@ -415,43 +406,6 @@ static void connect_thread_cb(QIOChannelSocket *sioc, int ret, void *opaque)
     }
 }
 
-static void *connect_thread_func(void *opaque)
-{
-    NBDConnectThread *thr = opaque;
-    int ret;
-    QIOChannelSocket *sioc = qio_channel_socket_new();
-
-    ret = qio_channel_socket_connect_sync(sioc, thr->saddr, NULL);
-    if (ret < 0) {
-        object_unref(OBJECT(sioc));
-        sioc = NULL;
-    }
-
-    thr->cb(sioc, ret, thr->cb_opaque);
-
-    qapi_free_SocketAddress(thr->saddr);
-    g_free(thr);
-
-    return NULL;
-}
-
-static void nbd_connect_thread_start(const SocketAddress *saddr,
-                                     NBDConnectThreadCallback cb,
-                                     void *cb_opaque)
-{
-    QemuThread thread;
-    NBDConnectThread *thr = g_new(NBDConnectThread, 1);
-
-    *thr = (NBDConnectThread) {
-        .saddr = QAPI_CLONE(SocketAddress, saddr),
-        .cb = cb,
-        .cb_opaque = cb_opaque,
-    };
-
-    qemu_thread_create(&thread, "nbd-connect",
-                       connect_thread_func, thr, QEMU_THREAD_DETACHED);
-}
-
 static int coroutine_fn
 nbd_co_establish_connection(BlockDriverState *bs)
 {
diff --git a/nbd/client-connect.c b/nbd/client-connect.c
new file mode 100644
index 0000000000..9f22c41a34
--- /dev/null
+++ b/nbd/client-connect.c
@@ -0,0 +1,72 @@
+/*
+ * QEMU Block driver for  NBD
+ *
+ * Copyright (c) 2020 Virtuozzo International GmbH.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+
+#include "qapi/qapi-visit-sockets.h"
+#include "qapi/clone-visitor.h"
+
+#include "block/nbd.h"
+
+typedef struct NBDConnectThread {
+    SocketAddress *saddr; /* address to connect to */
+    NBDConnectThreadCallback cb;
+    void *cb_opaque;
+} NBDConnectThread;
+
+static void *connect_thread_func(void *opaque)
+{
+    NBDConnectThread *thr = opaque;
+    int ret;
+    QIOChannelSocket *sioc = qio_channel_socket_new();
+
+    ret = qio_channel_socket_connect_sync(sioc, thr->saddr, NULL);
+    if (ret < 0) {
+        object_unref(OBJECT(sioc));
+        sioc = NULL;
+    }
+
+    thr->cb(sioc, ret, thr->cb_opaque);
+
+    qapi_free_SocketAddress(thr->saddr);
+    g_free(thr);
+
+    return NULL;
+}
+
+void nbd_connect_thread_start(const SocketAddress *saddr,
+                              NBDConnectThreadCallback cb, void *cb_opaque)
+{
+    QemuThread thread;
+    NBDConnectThread *thr = g_new(NBDConnectThread, 1);
+
+    *thr = (NBDConnectThread) {
+        .saddr = QAPI_CLONE(SocketAddress, saddr),
+        .cb = cb,
+        .cb_opaque = cb_opaque,
+    };
+
+    qemu_thread_create(&thread, "nbd-connect",
+                       connect_thread_func, thr, QEMU_THREAD_DETACHED);
+}
diff --git a/nbd/meson.build b/nbd/meson.build
index 2baaa36948..da8c65ae59 100644
--- a/nbd/meson.build
+++ b/nbd/meson.build
@@ -1,5 +1,6 @@
 block_ss.add(files(
   'client.c',
+  'client-connect.c',
   'common.c',
 ))
 blockdev_ss.add(files(
-- 
2.29.2

[PATCH 09/14] block/nbd: NBDConnectCB: drop bh_* fields

[Vladimir Sementsov-Ogievskiy]

Drop bh_* fields and add back link to bs instead. We are on the way of
simplifying reconnect logic in nbd driver, so look forward to further
commits based on that one.

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

diff --git a/block/nbd.c b/block/nbd.c
index ba281e2d5a..8bd52884c8 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -85,13 +85,6 @@ typedef enum NBDConnectThreadState {
 } NBDConnectThreadState;
 
 typedef struct NBDConnectCB {
-    /*
-     * Bottom half to schedule on completion. Scheduled only if bh_ctx is not
-     * NULL
-     */
-    QEMUBHFunc *bh_func;
-    void *bh_opaque;
-
     /* Result of last attempt. Valid in FAIL and SUCCESS states. */
     QIOChannelSocket *sioc;
 
@@ -99,6 +92,9 @@ typedef struct NBDConnectCB {
     QemuMutex mutex;
     NBDConnectThreadState state; /* current state of the thread */
     AioContext *bh_ctx; /* where to schedule bh (NULL means don't schedule) */
+
+    /* Link to NBD BDS. If NULL thread is detached, BDS is probably closed. */
+    BlockDriverState *bs;
 } NBDConnectCB;
 
 typedef struct BDRVNBDState {
@@ -351,32 +347,34 @@ static bool nbd_client_connecting_wait(BDRVNBDState *s)
     return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT;
 }
 
-static void connect_bh(void *opaque)
+static void nbd_init_connect_thread(BlockDriverState *bs)
 {
-    BDRVNBDState *state = opaque;
-
-    assert(state->wait_connect);
-    state->wait_connect = false;
-    aio_co_wake(state->connection_co);
-}
+    BDRVNBDState *s = bs->opaque;
 
-static void nbd_init_connect_thread(BDRVNBDState *s)
-{
     s->connect_thread = g_new(NBDConnectCB, 1);
 
     *s->connect_thread = (NBDConnectCB) {
         .state = CONNECT_THREAD_NONE,
-        .bh_func = connect_bh,
-        .bh_opaque = s,
+        .bs = bs,
     };
 
     qemu_mutex_init(&s->connect_thread->mutex);
 }
 
+static void connect_bh(void *opaque)
+{
+    BDRVNBDState *state = opaque;
+
+    assert(state->wait_connect);
+    state->wait_connect = false;
+    aio_co_wake(state->connection_co);
+}
+
 static void connect_thread_cb(QIOChannelSocket *sioc, int ret, void *opaque)
 {
     NBDConnectCB *thr = opaque;
     bool do_free = false;
+    BDRVNBDState *s = thr->bs ? thr->bs->opaque : NULL;
 
     qemu_mutex_lock(&thr->mutex);
 
@@ -386,7 +384,7 @@ static void connect_thread_cb(QIOChannelSocket *sioc, int ret, void *opaque)
     case CONNECT_THREAD_RUNNING:
         thr->state = ret < 0 ? CONNECT_THREAD_FAIL : CONNECT_THREAD_SUCCESS;
         if (thr->bh_ctx) {
-            aio_bh_schedule_oneshot(thr->bh_ctx, thr->bh_func, thr->bh_opaque);
+            aio_bh_schedule_oneshot(thr->bh_ctx, connect_bh, s);
 
             /* play safe, don't reuse bh_ctx on further connection attempts */
             thr->bh_ctx = NULL;
@@ -520,6 +518,7 @@ static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
             wake = true;
         }
         if (detach) {
+            thr->bs = NULL;
             thr->state = CONNECT_THREAD_RUNNING_DETACHED;
             s->connect_thread = NULL;
         }
@@ -2271,7 +2270,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
     /* successfully connected */
     s->state = NBD_CLIENT_CONNECTED;
 
-    nbd_init_connect_thread(s);
+    nbd_init_connect_thread(bs);
 
     s->connection_co = qemu_coroutine_create(nbd_connection_entry, s);
     bdrv_inc_in_flight(bs);
-- 
2.29.2

[PATCH 10/14] block/nbd: move wait_connect field under mutex protection

[Vladimir Sementsov-Ogievskiy]

Move wait_connect to NBDConnectCB and protect it by mutex. It provides
simpler logic than bothering with bh_ctx (which we can drop now).

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

diff --git a/block/nbd.c b/block/nbd.c
index 8bd52884c8..29bdbd38b6 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -88,13 +88,15 @@ typedef struct NBDConnectCB {
     /* Result of last attempt. Valid in FAIL and SUCCESS states. */
     QIOChannelSocket *sioc;
 
-    /* state and bh_ctx are protected by mutex */
     QemuMutex mutex;
+    /* All further fields are protected by mutex */
     NBDConnectThreadState state; /* current state of the thread */
-    AioContext *bh_ctx; /* where to schedule bh (NULL means don't schedule) */
 
     /* Link to NBD BDS. If NULL thread is detached, BDS is probably closed. */
     BlockDriverState *bs;
+
+    /* connection_co is waiting in yield() */
+    bool wait_connect;
 } NBDConnectCB;
 
 typedef struct BDRVNBDState {
@@ -129,7 +131,6 @@ typedef struct BDRVNBDState {
     char *x_dirty_bitmap;
     bool alloc_depth;
 
-    bool wait_connect;
     NBDConnectCB *connect_thread;
 } BDRVNBDState;
 
@@ -365,8 +366,6 @@ static void connect_bh(void *opaque)
 {
     BDRVNBDState *state = opaque;
 
-    assert(state->wait_connect);
-    state->wait_connect = false;
     aio_co_wake(state->connection_co);
 }
 
@@ -374,6 +373,7 @@ static void connect_thread_cb(QIOChannelSocket *sioc, int ret, void *opaque)
 {
     NBDConnectCB *thr = opaque;
     bool do_free = false;
+    bool do_wake = false;
     BDRVNBDState *s = thr->bs ? thr->bs->opaque : NULL;
 
     qemu_mutex_lock(&thr->mutex);
@@ -383,12 +383,8 @@ static void connect_thread_cb(QIOChannelSocket *sioc, int ret, void *opaque)
     switch (thr->state) {
     case CONNECT_THREAD_RUNNING:
         thr->state = ret < 0 ? CONNECT_THREAD_FAIL : CONNECT_THREAD_SUCCESS;
-        if (thr->bh_ctx) {
-            aio_bh_schedule_oneshot(thr->bh_ctx, connect_bh, s);
-
-            /* play safe, don't reuse bh_ctx on further connection attempts */
-            thr->bh_ctx = NULL;
-        }
+        do_wake = thr->wait_connect;
+        thr->wait_connect = false;
         break;
     case CONNECT_THREAD_RUNNING_DETACHED:
         do_free = true;
@@ -399,6 +395,17 @@ static void connect_thread_cb(QIOChannelSocket *sioc, int ret, void *opaque)
 
     qemu_mutex_unlock(&thr->mutex);
 
+    if (do_wake) {
+        /*
+         * At this point we are sure that connection_co sleeps in the
+         * corresponding yield point and we here have an exclusive right
+         * (and obligations) to wake it.
+         * Direct call to aio_co_wake() from thread context works bad. So use
+         * aio_bh_schedule_oneshot() as a mediator.
+         */
+        aio_bh_schedule_oneshot(bdrv_get_aio_context(thr->bs), connect_bh, s);
+    }
+
     if (do_free) {
         g_free(thr);
     }
@@ -435,20 +442,14 @@ nbd_co_establish_connection(BlockDriverState *bs)
         abort();
     }
 
-    thr->bh_ctx = qemu_get_current_aio_context();
+    thr->wait_connect = true;
 
     qemu_mutex_unlock(&thr->mutex);
 
-
     /*
      * We are going to wait for connect-thread finish, but
      * nbd_client_co_drain_begin() can interrupt.
-     *
-     * Note that wait_connect variable is not visible for connect-thread. It
-     * doesn't need mutex protection, it used only inside home aio context of
-     * bs.
      */
-    s->wait_connect = true;
     qemu_coroutine_yield();
 
     qemu_mutex_lock(&thr->mutex);
@@ -512,9 +513,8 @@ static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
 
     if (thr->state == CONNECT_THREAD_RUNNING) {
         /* We can cancel only in running state, when bh is not yet scheduled */
-        thr->bh_ctx = NULL;
-        if (s->wait_connect) {
-            s->wait_connect = false;
+        if (thr->wait_connect) {
+            thr->wait_connect = false;
             wake = true;
         }
         if (detach) {
-- 
2.29.2

[PATCH 11/14] block/nbd: refactor connect_bh()

[Vladimir Sementsov-Ogievskiy]

Now it's just a wrapper for aio_co_wake(). Make it more obvious.

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

diff --git a/block/nbd.c b/block/nbd.c
index 29bdbd38b6..6729561935 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -362,11 +362,9 @@ static void nbd_init_connect_thread(BlockDriverState *bs)
     qemu_mutex_init(&s->connect_thread->mutex);
 }
 
-static void connect_bh(void *opaque)
+static void coroutine_wake_bh(void *opaque)
 {
-    BDRVNBDState *state = opaque;
-
-    aio_co_wake(state->connection_co);
+    aio_co_wake(opaque);
 }
 
 static void connect_thread_cb(QIOChannelSocket *sioc, int ret, void *opaque)
@@ -403,7 +401,8 @@ static void connect_thread_cb(QIOChannelSocket *sioc, int ret, void *opaque)
          * Direct call to aio_co_wake() from thread context works bad. So use
          * aio_bh_schedule_oneshot() as a mediator.
          */
-        aio_bh_schedule_oneshot(bdrv_get_aio_context(thr->bs), connect_bh, s);
+        aio_bh_schedule_oneshot(bdrv_get_aio_context(thr->bs),
+                                coroutine_wake_bh, s->connection_co);
     }
 
     if (do_free) {
-- 
2.29.2

[PATCH 12/14] block/nbd: refactor nbd_co_establish_connection

[Vladimir Sementsov-Ogievskiy]

We are going to drop nbd connect thread states for simplicity. This is
a step that makes further commit simpler.

Note, that yank_* calls moved out of thr->mutex. They shouldn't be
related and already called from other places in the file not under the
mutex.

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

diff --git a/block/nbd.c b/block/nbd.c
index 6729561935..f0319d2256 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -413,7 +413,6 @@ static void connect_thread_cb(QIOChannelSocket *sioc, int ret, void *opaque)
 static int coroutine_fn
 nbd_co_establish_connection(BlockDriverState *bs)
 {
-    int ret;
     BDRVNBDState *s = bs->opaque;
     NBDConnectCB *thr = s->connect_thread;
 
@@ -430,10 +429,7 @@ nbd_co_establish_connection(BlockDriverState *bs)
         thr->state = CONNECT_THREAD_NONE;
         s->sioc = thr->sioc;
         thr->sioc = NULL;
-        yank_register_function(BLOCKDEV_YANK_INSTANCE(bs->node_name),
-                               nbd_yank, bs);
-        qemu_mutex_unlock(&thr->mutex);
-        return 0;
+        goto out;
     case CONNECT_THREAD_RUNNING:
         /* Already running, will wait */
         break;
@@ -459,11 +455,6 @@ nbd_co_establish_connection(BlockDriverState *bs)
         thr->state = CONNECT_THREAD_NONE;
         s->sioc = thr->sioc;
         thr->sioc = NULL;
-        if (s->sioc) {
-            yank_register_function(BLOCKDEV_YANK_INSTANCE(bs->node_name),
-                                   nbd_yank, bs);
-        }
-        ret = (s->sioc ? 0 : -1);
         break;
     case CONNECT_THREAD_RUNNING:
     case CONNECT_THREAD_RUNNING_DETACHED:
@@ -472,7 +463,6 @@ nbd_co_establish_connection(BlockDriverState *bs)
          * failed. Still connect thread is executing in background, and its
          * result may be used for next connection attempt.
          */
-        ret = -1;
         break;
 
     case CONNECT_THREAD_NONE:
@@ -486,9 +476,15 @@ nbd_co_establish_connection(BlockDriverState *bs)
         abort();
     }
 
+out:
     qemu_mutex_unlock(&thr->mutex);
 
-    return ret;
+    if (s->sioc) {
+        yank_register_function(BLOCKDEV_YANK_INSTANCE(bs->node_name),
+                               nbd_yank, bs);
+    }
+
+    return s->sioc ? 0 : -1;
 }
 
 /*
-- 
2.29.2

[PATCH 13/14] block/nbd: nbd_co_establish_connection_cancel(): rename wake to do_wake

[Vladimir Sementsov-Ogievskiy]

Rename local variable to look like do_free in same function and like
do_wake and do_free in connect_thread_cb

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

diff --git a/block/nbd.c b/block/nbd.c
index f0319d2256..9cee5b6650 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -501,7 +501,7 @@ static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
 {
     BDRVNBDState *s = bs->opaque;
     NBDConnectCB *thr = s->connect_thread;
-    bool wake = false;
+    bool do_wake = false;
     bool do_free = false;
 
     qemu_mutex_lock(&thr->mutex);
@@ -510,7 +510,7 @@ static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
         /* We can cancel only in running state, when bh is not yet scheduled */
         if (thr->wait_connect) {
             thr->wait_connect = false;
-            wake = true;
+            do_wake = true;
         }
         if (detach) {
             thr->bs = NULL;
@@ -528,7 +528,7 @@ static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
         s->connect_thread = NULL;
     }
 
-    if (wake) {
+    if (do_wake) {
         aio_co_wake(s->connection_co);
     }
 }
-- 
2.29.2

[PATCH 14/14] block/nbd: drop thr->state

[Vladimir Sementsov-Ogievskiy]

thr->state variable mostly duplicates information that is already
obvious from the other fields: thr->bs=NULL means DETACHED,
thr->sioc!=NULL means SUCCESS. The only bit of information we need is
"is thread running now or not". So, drop state and add simple boolean
instead. It simplifies the logic a lot.

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

diff --git a/block/nbd.c b/block/nbd.c
index 9cee5b6650..5320a359f6 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -66,31 +66,16 @@ typedef enum NBDClientState {
     NBD_CLIENT_QUIT
 } NBDClientState;
 
-typedef enum NBDConnectThreadState {
-    /* No thread, no pending results */
-    CONNECT_THREAD_NONE,
-
-    /* Thread is running, no results for now */
-    CONNECT_THREAD_RUNNING,
-
+typedef struct NBDConnectCB {
     /*
-     * Thread is running, but requestor exited. Thread should close
-     * the new socket and free the connect state on exit.
+     * Result of last attempt. Set in connect_thread_cb()  on success. Should be
+     * set to NULL before starting the thread.
      */
-    CONNECT_THREAD_RUNNING_DETACHED,
-
-    /* Thread finished, results are stored in a state */
-    CONNECT_THREAD_FAIL,
-    CONNECT_THREAD_SUCCESS
-} NBDConnectThreadState;
-
-typedef struct NBDConnectCB {
-    /* Result of last attempt. Valid in FAIL and SUCCESS states. */
     QIOChannelSocket *sioc;
 
     QemuMutex mutex;
     /* All further fields are protected by mutex */
-    NBDConnectThreadState state; /* current state of the thread */
+    bool running; /* thread is running now */
 
     /* Link to NBD BDS. If NULL thread is detached, BDS is probably closed. */
     BlockDriverState *bs;
@@ -354,10 +339,7 @@ static void nbd_init_connect_thread(BlockDriverState *bs)
 
     s->connect_thread = g_new(NBDConnectCB, 1);
 
-    *s->connect_thread = (NBDConnectCB) {
-        .state = CONNECT_THREAD_NONE,
-        .bs = bs,
-    };
+    *s->connect_thread = (NBDConnectCB) { .bs = bs };
 
     qemu_mutex_init(&s->connect_thread->mutex);
 }
@@ -374,22 +356,21 @@ static void connect_thread_cb(QIOChannelSocket *sioc, int ret, void *opaque)
     bool do_wake = false;
     BDRVNBDState *s = thr->bs ? thr->bs->opaque : NULL;
 
+    /* We are in context of connect thread ! */
+
     qemu_mutex_lock(&thr->mutex);
 
+    assert(thr->running);
+    assert(thr->sioc == NULL);
+    assert(thr->bs || !thr->wait_connect);
+
+    thr->running = false;
     thr->sioc = sioc;
 
-    switch (thr->state) {
-    case CONNECT_THREAD_RUNNING:
-        thr->state = ret < 0 ? CONNECT_THREAD_FAIL : CONNECT_THREAD_SUCCESS;
-        do_wake = thr->wait_connect;
-        thr->wait_connect = false;
-        break;
-    case CONNECT_THREAD_RUNNING_DETACHED:
-        do_free = true;
-        break;
-    default:
-        abort();
-    }
+    do_wake = thr->wait_connect;
+    thr->wait_connect = false;
+
+    do_free = !thr->bs; /* detached */
 
     qemu_mutex_unlock(&thr->mutex);
 
@@ -416,25 +397,21 @@ nbd_co_establish_connection(BlockDriverState *bs)
     BDRVNBDState *s = bs->opaque;
     NBDConnectCB *thr = s->connect_thread;
 
+    assert(!s->sioc);
+
     qemu_mutex_lock(&thr->mutex);
 
-    switch (thr->state) {
-    case CONNECT_THREAD_FAIL:
-    case CONNECT_THREAD_NONE:
-        thr->state = CONNECT_THREAD_RUNNING;
-        nbd_connect_thread_start(s->saddr, connect_thread_cb, thr);
-        break;
-    case CONNECT_THREAD_SUCCESS:
+    if (thr->sioc) {
         /* Previous attempt finally succeeded in background */
-        thr->state = CONNECT_THREAD_NONE;
+        assert(!thr->running);
         s->sioc = thr->sioc;
         thr->sioc = NULL;
         goto out;
-    case CONNECT_THREAD_RUNNING:
-        /* Already running, will wait */
-        break;
-    default:
-        abort();
+    }
+
+    if (!thr->running) {
+        thr->running = true;
+        nbd_connect_thread_start(s->saddr, connect_thread_cb, thr);
     }
 
     thr->wait_connect = true;
@@ -449,32 +426,8 @@ nbd_co_establish_connection(BlockDriverState *bs)
 
     qemu_mutex_lock(&thr->mutex);
 
-    switch (thr->state) {
-    case CONNECT_THREAD_SUCCESS:
-    case CONNECT_THREAD_FAIL:
-        thr->state = CONNECT_THREAD_NONE;
-        s->sioc = thr->sioc;
-        thr->sioc = NULL;
-        break;
-    case CONNECT_THREAD_RUNNING:
-    case CONNECT_THREAD_RUNNING_DETACHED:
-        /*
-         * Obviously, drained section wants to start. Report the attempt as
-         * failed. Still connect thread is executing in background, and its
-         * result may be used for next connection attempt.
-         */
-        break;
-
-    case CONNECT_THREAD_NONE:
-        /*
-         * Impossible. We've seen this thread running. So it should be
-         * running or at least give some results.
-         */
-        abort();
-
-    default:
-        abort();
-    }
+    s->sioc = thr->sioc;
+    thr->sioc = NULL;
 
 out:
     qemu_mutex_unlock(&thr->mutex);
@@ -506,26 +459,19 @@ static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
 
     qemu_mutex_lock(&thr->mutex);
 
-    if (thr->state == CONNECT_THREAD_RUNNING) {
-        /* We can cancel only in running state, when bh is not yet scheduled */
-        if (thr->wait_connect) {
-            thr->wait_connect = false;
-            do_wake = true;
-        }
-        if (detach) {
-            thr->bs = NULL;
-            thr->state = CONNECT_THREAD_RUNNING_DETACHED;
-            s->connect_thread = NULL;
-        }
-    } else if (detach) {
-        do_free = true;
+    do_wake = thr->wait_connect;
+    thr->wait_connect = false;
+
+    if (detach) {
+        s->connect_thread = NULL;
+        thr->bs = NULL;
+        do_free = !thr->running;
     }
 
     qemu_mutex_unlock(&thr->mutex);
 
     if (do_free) {
         g_free(thr);
-        s->connect_thread = NULL;
     }
 
     if (do_wake) {
-- 
2.29.2

DROP THIS Re: [PATCH 00/14] nbd: move reconnect-thread to separate file

[Vladimir Sementsov-Ogievskiy]

07.04.2021 13:46, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> There are problems with nbd driver:
> 
>   - nbd reconnect is cancelled on drain, which is bad as Roman describes
>     in his "[PATCH 0/7] block/nbd: decouple reconnect from drain"
>   - nbd driver is too complicated around drained sections and aio context
>     switch. It's nearly impossible to follow all the logic, including
>     abuse of bs->in_flight, which is temporary decreased in some places
>     (like nbd_read_eof()). Additional reconnect thread and two different
>     state machines (we have BDRVNBDState::state and
>     BDRVNBDState::connect_thread->state) doesn't make things simpler :)
> 
> So, I have a plan:
> 
> 1. Move nbd negotiation to connect_thread
> 
> 2. Do receive NBD replies in request coroutines, not in connection_co
>    
>     At this point we can drop connection_co, and when we don't have
>     endless running coroutine, NBD driver becomes a usual block driver,
>     and we can drop abuse of bs->in_flight, and probably drop most of
>     complicated logic around drained section and aio context switch in
>     nbd driver.
> 
> 3. Still, as Roman describes, with [2] we loose a possibility to
>     reconnect immediately when connection breaks (with current code we
>     have endless read in reconnect_co, but actually for this to work
>     keep-alive should be setup correctly). So, we'll need to reinvent it,
>     checking connection periodically by timeout, with help of getsockopt
>     or just sending a kind of PING request (zero-length READ or something
>     like this).
> 
> And this series a kind of preparation. The main point of it is moving
> connect-thread to a separate file.
> 


Finally I don't like it. I'm in progress of creating new series to substitute this one, so don't waste your time on review.


-- 
Best regards,
Vladimir