Libguestfs - May 2019 - [nbdkit PATCH 2/2] nbd: Add shared=true parameter

I got annoyed by qemu-nbd's default of only allowing a single
connection; combine that with nbdkit's nbd plugin, and even 'qemu-nbd
--list' of nbdkit counts as the single connection and immediately
hangs up. If we introduce a shared mode, then 'qemu-nbd --list' can
connect as many times as it wants without killing the original
qemu-nbd wrapped by nbdkit.  But this in turn required a way to wait
for the socket to appear (as nbdkit --exit-with-parent has to be
invoked before exec qemu-nbd, but the socket isn't created until the
latter case).

RFC in part because I wonder if nbdkit should offer a shared mode to
ALL plugins, rather than just the nbd plugin providing its own.

Eric Blake (2):
  nbd: Add retry=N parameter
  nbd: Add shared=true parameter

 plugins/nbd/nbdkit-nbd-plugin.pod |  22 ++++-
 plugins/nbd/nbd.c                 | 134 +++++++++++++++++++++---------
 2 files changed, 115 insertions(+), 41 deletions(-)

-- 
2.20.1

A common command line use case for the nbd plugin is
 ( sock=`mktemp -u` &&
   nbdkit --exit-with-parent nbd socket=$sock &
   exec /path/to/server -k $sock)

As long as the first client to nbdkit doesn't try to connect until
after /path/to/server has created $sock, we are fine. But on a heavily
loaded system, it would be nice to give nbdkit some leeway to try
again to connect to the socket if a client connects quickly to nbdkit.

This becomes even more useful in the next patch when adding an option
for the nbd plugin to connect during .config_complete rather than
during .open, as the window of time for the external server to make
the socket available is no longer dependent on when nbdkit's first
client arrives.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/nbd/nbdkit-nbd-plugin.pod |  6 ++++++
 plugins/nbd/nbd.c                 | 24 ++++++++++++++++++++++--
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/plugins/nbd/nbdkit-nbd-plugin.pod
b/plugins/nbd/nbdkit-nbd-plugin.pod
index 913d1e5..4f9efa1 100644
--- a/plugins/nbd/nbdkit-nbd-plugin.pod
+++ b/plugins/nbd/nbdkit-nbd-plugin.pod
@@ -5,6 +5,7 @@ nbdkit-nbd-plugin - nbdkit nbd plugin
 =head1 SYNOPSIS

  nbdkit nbd { socket=SOCKNAME | hostname=HOST [port=PORT] } [export=NAME]
+    [retry=N]

 =head1 DESCRIPTION

@@ -50,6 +51,11 @@ If this parameter is given, and the server speaks new style
protocol,
 then connect to the named export instead of the default export (the
 empty string).

+=item B<retry=>N
+
+If the initial connection attempt to the server fails, retry up to
+B<N> times more after a second delay between tries (default 0).
+
 =back

 =head1 EXAMPLES
diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
index 1195eb8..6665cd9 100644
--- a/plugins/nbd/nbd.c
+++ b/plugins/nbd/nbd.c
@@ -70,6 +70,9 @@ static char *servname;
 /* Name of export on remote server, default '', ignored for oldstyle */
 static const char *export;

+/* Number of retries */
+static unsigned long retry;
+
 static void
 nbd_unload (void)
 {
@@ -79,11 +82,14 @@ nbd_unload (void)

 /* Called for each key=value passed on the command line.  This plugin
  * accepts socket=<sockname> or
hostname=<hostname>/port=<port>
- * (exactly one connection required) and export=<name> (optional).
+ * (exactly one connection required), and optional parameters
+ * export=<name>, retry=<n>.
  */
 static int
 nbd_config (const char *key, const char *value)
 {
+  char *end;
+
   if (strcmp (key, "socket") == 0) {
     /* See FILENAMES AND PATHS in nbdkit-plugin(3) */
     free (sockname);
@@ -97,6 +103,14 @@ nbd_config (const char *key, const char *value)
     port = value;
   else if (strcmp (key, "export") == 0)
     export = value;
+  else if (strcmp (key, "retry") == 0) {
+    errno = 0;
+    retry = strtoul (value, &end, 0);
+    if (value == end || errno) {
+      nbdkit_error ("could not parse retry as integer (%s)", value);
+      return -1;
+    }
+  }
   else {
     nbdkit_error ("unknown parameter '%s'", key);
     return -1;
@@ -960,12 +974,18 @@ nbd_open (int readonly)
     return NULL;
   }

+ retry:
   if (sockname)
     h->fd = nbd_connect_unix ();
   else
     h->fd = nbd_connect_tcp ();
-  if (h->fd == -1)
+  if (h->fd == -1) {
+    if (retry--) {
+      sleep (1);
+      goto retry;
+    }
     goto err;
+  }

   /* old and new handshake share same meaning of first 16 bytes */
   if (read_full (h->fd, &old, offsetof (struct old_handshake,
exportsize))) {
-- 
2.20.1

qemu-nbd defaults to permitting only a single NBD client. Of course,
you can run qemu-nbd -t to work around it, but other servers may have
similar restrictions, at which point nbdkit's nbd plugin can provide
the automatic fanout to multiple clients via a single server link.

Note that the shared=true parameter makes the previously-added retry=N
parameter more useful, as it is much easier to run into the scenario
where the server has not created the socket in time for
.config_complete, when compared to a later .open from nbdkit's first
client.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/nbd/nbdkit-nbd-plugin.pod |  18 ++++-
 plugins/nbd/nbd.c                 | 110 ++++++++++++++++++++----------
 2 files changed, 88 insertions(+), 40 deletions(-)

diff --git a/plugins/nbd/nbdkit-nbd-plugin.pod
b/plugins/nbd/nbdkit-nbd-plugin.pod
index 4f9efa1..8dd8681 100644
--- a/plugins/nbd/nbdkit-nbd-plugin.pod
+++ b/plugins/nbd/nbdkit-nbd-plugin.pod
@@ -5,7 +5,7 @@ nbdkit-nbd-plugin - nbdkit nbd plugin
 =head1 SYNOPSIS

  nbdkit nbd { socket=SOCKNAME | hostname=HOST [port=PORT] } [export=NAME]
-    [retry=N]
+    [retry=N] [shared=BOOL]

 =head1 DESCRIPTION

@@ -56,6 +56,15 @@ empty string).
 If the initial connection attempt to the server fails, retry up to
 B<N> times more after a second delay between tries (default 0).

+=item B<shared=>BOOL
+
+If this parameter is false (default), the plugin will open a distinct
+connection to the server for each client making a connection to
+nbdkit, and the remote server need not be running at the time nbdkit
+is started. If this parameter is set to true, the plugin will open a
+single connection to the server when nbdkit is first started, and all
+clients to nbdkit will share that single connection.
+
 =back

 =head1 EXAMPLES
@@ -76,10 +85,13 @@ that the old server exits.
 Combine nbdkit's partition filter with qemu-nbd's ability to visit
 qcow2 files (nbdkit does not have a native qcow2 plugin), performing
 the same task as the deprecated C<qemu-nbd -P 1 -f qcow2
-/path/to/image.qcow2> command:
+/path/to/image.qcow2> command. Allow multiple clients, even though
+C<qemu-nbd> without B<-t> normally quits after the first client,
and
+utilize a 5-second retry to give qemu-nbd time to create the socket:

  ( sock=`mktemp -u`
-   nbdkit --exit-with-parent --filter=partition nbd socket=$sock partition=1
&
+   nbdkit --exit-with-parent --filter=partition nbd socket=$sock \
+     shared=1 retry=5 partition=1 &
    exec qemu-nbd -k $sock -f qcow2 /path/to/image.qcow2 )

 Conversely, expose the contents of export I<foo> from a new style
diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
index 6665cd9..9389a55 100644
--- a/plugins/nbd/nbd.c
+++ b/plugins/nbd/nbd.c
@@ -57,6 +57,37 @@
 #include "byte-swapping.h"
 #include "cleanup.h"

+/* The per-transaction details */
+struct transaction {
+  uint64_t cookie;
+  sem_t sem;
+  void *buf;
+  uint64_t offset;
+  uint32_t count;
+  uint32_t err;
+  struct nbdkit_extents *extents;
+  struct transaction *next;
+};
+
+/* The per-connection handle */
+struct handle {
+  /* These fields are read-only once initialized */
+  int fd;
+  int flags;
+  int64_t size;
+  bool structured;
+  bool extents;
+  pthread_t reader;
+
+  /* Prevents concurrent threads from interleaving writes to server */
+  pthread_mutex_t write_lock;
+
+  pthread_mutex_t trans_lock; /* Covers access to all fields below */
+  struct transaction *trans;
+  uint64_t unique;
+  bool dead;
+};
+
 /* Connect to server via absolute name of Unix socket */
 static char *sockname;

@@ -73,9 +104,18 @@ static const char *export;
 /* Number of retries */
 static unsigned long retry;

+/* True to share single server connection among all clients */
+static bool shared;
+static struct handle *shared_handle;
+
+static struct handle *nbd_open_handle (int readonly);
+static void nbd_close_handle (struct handle *h);
+
 static void
 nbd_unload (void)
 {
+  if (shared)
+    nbd_close_handle (shared_handle);
   free (sockname);
   free (servname);
 }
@@ -89,6 +129,7 @@ static int
 nbd_config (const char *key, const char *value)
 {
   char *end;
+  int r;

   if (strcmp (key, "socket") == 0) {
     /* See FILENAMES AND PATHS in nbdkit-plugin(3) */
@@ -111,6 +152,12 @@ nbd_config (const char *key, const char *value)
       return -1;
     }
   }
+  else if (strcmp (key, "shared") == 0) {
+    r = nbdkit_parse_bool (value);
+    if (r == -1)
+      return -1;
+    shared = r;
+  }
   else {
     nbdkit_error ("unknown parameter '%s'", key);
     return -1;
@@ -157,6 +204,9 @@ nbd_config_complete (void)

   if (!export)
     export = "";
+
+  if (shared && (shared_handle = nbd_open_handle (false)) == NULL)
+    return -1;
   return 0;
 }

@@ -168,37 +218,6 @@ nbd_config_complete (void)

 #define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL

-/* The per-transaction details */
-struct transaction {
-  uint64_t cookie;
-  sem_t sem;
-  void *buf;
-  uint64_t offset;
-  uint32_t count;
-  uint32_t err;
-  struct nbdkit_extents *extents;
-  struct transaction *next;
-};
-
-/* The per-connection handle */
-struct handle {
-  /* These fields are read-only once initialized */
-  int fd;
-  int flags;
-  int64_t size;
-  bool structured;
-  bool extents;
-  pthread_t reader;
-
-  /* Prevents concurrent threads from interleaving writes to server */
-  pthread_mutex_t write_lock;
-
-  pthread_mutex_t trans_lock; /* Covers access to all fields below */
-  struct transaction *trans;
-  uint64_t unique;
-  bool dead;
-};
-
 /* Read an entire buffer, returning 0 on success or -1 with errno set. */
 static int
 read_full (int fd, void *buf, size_t len)
@@ -960,9 +979,9 @@ nbd_connect_tcp (void)
   return fd;
 }

-/* Create the per-connection handle. */
-static void *
-nbd_open (int readonly)
+/* Create the shared or per-connection handle. */
+static struct handle *
+nbd_open_handle (int readonly)
 {
   struct handle *h;
   struct old_handshake old;
@@ -1091,12 +1110,19 @@ nbd_open (int readonly)
   return NULL;
 }

+/* Create the per-connection handle. */
+static void *
+nbd_open (int readonly)
+{
+  if (shared)
+    return shared_handle;
+  return nbd_open_handle (readonly);
+}
+
 /* Free up the per-connection handle. */
 static void
-nbd_close (void *handle)
+nbd_close_handle (struct handle *h)
 {
-  struct handle *h = handle;
-
   if (!h->dead) {
     nbd_request_raw (h, 0, NBD_CMD_DISC, 0, 0, 0, NULL);
     shutdown (h->fd, SHUT_WR);
@@ -1109,6 +1135,16 @@ nbd_close (void *handle)
   free (h);
 }

+/* Free up the per-connection handle. */
+static void
+nbd_close (void *handle)
+{
+  struct handle *h = handle;
+
+  if (!shared)
+    nbd_close_handle (h);
+}
+
 /* Get the file size. */
 static int64_t
 nbd_get_size (void *handle)
-- 
2.20.1

Looks reasonable, ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top