Re: [PATCH v2 06/10] block/nbd: bs-independent interface for nbd_co_establish_connection()

[Roman Kagan]

On Thu, Apr 08, 2021 at 05:08:23PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We are going to split connection code to separate file. Now we are
> ready to give nbd_co_establish_connection() clean and bs-independent
> interface.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/nbd.c | 49 +++--
>  1 file changed, 31 insertions(+), 18 deletions(-)

Reviewed-by: Roman Kagan 

[PATCH v2 06/10] block/nbd: bs-independent interface for nbd_co_establish_connection()

[Vladimir Sementsov-Ogievskiy]

We are going to split connection code to separate file. Now we are
ready to give nbd_co_establish_connection() clean and bs-independent
interface.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/nbd.c | 49 +++--
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 85c20e7810..a487fd1e68 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -122,7 +122,8 @@ typedef struct BDRVNBDState {
 static void connect_thread_state_unref(NBDConnectThread *thr);
 static int nbd_establish_connection(BlockDriverState *bs, SocketAddress *saddr,
 Error **errp);
-static int nbd_co_establish_connection(BlockDriverState *bs, Error **errp);
+static coroutine_fn QIOChannelSocket *
+nbd_co_establish_connection(NBDConnectThread *thr, Error **errp);
 static void nbd_co_establish_connection_cancel(BlockDriverState *bs);
 static int nbd_client_handshake(BlockDriverState *bs, Error **errp);
 static void nbd_yank(void *opaque);
@@ -390,22 +391,36 @@ static void *connect_thread_func(void *opaque)
 return NULL;
 }
 
-static int coroutine_fn
-nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
+/*
+ * Get a new connection in context of @thr:
+ *   if thread is running, wait for completion
+ *   if thread is already succeeded in background, and user didn't get the
+ * result, just return it now
+ *   otherwise if thread is not running, start a thread and wait for completion
+ */
+static coroutine_fn QIOChannelSocket *
+nbd_co_establish_connection(NBDConnectThread *thr, Error **errp)
 {
+QIOChannelSocket *sioc = NULL;
 QemuThread thread;
-BDRVNBDState *s = bs->opaque;
-NBDConnectThread *thr = s->connect_thread;
-
-assert(!s->sioc);
 
 qemu_mutex_lock(>mutex);
 
+/*
+ * Don't call nbd_co_establish_connection() in several coroutines in
+ * parallel. Only one call at once is supported.
+ */
+assert(!thr->wait_co);
+
 if (!thr->running) {
 if (thr->sioc) {
 /* Previous attempt finally succeeded in background */
-goto out;
+sioc = g_steal_pointer(>sioc);
+qemu_mutex_unlock(>mutex);
+
+return sioc;
 }
+
 thr->running = true;
 error_free(thr->err);
 thr->err = NULL;
@@ -420,13 +435,12 @@ nbd_co_establish_connection(BlockDriverState *bs, Error 
**errp)
 
 /*
  * We are going to wait for connect-thread finish, but
- * nbd_client_co_drain_begin() can interrupt.
+ * nbd_co_establish_connection_cancel() can interrupt.
  */
 qemu_coroutine_yield();
 
 qemu_mutex_lock(>mutex);
 
-out:
 if (thr->running) {
 /*
  * Obviously, drained section wants to start. Report the attempt as
@@ -437,17 +451,12 @@ out:
 } else {
 error_propagate(errp, thr->err);
 thr->err = NULL;
-s->sioc = thr->sioc;
-thr->sioc = NULL;
-if (s->sioc) {
-yank_register_function(BLOCKDEV_YANK_INSTANCE(bs->node_name),
-   nbd_yank, bs);
-}
+sioc = g_steal_pointer(>sioc);
 }
 
 qemu_mutex_unlock(>mutex);
 
-return s->sioc ? 0 : -1;
+return sioc;
 }
 
 /*
@@ -514,11 +523,15 @@ static coroutine_fn void 
nbd_reconnect_attempt(BDRVNBDState *s)
 s->ioc = NULL;
 }
 
-if (nbd_co_establish_connection(s->bs, NULL) < 0) {
+s->sioc = nbd_co_establish_connection(s->connect_thread, NULL);
+if (!s->sioc) {
 ret = -ECONNREFUSED;
 goto out;
 }
 
+yank_register_function(BLOCKDEV_YANK_INSTANCE(s->bs->node_name), nbd_yank,
+   s->bs);
+
 bdrv_dec_in_flight(s->bs);
 
 ret = nbd_client_handshake(s->bs, NULL);
-- 
2.29.2