[v3,22/33] block/nbd: pass monitor directly to connection thread

Commit Message

Vladimir Sementsov-Ogievskiy April 16, 2021, 8:09 a.m. UTC

monitor_cur() is used by socket_get_fd, but it doesn't work in
connection thread. Let's monitor directly to cover this thing. We are
going to unify connection establishing path in nbd_open and reconnect,
so we should support fd-passing.

Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
---
 include/block/nbd.h     |  3 ++-
 block/nbd.c             |  5 ++++-
 nbd/client-connection.c | 11 +++++++----
 3 files changed, 13 insertions(+), 6 deletions(-)

Comments

Eric Blake June 3, 2021, 6:16 p.m. UTC | #1

On Fri, Apr 16, 2021 at 11:09:00AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> monitor_cur() is used by socket_get_fd, but it doesn't work in
> connection thread. Let's monitor directly to cover this thing. We are
> going to unify connection establishing path in nbd_open and reconnect,
> so we should support fd-passing.

Grammar suggestion:

Let's pass in the monitor directly to work around this.  This gets us
closer to unifing the path for establishing a connection in nbd_open
and reconnect, by supporting fd-passing.


But given Dan's review on 21/33, I suspect you won't be using this
patch in this form after all (instead, the caller of
nbd_client_connection_new will use the new monitor_resolve_fd or
whatever we call that, so that nbd_client_connection_new remains
oblivious to the monitor).

Vladimir Sementsov-Ogievskiy June 3, 2021, 6:31 p.m. UTC | #2

03.06.2021 21:16, Eric Blake wrote:
> On Fri, Apr 16, 2021 at 11:09:00AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> monitor_cur() is used by socket_get_fd, but it doesn't work in
>> connection thread. Let's monitor directly to cover this thing. We are
>> going to unify connection establishing path in nbd_open and reconnect,
>> so we should support fd-passing.
> 
> Grammar suggestion:
> 
> Let's pass in the monitor directly to work around this.  This gets us
> closer to unifing the path for establishing a connection in nbd_open
> and reconnect, by supporting fd-passing.
> 
> 
> But given Dan's review on 21/33, I suspect you won't be using this
> patch in this form after all (instead, the caller of
> nbd_client_connection_new will use the new monitor_resolve_fd or
> whatever we call that, so that nbd_client_connection_new remains
> oblivious to the monitor).
> 

Yes.. I even have some patches for it locally. Seems I didn't send them, don't remember why :/ Will check tomorrow.

Patch

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 5bb54d831c..10756d2544 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -415,7 +415,8 @@  NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr,
                                                bool do_negotiation,
                                                const char *export_name,
                                                const char *x_dirty_bitmap,
-                                               QCryptoTLSCreds *tlscreds);
+                                               QCryptoTLSCreds *tlscreds,
+                                               Monitor *mon);
 void nbd_client_connection_release(NBDClientConnection *conn);
 
 QIOChannelSocket *coroutine_fn
diff --git a/block/nbd.c b/block/nbd.c
index c1e61a2a52..ec69a4ad65 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -45,6 +45,8 @@ 
 #include "block/nbd.h"
 #include "block/block_int.h"
 
+#include "monitor/monitor.h"
+
 #include "qemu/yank.h"
 
 #define EN_OPTSTR ":exportname="
@@ -2064,7 +2066,8 @@  static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     s->conn = nbd_client_connection_new(s->saddr, true, s->export,
-                                        s->x_dirty_bitmap, s->tlscreds);
+                                        s->x_dirty_bitmap, s->tlscreds,
+                                        monitor_cur());
 
     /*
      * establish TCP connection, return error if it fails
diff --git a/nbd/client-connection.c b/nbd/client-connection.c
index 54f73c6c24..c26cd59464 100644
--- a/nbd/client-connection.c
+++ b/nbd/client-connection.c
@@ -37,6 +37,7 @@  struct NBDClientConnection {
     bool do_negotiation;
 
     bool do_retry;
+    Monitor *mon;
 
     /*
      * Result of last attempt. Valid in FAIL and SUCCESS states.
@@ -67,7 +68,8 @@  NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr,
                                                bool do_negotiation,
                                                const char *export_name,
                                                const char *x_dirty_bitmap,
-                                               QCryptoTLSCreds *tlscreds)
+                                               QCryptoTLSCreds *tlscreds,
+                                               Monitor *mon)
 {
     NBDClientConnection *conn = g_new(NBDClientConnection, 1);
 
@@ -76,6 +78,7 @@  NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr,
         .saddr = QAPI_CLONE(SocketAddress, saddr),
         .tlscreds = tlscreds,
         .do_negotiation = do_negotiation,
+        .mon = mon,
 
         .initial_info.request_sizes = true,
         .initial_info.structured_reply = true,
@@ -110,7 +113,7 @@  static void nbd_client_connection_do_free(NBDClientConnection *conn)
  */
 static int nbd_connect(QIOChannelSocket *sioc, SocketAddress *addr,
                        NBDExportInfo *info, QCryptoTLSCreds *tlscreds,
-                       QIOChannel **outioc, Error **errp)
+                       QIOChannel **outioc, Monitor *mon, Error **errp)
 {
     int ret;
 
@@ -118,7 +121,7 @@  static int nbd_connect(QIOChannelSocket *sioc, SocketAddress *addr,
         *outioc = NULL;
     }
 
-    ret = qio_channel_socket_connect_sync(sioc, addr, errp);
+    ret = qio_channel_socket_connect_sync_mon(sioc, addr, mon, errp);
     if (ret < 0) {
         return ret;
     }
@@ -171,7 +174,7 @@  static void *connect_thread_func(void *opaque)
 
         ret = nbd_connect(conn->sioc, conn->saddr,
                           conn->do_negotiation ? &conn->updated_info : NULL,
-                          conn->tlscreds, &conn->ioc, &conn->err);
+                          conn->tlscreds, &conn->ioc, conn->mon, &conn->err);
         conn->updated_info.x_dirty_bitmap = NULL;
         conn->updated_info.name = NULL;