Libguestfs - Aug 2020 - Re: [PATCH nbdkit 2/2] vddk: Relax thread model to PARALLEL and implement a disk handle pool.

I believe this roughly implements Nir's proposal here:

https://www.redhat.com/archives/libguestfs/2020-August/msg00028.html

Unfortunately it doesn't work for me.  It actually slows things down
quite a lot, for reasons I don't understand.  Note the adjustment of
the pool-max parameter and how it affects the total time.  The results
are quite reproducible.

$ ./nbdkit -r -U - vddk libdir=~/tmp/vddk-7.0.0/vmware-vix-disklib-distrib
user=root password=*** server=*** thumbprint=*** vm=moref=3
file='[datastore1] Fedora 28/Fedora 28.vmdk' pool-max=1 --run 'time
qemu-img convert -p -W -m 16 $nbd /var/tmp/out'
    (100.00/100%)

real	1m8.031s
user	0m0.112s
sys	0m1.560s

$ ./nbdkit -r -U - vddk libdir=~/tmp/vddk-7.0.0/vmware-vix-disklib-distrib
user=root password=*** server=*** thumbprint=*** vm=moref=3
file='[datastore1] Fedora 28/Fedora 28.vmdk' pool-max=8 --run 'time
qemu-img convert -p -W -m 16 $nbd /var/tmp/out'
    (100.00/100%)

real	1m27.790s
user	0m0.099s
sys	0m1.625s

I'm still investigating this.  Could be a simple coding error somewhere.

Rich.

See comment in code and
https://www.redhat.com/archives/libguestfs/2020-August/msg00023.html
---
 plugins/vddk/vddk.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index c24da96f..5926e181 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -478,11 +478,18 @@ vddk_dump_plugin (void)
 #endif
 }
 
-/* XXX To really do threading correctly in accordance with the VDDK
- * documentation, we must do all open/close calls from a single
- * thread.  This is a huge pain.
+/* The rules on threads and VDDK are here:
+ *
https://code.vmware.com/docs/11750/virtual-disk-development-kit-programming-guide/GUID-6BE903E8-DC70-46D9-98E4-E34A2002C2AD.html
+ *
+ * Before nbdkit 1.22 we used SERIALIZE_ALL_REQUESTS.  Since nbdkit
+ * 1.22 we changed this to SERIALIZE_REQUESTS and added a mutex around
+ * calls to VixDiskLib_Open and VixDiskLib_Close.  This is not quite
+ * within the letter of the rules, but is within the spirit.
  */
-#define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS
+#define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS
+
+/* Lock protecting open/close calls - see above. */
+static pthread_mutex_t open_close_lock = PTHREAD_MUTEX_INITIALIZER;
 
 /* The per-connection handle. */
 struct vddk_handle {
@@ -524,6 +531,7 @@ free_connect_params (VixDiskLibConnectParams *params)
 static void *
 vddk_open (int readonly)
 {
+  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&open_close_lock);
   struct vddk_handle *h;
   VixError err;
   uint32_t flags;
@@ -616,6 +624,7 @@ vddk_open (int readonly)
 static void
 vddk_close (void *handle)
 {
+  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&open_close_lock);
   struct vddk_handle *h = handle;
 
   DEBUG_CALL ("VixDiskLib_Close", "handle");
-- 
2.27.0

The pool is only used for readonly connections, since writable
connections usually take a lock on the server side and therefore you
cannot open more than one.
---
 plugins/vddk/nbdkit-vddk-plugin.pod |   7 +
 plugins/vddk/vddk.c                 | 201 ++++++++++++++++++++++------
 2 files changed, 164 insertions(+), 44 deletions(-)

diff --git a/plugins/vddk/nbdkit-vddk-plugin.pod
b/plugins/vddk/nbdkit-vddk-plugin.pod
index e5539da9..288aed4b 100644
--- a/plugins/vddk/nbdkit-vddk-plugin.pod
+++ b/plugins/vddk/nbdkit-vddk-plugin.pod
@@ -175,6 +175,13 @@ Read the password from file descriptor number C<FD>,
inherited from
 the parent process when nbdkit starts up.  This is also a secure
 method to supply a password.
 
+=item B<pool-max=>N
+
+To improve performance, for read-only connections (see I<-r> option)
+the plugin will open a pool of VixDiskLibHandle disk handles.  You can
+use this option to control the maximum size of the pool.  The default
+is 8.  To disable this feature, set it to 0 or 1.
+
 =item B<port=>PORT
 
 The port on the VCenter/ESXi host.  Defaults to 443.
diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index 5926e181..173d5031 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -52,6 +52,7 @@
 #include "isaligned.h"
 #include "minmax.h"
 #include "rounding.h"
+#include "vector.h"
 
 #include "vddk.h"
 #include "vddk-structs.h"
@@ -85,6 +86,7 @@ char *libdir;                              /* libdir */
 static uint16_t nfc_host_port;             /* nfchostport */
 char *password;                            /* password */
 static uint16_t port;                      /* port */
+static unsigned pool_max = 8;              /* pool-max */
 static const char *server_name;            /* server */
 static bool single_link;                   /* single-link */
 static const char *snapshot_moref;         /* snapshot */
@@ -233,6 +235,12 @@ vddk_config (const char *key, const char *value)
     if (nbdkit_parse_uint16_t ("port", value, &port) == -1)
       return -1;
   }
+  else if (strcmp (key, "pool-max") == 0) {
+    if (nbdkit_parse_unsigned ("pool-max", value, &pool_max) ==
-1)
+      return -1;
+    if (pool_max < 1)
+      pool_max = 1;
+  }
   else if (strcmp (key, "reexeced_") == 0) {
     /* Special name because it is only for internal use. */
     reexeced = (char *)value;
@@ -482,20 +490,37 @@ vddk_dump_plugin (void)
  *
https://code.vmware.com/docs/11750/virtual-disk-development-kit-programming-guide/GUID-6BE903E8-DC70-46D9-98E4-E34A2002C2AD.html
  *
  * Before nbdkit 1.22 we used SERIALIZE_ALL_REQUESTS.  Since nbdkit
- * 1.22 we changed this to SERIALIZE_REQUESTS and added a mutex around
- * calls to VixDiskLib_Open and VixDiskLib_Close.  This is not quite
- * within the letter of the rules, but is within the spirit.
+ * 1.22 we changed this to PARALLEL, added a mutex around calls to
+ * VixDiskLib_Open and VixDiskLib_Close, and use a pool of disk
+ * handles.  This is not quite within the letter of the rules, but is
+ * within the spirit.
  */
-#define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS
+#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
 
 /* Lock protecting open/close calls - see above. */
 static pthread_mutex_t open_close_lock = PTHREAD_MUTEX_INITIALIZER;
 
-/* The per-connection handle. */
+struct handle;
 struct vddk_handle {
+  VixDiskLibHandle vddk_handle;
+  bool in_use;
+  struct handle *h;
+};
+DEFINE_VECTOR_TYPE(vddk_handles, struct vddk_handle)
+
+/* The per-connection handle. */
+struct handle {
   VixDiskLibConnectParams *params; /* connection parameters */
   VixDiskLibConnection connection; /* connection */
-  VixDiskLibHandle handle;         /* disk handle */
+  int readonly;                    /* readonly flag for this connection */
+  uint32_t flags;                  /* open flags */
+
+  /* Pool of VDDK disk handles.  Do not access this directly, use
+   * GET_VDDK_HANDLE_FOR_CURRENT_SCOPE macro to get a free handle.
+   */
+  pthread_mutex_t vddk_handles_lock;
+  pthread_cond_t vddk_handles_cond;
+  vddk_handles vddk_handles;
 };
 
 static inline VixDiskLibConnectParams *
@@ -531,17 +556,28 @@ free_connect_params (VixDiskLibConnectParams *params)
 static void *
 vddk_open (int readonly)
 {
-  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&open_close_lock);
-  struct vddk_handle *h;
+  struct handle *h;
   VixError err;
-  uint32_t flags;
 
-  h = malloc (sizeof *h);
+  h = calloc (1, sizeof *h);
   if (h == NULL) {
-    nbdkit_error ("malloc: %m");
+    nbdkit_error ("calloc: %m");
     return NULL;
   }
 
+  h->readonly = readonly;
+  pthread_mutex_init (&h->vddk_handles_lock, NULL);
+  pthread_cond_init (&h->vddk_handles_cond, NULL);
+
+  /* We have to reserve this vector to ensure that it is not
+   * reallocated, as that would make the pointer returned by
+   * get_vddk_handle in another thread invalid.
+   */
+  if (vddk_handles_reserve (&h->vddk_handles, pool_max) == -1) {
+    nbdkit_error ("realloc: %m");
+    goto err0;
+  }
+
   h->params = allocate_connect_params ();
   if (h->params == NULL) {
     nbdkit_error ("allocate VixDiskLibConnectParams: %m");
@@ -589,49 +625,120 @@ vddk_open (int readonly)
     goto err1;
   }
 
-  flags = 0;
+  h->flags = 0;
   if (readonly)
-    flags |= VIXDISKLIB_FLAG_OPEN_READ_ONLY;
+    h->flags |= VIXDISKLIB_FLAG_OPEN_READ_ONLY;
   if (single_link)
-    flags |= VIXDISKLIB_FLAG_OPEN_SINGLE_LINK;
+    h->flags |= VIXDISKLIB_FLAG_OPEN_SINGLE_LINK;
   if (unbuffered)
-    flags |= VIXDISKLIB_FLAG_OPEN_UNBUFFERED;
-
-  DEBUG_CALL ("VixDiskLib_Open",
-              "connection, %s, %d, &handle", filename, flags);
-  err = VixDiskLib_Open (h->connection, filename, flags, &h->handle);
-  if (err != VIX_OK) {
-    VDDK_ERROR (err, "VixDiskLib_Open: %s", filename);
-    goto err2;
-  }
-
-  nbdkit_debug ("transport mode: %s",
-                VixDiskLib_GetTransportMode (h->handle));
+    h->flags |= VIXDISKLIB_FLAG_OPEN_UNBUFFERED;
 
   return h;
 
- err2:
-  DEBUG_CALL ("VixDiskLib_Disconnect", "connection");
-  VixDiskLib_Disconnect (h->connection);
  err1:
   free_connect_params (h->params);
  err0:
+  free (h->vddk_handles.ptr);
+  pthread_cond_destroy (&h->vddk_handles_cond);
+  pthread_mutex_destroy (&h->vddk_handles_lock);
   free (h);
   return NULL;
 }
 
+/* Get a VDDK handle on demand. */
+static VixDiskLibHandle
+open_vddk_handle (struct handle *h)
+{
+  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&open_close_lock);
+  VixDiskLibHandle vddk_handle;
+  VixError err;
+
+  DEBUG_CALL ("VixDiskLib_Open",
+              "connection, %s, %d, &handle", filename,
h->flags);
+  err = VixDiskLib_Open (h->connection, filename, h->flags,
&vddk_handle);
+  if (err != VIX_OK) {
+    VDDK_ERROR (err, "VixDiskLib_Open: %s", filename);
+    return NULL;
+  }
+
+  nbdkit_debug ("transport mode: %s",
+                VixDiskLib_GetTransportMode (vddk_handle));
+  return vddk_handle;
+}
+
+static struct vddk_handle *
+get_vddk_handle (struct handle *h)
+{
+  const unsigned max = h->readonly ? pool_max : 1;
+  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&h->vddk_handles_lock);
+  VixDiskLibHandle vddk_handle;
+  size_t i;
+
+ again:
+  /* See if there's a handle in the pool which is not in use. */
+  for (i = 0; i < h->vddk_handles.size; ++i) {
+    if (!h->vddk_handles.ptr[i].in_use) {
+      h->vddk_handles.ptr[i].in_use = true;
+      return &h->vddk_handles.ptr[i];
+    }
+  }
+
+  /* If the pool is too big we have to wait for another thread to
+   * finish using its handle and try again.
+   */
+  if (h->vddk_handles.size >= max) {
+    pthread_cond_wait (&h->vddk_handles_cond,
&h->vddk_handles_lock);
+    goto again;
+  }
+
+  /* Open another handle and add it to the pool.  Note that
+   * vddk_handles_append cannot fail because we reserved space in
+   * vddk_open.
+   */
+  vddk_handle = open_vddk_handle (h);
+  if (vddk_handle == NULL)
+    return NULL;
+  vddk_handles_append (&h->vddk_handles,
+                       (struct vddk_handle){ vddk_handle, true, h });
+  return &h->vddk_handles.ptr[h->vddk_handles.size-1];
+}
+
+static void
+put_vddk_handle (struct vddk_handle **p)
+{
+  if (*p) {
+    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&(*p)->h->vddk_handles_lock);
+    assert ((*p)->in_use);
+    (*p)->in_use = false;
+    pthread_cond_signal (&(*p)->h->vddk_handles_cond);
+  }
+}
+
+/* Wrap get/put_vddk_handle in nicer macro. */
+#define GET_VDDK_HANDLE_FOR_CURRENT_SCOPE(h, name)      \
+  __attribute__((cleanup (put_vddk_handle)))            \
+  struct vddk_handle *name##_h = get_vddk_handle (h);   \
+  if (name##_h == NULL) return -1;                      \
+  VixDiskLibHandle name = name##_h->vddk_handle
+
 /* Free up the per-connection handle. */
 static void
 vddk_close (void *handle)
 {
   ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&open_close_lock);
-  struct vddk_handle *h = handle;
+  struct handle *h = handle;
+  size_t i;
 
-  DEBUG_CALL ("VixDiskLib_Close", "handle");
-  VixDiskLib_Close (h->handle);
+  for (i = 0; i < h->vddk_handles.size; ++i) {
+    DEBUG_CALL ("VixDiskLib_Close", "handle");
+    VixDiskLib_Close (h->vddk_handles.ptr[i].vddk_handle);
+  }
+  free (h->vddk_handles.ptr);
   DEBUG_CALL ("VixDiskLib_Disconnect", "connection");
   VixDiskLib_Disconnect (h->connection);
   free_connect_params (h->params);
+  pthread_cond_destroy (&h->vddk_handles_cond);
+  pthread_mutex_destroy (&h->vddk_handles_lock);
   free (h);
 }
 
@@ -639,13 +746,14 @@ vddk_close (void *handle)
 static int64_t
 vddk_get_size (void *handle)
 {
-  struct vddk_handle *h = handle;
+  struct handle *h = handle;
+  GET_VDDK_HANDLE_FOR_CURRENT_SCOPE (h, vddk_handle);
   VixDiskLibInfo *info;
   VixError err;
   uint64_t size;
 
   DEBUG_CALL ("VixDiskLib_GetInfo", "handle, &info");
-  err = VixDiskLib_GetInfo (h->handle, &info);
+  err = VixDiskLib_GetInfo (vddk_handle, &info);
   if (err != VIX_OK) {
     VDDK_ERROR (err, "VixDiskLib_GetInfo");
     return -1;
@@ -687,7 +795,8 @@ static int
 vddk_pread (void *handle, void *buf, uint32_t count, uint64_t offset,
             uint32_t flags)
 {
-  struct vddk_handle *h = handle;
+  struct handle *h = handle;
+  GET_VDDK_HANDLE_FOR_CURRENT_SCOPE (h, vddk_handle);
   VixError err;
 
   /* Align to sectors. */
@@ -706,7 +815,7 @@ vddk_pread (void *handle, void *buf, uint32_t count,
uint64_t offset,
                        "handle, %" PRIu64 " sectors, "
                        "%" PRIu32 " sectors, buffer",
                        offset, count);
-  err = VixDiskLib_Read (h->handle, offset, count, buf);
+  err = VixDiskLib_Read (vddk_handle, offset, count, buf);
   if (err != VIX_OK) {
     VDDK_ERROR (err, "VixDiskLib_Read");
     return -1;
@@ -726,7 +835,8 @@ vddk_pwrite (void *handle, const void *buf, uint32_t count,
uint64_t offset,
              uint32_t flags)
 {
   const bool fua = flags & NBDKIT_FLAG_FUA;
-  struct vddk_handle *h = handle;
+  struct handle *h = handle;
+  GET_VDDK_HANDLE_FOR_CURRENT_SCOPE (h, vddk_handle);
   VixError err;
 
   /* Align to sectors. */
@@ -745,7 +855,7 @@ vddk_pwrite (void *handle, const void *buf, uint32_t count,
uint64_t offset,
                        "handle, %" PRIu64 " sectors, "
                        "%" PRIu32 " sectors, buffer",
                        offset, count);
-  err = VixDiskLib_Write (h->handle, offset, count, buf);
+  err = VixDiskLib_Write (vddk_handle, offset, count, buf);
   if (err != VIX_OK) {
     VDDK_ERROR (err, "VixDiskLib_Write");
     return -1;
@@ -761,7 +871,8 @@ vddk_pwrite (void *handle, const void *buf, uint32_t count,
uint64_t offset,
 static int
 vddk_flush (void *handle, uint32_t flags)
 {
-  struct vddk_handle *h = handle;
+  struct handle *h = handle;
+  GET_VDDK_HANDLE_FOR_CURRENT_SCOPE (h, vddk_handle);
   VixError err;
 
   /* The Flush call was not available in VDDK < 6.0 so this is simply
@@ -771,7 +882,7 @@ vddk_flush (void *handle, uint32_t flags)
     return 0;
 
   DEBUG_CALL ("VixDiskLib_Flush", "handle");
-  err = VixDiskLib_Flush (h->handle);
+  err = VixDiskLib_Flush (vddk_handle);
   if (err != VIX_OK) {
     VDDK_ERROR (err, "VixDiskLib_Flush");
     return -1;
@@ -783,7 +894,8 @@ vddk_flush (void *handle, uint32_t flags)
 static int
 vddk_can_extents (void *handle)
 {
-  struct vddk_handle *h = handle;
+  struct handle *h = handle;
+  GET_VDDK_HANDLE_FOR_CURRENT_SCOPE (h, vddk_handle);
   VixError err;
   VixDiskLibBlockList *block_list;
 
@@ -808,7 +920,7 @@ vddk_can_extents (void *handle)
   DEBUG_CALL ("VixDiskLib_QueryAllocatedBlocks",
               "handle, 0, %d sectors, %d sectors",
               VIXDISKLIB_MIN_CHUNK_SIZE, VIXDISKLIB_MIN_CHUNK_SIZE);
-  err = VixDiskLib_QueryAllocatedBlocks (h->handle,
+  err = VixDiskLib_QueryAllocatedBlocks (vddk_handle,
                                          0, VIXDISKLIB_MIN_CHUNK_SIZE,
                                          VIXDISKLIB_MIN_CHUNK_SIZE,
                                          &block_list);
@@ -864,7 +976,8 @@ static int
 vddk_extents (void *handle, uint32_t count, uint64_t offset, uint32_t flags,
               struct nbdkit_extents *extents)
 {
-  struct vddk_handle *h = handle;
+  struct handle *h = handle;
+  GET_VDDK_HANDLE_FOR_CURRENT_SCOPE (h, vddk_handle);
   bool req_one = flags & NBDKIT_FLAG_REQ_ONE;
   uint64_t position, end, start_sector;
 
@@ -896,7 +1009,7 @@ vddk_extents (void *handle, uint32_t count, uint64_t
offset, uint32_t flags,
                 "handle, %" PRIu64 " sectors, %" PRIu64
" sectors, "
                 "%d sectors",
                 start_sector, nr_sectors, VIXDISKLIB_MIN_CHUNK_SIZE);
-    err = VixDiskLib_QueryAllocatedBlocks (h->handle,
+    err = VixDiskLib_QueryAllocatedBlocks (vddk_handle,
                                            start_sector, nr_sectors,
                                            VIXDISKLIB_MIN_CHUNK_SIZE,
                                            &block_list);
-- 
2.27.0

Even running this with lots of extra debugging it seems to be doing
what is expected.  I can't see the bug if there is one.

Note I'm only testing qemu-img convert as that is what's relevant to
virt-v2v.  Previously I was using a hand-written test using libnbd
that did random reads across the disk, and showed a modest improvement
with 8 handles.

Here are the results across a range of pool sizes, all with -W -m 16:

1 handle:
real	1m8.031s
user	0m0.112s
sys	0m1.560s

2 handles:
real	1m6.465s
user	0m0.106s
sys	0m1.607s

4 handles:
real	1m21.488s
user	0m0.126s
sys	0m1.620s

8 handles:
real	1m27.790s
user	0m0.099s
sys	0m1.625s

16 handles:
real	1m33.974s
user	0m0.124s
sys	0m1.718s

I also tried matching the number of coroutines to the number
of handles in the pool (ie pool-max == -m):

1 coroutine, 1 handle:
real	     1m9.545s
user	     0m0.083s
sys	     0m1.592s

2 coroutines, 2 handles:
real	      1m6.130s
user	      0m0.078s
sys	      0m1.567s

4 coroutines, 4 handles:
real	      1m22.109s
user	      0m0.107s
sys	      0m1.690s

8 coroutines, 8 handles:
real	      1m26.490s
user	      0m0.108s
sys	      0m1.650s

16 coroutines, 16 handles (same as last result above):
real	1m33.974s
user	0m0.124s
sys	0m1.718s


Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Thu, Aug 6, 2020, 16:16 Richard W.M. Jones <rjones@redhat.com> wrote:
> See comment in code and
> https://www.redhat.com/archives/libguestfs/2020-August/msg00023.html
> ---
>  plugins/vddk/vddk.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
> index c24da96f..5926e181 100644
> --- a/plugins/vddk/vddk.c
> +++ b/plugins/vddk/vddk.c
> @@ -478,11 +478,18 @@ vddk_dump_plugin (void)
>  #endif
>  }
>
> -/* XXX To really do threading correctly in accordance with the VDDK
> - * documentation, we must do all open/close calls from a single
> - * thread.  This is a huge pain.
> +/* The rules on threads and VDDK are here:
> + *
>
https://code.vmware.com/docs/11750/virtual-disk-development-kit-programming-guide/GUID-6BE903E8-DC70-46D9-98E4-E34A2002C2AD.html
> + *
> + * Before nbdkit 1.22 we used SERIALIZE_ALL_REQUESTS.  Since nbdkit
> + * 1.22 we changed this to SERIALIZE_REQUESTS and added a mutex around
> + * calls to VixDiskLib_Open and VixDiskLib_Close.  This is not quite
> + * within the letter of the rules, but is within the spirit.
>
The document is very clear about using the same thread for open an close.
Using a lock is not the same.

I think Eric already wrote about this.

  */
> -#define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS
> +#define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS
> +
> +/* Lock protecting open/close calls - see above. */
> +static pthread_mutex_t open_close_lock = PTHREAD_MUTEX_INITIALIZER;
>
>  /* The per-connection handle. */
>  struct vddk_handle {
> @@ -524,6 +531,7 @@ free_connect_params (VixDiskLibConnectParams *params)
>  static void *
>  vddk_open (int readonly)
>  {
> +  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&open_close_lock);
>    struct vddk_handle *h;
>    VixError err;
>    uint32_t flags;
> @@ -616,6 +624,7 @@ vddk_open (int readonly)
>  static void
>  vddk_close (void *handle)
>  {
> +  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&open_close_lock);
>    struct vddk_handle *h = handle;
>
>    DEBUG_CALL ("VixDiskLib_Close", "handle");
> --
> 2.27.0
>
>

On Thu, Aug 6, 2020, 16:16 Richard W.M. Jones <rjones@redhat.com> wrote:
> The pool is only used for readonly connections, since writable
> connections usually take a lock on the server side and therefore you
> cannot open more than one.
> ---
>  plugins/vddk/nbdkit-vddk-plugin.pod |   7 +
>  plugins/vddk/vddk.c                 | 201 ++++++++++++++++++++++------
>  2 files changed, 164 insertions(+), 44 deletions(-)
>
> diff --git a/plugins/vddk/nbdkit-vddk-plugin.pod
> b/plugins/vddk/nbdkit-vddk-plugin.pod
> index e5539da9..288aed4b 100644
> --- a/plugins/vddk/nbdkit-vddk-plugin.pod
> +++ b/plugins/vddk/nbdkit-vddk-plugin.pod
> @@ -175,6 +175,13 @@ Read the password from file descriptor number
C<FD>,
> inherited from
>  the parent process when nbdkit starts up.  This is also a secure
>  method to supply a password.
>
> +=item B<pool-max=>N
> +
> +To improve performance, for read-only connections (see I<-r> option)
> +the plugin will open a pool of VixDiskLibHandle disk handles.  You can
> +use this option to control the maximum size of the pool.  The default
> +is 8.  To disable this feature, set it to 0 or 1.
> +
>  =item B<port=>PORT
>
>  The port on the VCenter/ESXi host.  Defaults to 443.
> diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
> index 5926e181..173d5031 100644
> --- a/plugins/vddk/vddk.c
> +++ b/plugins/vddk/vddk.c
> @@ -52,6 +52,7 @@
>  #include "isaligned.h"
>  #include "minmax.h"
>  #include "rounding.h"
> +#include "vector.h"
>
>  #include "vddk.h"
>  #include "vddk-structs.h"
> @@ -85,6 +86,7 @@ char *libdir;                              /* libdir */
>  static uint16_t nfc_host_port;             /* nfchostport */
>  char *password;                            /* password */
>  static uint16_t port;                      /* port */
> +static unsigned pool_max = 8;              /* pool-max */
>  static const char *server_name;            /* server */
>  static bool single_link;                   /* single-link */
>  static const char *snapshot_moref;         /* snapshot */
> @@ -233,6 +235,12 @@ vddk_config (const char *key, const char *value)
>      if (nbdkit_parse_uint16_t ("port", value, &port) == -1)
>        return -1;
>    }
> +  else if (strcmp (key, "pool-max") == 0) {
> +    if (nbdkit_parse_unsigned ("pool-max", value, &pool_max)
== -1)
> +      return -1;
> +    if (pool_max < 1)
> +      pool_max = 1;
> +  }
>    else if (strcmp (key, "reexeced_") == 0) {
>      /* Special name because it is only for internal use. */
>      reexeced = (char *)value;
> @@ -482,20 +490,37 @@ vddk_dump_plugin (void)
>   *
>
https://code.vmware.com/docs/11750/virtual-disk-development-kit-programming-guide/GUID-6BE903E8-DC70-46D9-98E4-E34A2002C2AD.html
>   *
>   * Before nbdkit 1.22 we used SERIALIZE_ALL_REQUESTS.  Since nbdkit
> - * 1.22 we changed this to SERIALIZE_REQUESTS and added a mutex around
> - * calls to VixDiskLib_Open and VixDiskLib_Close.  This is not quite
> - * within the letter of the rules, but is within the spirit.
> + * 1.22 we changed this to PARALLEL, added a mutex around calls to
> + * VixDiskLib_Open and VixDiskLib_Close, and use a pool of disk
> + * handles.  This is not quite within the letter of the rules, but is
> + * within the spirit.
>   */
> -#define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS
> +#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
>
>  /* Lock protecting open/close calls - see above. */
>  static pthread_mutex_t open_close_lock = PTHREAD_MUTEX_INITIALIZER;
>
> -/* The per-connection handle. */
> +struct handle;
>  struct vddk_handle {
> +  VixDiskLibHandle vddk_handle;
> +  bool in_use;
> +  struct handle *h;
> +};
> +DEFINE_VECTOR_TYPE(vddk_handles, struct vddk_handle)
> +
> +/* The per-connection handle. */
> +struct handle {
>    VixDiskLibConnectParams *params; /* connection parameters */
>    VixDiskLibConnection connection; /* connection */
>
Given the poor results, I suspect that that handles created using same
connection share a lock. This also makes sense if connection abstract a
blocking socket.

With multiple nbd connections we got 60% improvement. I expect to see
similar results if we use multiple connection+handle pairs.

Can you try to move the connection into the vddk_handle struct?

-  VixDiskLibHandle handle;         /* disk handle */
> +  int readonly;                    /* readonly flag for this connection */
> +  uint32_t flags;                  /* open flags */
> +
> +  /* Pool of VDDK disk handles.  Do not access this directly, use
> +   * GET_VDDK_HANDLE_FOR_CURRENT_SCOPE macro to get a free handle.
> +   */
> +  pthread_mutex_t vddk_handles_lock;
> +  pthread_cond_t vddk_handles_cond;
> +  vddk_handles vddk_handles;
>  };
>
>  static inline VixDiskLibConnectParams *
> @@ -531,17 +556,28 @@ free_connect_params (VixDiskLibConnectParams *params)
>  static void *
>  vddk_open (int readonly)
>  {
> -  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&open_close_lock);
> -  struct vddk_handle *h;
> +  struct handle *h;
>    VixError err;
> -  uint32_t flags;
>
> -  h = malloc (sizeof *h);
> +  h = calloc (1, sizeof *h);
>    if (h == NULL) {
> -    nbdkit_error ("malloc: %m");
> +    nbdkit_error ("calloc: %m");
>      return NULL;
>    }
>
> +  h->readonly = readonly;
> +  pthread_mutex_init (&h->vddk_handles_lock, NULL);
> +  pthread_cond_init (&h->vddk_handles_cond, NULL);
> +
> +  /* We have to reserve this vector to ensure that it is not
> +   * reallocated, as that would make the pointer returned by
> +   * get_vddk_handle in another thread invalid.
> +   */
> +  if (vddk_handles_reserve (&h->vddk_handles, pool_max) == -1) {
> +    nbdkit_error ("realloc: %m");
> +    goto err0;
> +  }
> +
>    h->params = allocate_connect_params ();
>    if (h->params == NULL) {
>      nbdkit_error ("allocate VixDiskLibConnectParams: %m");
> @@ -589,49 +625,120 @@ vddk_open (int readonly)
>      goto err1;
>    }
>
> -  flags = 0;
> +  h->flags = 0;
>    if (readonly)
> -    flags |= VIXDISKLIB_FLAG_OPEN_READ_ONLY;
> +    h->flags |= VIXDISKLIB_FLAG_OPEN_READ_ONLY;
>    if (single_link)
> -    flags |= VIXDISKLIB_FLAG_OPEN_SINGLE_LINK;
> +    h->flags |= VIXDISKLIB_FLAG_OPEN_SINGLE_LINK;
>    if (unbuffered)
> -    flags |= VIXDISKLIB_FLAG_OPEN_UNBUFFERED;
> -
> -  DEBUG_CALL ("VixDiskLib_Open",
> -              "connection, %s, %d, &handle", filename,
flags);
> -  err = VixDiskLib_Open (h->connection, filename, flags,
&h->handle);
> -  if (err != VIX_OK) {
> -    VDDK_ERROR (err, "VixDiskLib_Open: %s", filename);
> -    goto err2;
> -  }
> -
> -  nbdkit_debug ("transport mode: %s",
> -                VixDiskLib_GetTransportMode (h->handle));
> +    h->flags |= VIXDISKLIB_FLAG_OPEN_UNBUFFERED;
>
>    return h;
>
> - err2:
> -  DEBUG_CALL ("VixDiskLib_Disconnect", "connection");
> -  VixDiskLib_Disconnect (h->connection);
>   err1:
>    free_connect_params (h->params);
>   err0:
> +  free (h->vddk_handles.ptr);
> +  pthread_cond_destroy (&h->vddk_handles_cond);
> +  pthread_mutex_destroy (&h->vddk_handles_lock);
>    free (h);
>    return NULL;
>  }
>
> +/* Get a VDDK handle on demand. */
> +static VixDiskLibHandle
> +open_vddk_handle (struct handle *h)
> +{
> +  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&open_close_lock);
> +  VixDiskLibHandle vddk_handle;
> +  VixError err;
> +
> +  DEBUG_CALL ("VixDiskLib_Open",
> +              "connection, %s, %d, &handle", filename,
h->flags);
> +  err = VixDiskLib_Open (h->connection, filename, h->flags,
&vddk_handle);
> +  if (err != VIX_OK) {
> +    VDDK_ERROR (err, "VixDiskLib_Open: %s", filename);
> +    return NULL;
> +  }
> +
> +  nbdkit_debug ("transport mode: %s",
> +                VixDiskLib_GetTransportMode (vddk_handle));
> +  return vddk_handle;
> +}
> +
> +static struct vddk_handle *
> +get_vddk_handle (struct handle *h)
> +{
> +  const unsigned max = h->readonly ? pool_max : 1;
> +  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&h->vddk_handles_lock);
> +  VixDiskLibHandle vddk_handle;
> +  size_t i;
> +
> + again:
> +  /* See if there's a handle in the pool which is not in use. */
> +  for (i = 0; i < h->vddk_handles.size; ++i) {
> +    if (!h->vddk_handles.ptr[i].in_use) {
> +      h->vddk_handles.ptr[i].in_use = true;
> +      return &h->vddk_handles.ptr[i];
> +    }
> +  }
> +
> +  /* If the pool is too big we have to wait for another thread to
> +   * finish using its handle and try again.
> +   */
> +  if (h->vddk_handles.size >= max) {
> +    pthread_cond_wait (&h->vddk_handles_cond,
&h->vddk_handles_lock);
> +    goto again;
> +  }
> +
> +  /* Open another handle and add it to the pool.  Note that
> +   * vddk_handles_append cannot fail because we reserved space in
> +   * vddk_open.
> +   */
> +  vddk_handle = open_vddk_handle (h);
> +  if (vddk_handle == NULL)
> +    return NULL;
> +  vddk_handles_append (&h->vddk_handles,
> +                       (struct vddk_handle){ vddk_handle, true, h });
> +  return &h->vddk_handles.ptr[h->vddk_handles.size-1];
> +}
> +
> +static void
> +put_vddk_handle (struct vddk_handle **p)
> +{
> +  if (*p) {
> +    ACQUIRE_LOCK_FOR_CURRENT_SCOPE
(&(*p)->h->vddk_handles_lock);
> +    assert ((*p)->in_use);
> +    (*p)->in_use = false;
> +    pthread_cond_signal (&(*p)->h->vddk_handles_cond);
> +  }
> +}
> +
> +/* Wrap get/put_vddk_handle in nicer macro. */
> +#define GET_VDDK_HANDLE_FOR_CURRENT_SCOPE(h, name)      \
> +  __attribute__((cleanup (put_vddk_handle)))            \
> +  struct vddk_handle *name##_h = get_vddk_handle (h);   \
> +  if (name##_h == NULL) return -1;                      \
> +  VixDiskLibHandle name = name##_h->vddk_handle
> +
>  /* Free up the per-connection handle. */
>  static void
>  vddk_close (void *handle)
>  {
>    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&open_close_lock);
> -  struct vddk_handle *h = handle;
> +  struct handle *h = handle;
> +  size_t i;
>
> -  DEBUG_CALL ("VixDiskLib_Close", "handle");
> -  VixDiskLib_Close (h->handle);
> +  for (i = 0; i < h->vddk_handles.size; ++i) {
> +    DEBUG_CALL ("VixDiskLib_Close", "handle");
> +    VixDiskLib_Close (h->vddk_handles.ptr[i].vddk_handle);
> +  }
> +  free (h->vddk_handles.ptr);
>    DEBUG_CALL ("VixDiskLib_Disconnect", "connection");
>    VixDiskLib_Disconnect (h->connection);
>    free_connect_params (h->params);
> +  pthread_cond_destroy (&h->vddk_handles_cond);
> +  pthread_mutex_destroy (&h->vddk_handles_lock);
>    free (h);
>  }
>
> @@ -639,13 +746,14 @@ vddk_close (void *handle)
>  static int64_t
>  vddk_get_size (void *handle)
>  {
> -  struct vddk_handle *h = handle;
> +  struct handle *h = handle;
> +  GET_VDDK_HANDLE_FOR_CURRENT_SCOPE (h, vddk_handle);
>    VixDiskLibInfo *info;
>    VixError err;
>    uint64_t size;
>
>    DEBUG_CALL ("VixDiskLib_GetInfo", "handle,
&info");
> -  err = VixDiskLib_GetInfo (h->handle, &info);
> +  err = VixDiskLib_GetInfo (vddk_handle, &info);
>    if (err != VIX_OK) {
>      VDDK_ERROR (err, "VixDiskLib_GetInfo");
>      return -1;
> @@ -687,7 +795,8 @@ static int
>  vddk_pread (void *handle, void *buf, uint32_t count, uint64_t offset,
>              uint32_t flags)
>  {
> -  struct vddk_handle *h = handle;
> +  struct handle *h = handle;
> +  GET_VDDK_HANDLE_FOR_CURRENT_SCOPE (h, vddk_handle);
>    VixError err;
>
>    /* Align to sectors. */
> @@ -706,7 +815,7 @@ vddk_pread (void *handle, void *buf, uint32_t count,
> uint64_t offset,
>                         "handle, %" PRIu64 " sectors, "
>                         "%" PRIu32 " sectors, buffer",
>                         offset, count);
> -  err = VixDiskLib_Read (h->handle, offset, count, buf);
> +  err = VixDiskLib_Read (vddk_handle, offset, count, buf);
>    if (err != VIX_OK) {
>      VDDK_ERROR (err, "VixDiskLib_Read");
>      return -1;
> @@ -726,7 +835,8 @@ vddk_pwrite (void *handle, const void *buf, uint32_t
> count, uint64_t offset,
>               uint32_t flags)
>  {
>    const bool fua = flags & NBDKIT_FLAG_FUA;
> -  struct vddk_handle *h = handle;
> +  struct handle *h = handle;
> +  GET_VDDK_HANDLE_FOR_CURRENT_SCOPE (h, vddk_handle);
>    VixError err;
>
>    /* Align to sectors. */
> @@ -745,7 +855,7 @@ vddk_pwrite (void *handle, const void *buf, uint32_t
> count, uint64_t offset,
>                         "handle, %" PRIu64 " sectors, "
>                         "%" PRIu32 " sectors, buffer",
>                         offset, count);
> -  err = VixDiskLib_Write (h->handle, offset, count, buf);
> +  err = VixDiskLib_Write (vddk_handle, offset, count, buf);
>    if (err != VIX_OK) {
>      VDDK_ERROR (err, "VixDiskLib_Write");
>      return -1;
> @@ -761,7 +871,8 @@ vddk_pwrite (void *handle, const void *buf, uint32_t
> count, uint64_t offset,
>  static int
>  vddk_flush (void *handle, uint32_t flags)
>  {
> -  struct vddk_handle *h = handle;
> +  struct handle *h = handle;
> +  GET_VDDK_HANDLE_FOR_CURRENT_SCOPE (h, vddk_handle);
>    VixError err;
>
>    /* The Flush call was not available in VDDK < 6.0 so this is simply
> @@ -771,7 +882,7 @@ vddk_flush (void *handle, uint32_t flags)
>      return 0;
>
>    DEBUG_CALL ("VixDiskLib_Flush", "handle");
> -  err = VixDiskLib_Flush (h->handle);
> +  err = VixDiskLib_Flush (vddk_handle);
>    if (err != VIX_OK) {
>      VDDK_ERROR (err, "VixDiskLib_Flush");
>      return -1;
> @@ -783,7 +894,8 @@ vddk_flush (void *handle, uint32_t flags)
>  static int
>  vddk_can_extents (void *handle)
>  {
> -  struct vddk_handle *h = handle;
> +  struct handle *h = handle;
> +  GET_VDDK_HANDLE_FOR_CURRENT_SCOPE (h, vddk_handle);
>    VixError err;
>    VixDiskLibBlockList *block_list;
>
> @@ -808,7 +920,7 @@ vddk_can_extents (void *handle)
>    DEBUG_CALL ("VixDiskLib_QueryAllocatedBlocks",
>                "handle, 0, %d sectors, %d sectors",
>                VIXDISKLIB_MIN_CHUNK_SIZE, VIXDISKLIB_MIN_CHUNK_SIZE);
> -  err = VixDiskLib_QueryAllocatedBlocks (h->handle,
> +  err = VixDiskLib_QueryAllocatedBlocks (vddk_handle,
>                                           0, VIXDISKLIB_MIN_CHUNK_SIZE,
>                                           VIXDISKLIB_MIN_CHUNK_SIZE,
>                                           &block_list);
> @@ -864,7 +976,8 @@ static int
>  vddk_extents (void *handle, uint32_t count, uint64_t offset, uint32_t
> flags,
>                struct nbdkit_extents *extents)
>  {
> -  struct vddk_handle *h = handle;
> +  struct handle *h = handle;
> +  GET_VDDK_HANDLE_FOR_CURRENT_SCOPE (h, vddk_handle);
>    bool req_one = flags & NBDKIT_FLAG_REQ_ONE;
>    uint64_t position, end, start_sector;
>
> @@ -896,7 +1009,7 @@ vddk_extents (void *handle, uint32_t count, uint64_t
> offset, uint32_t flags,
>                  "handle, %" PRIu64 " sectors, %"
PRIu64 " sectors, "
>                  "%d sectors",
>                  start_sector, nr_sectors, VIXDISKLIB_MIN_CHUNK_SIZE);
> -    err = VixDiskLib_QueryAllocatedBlocks (h->handle,
> +    err = VixDiskLib_QueryAllocatedBlocks (vddk_handle,
>                                             start_sector, nr_sectors,
>                                             VIXDISKLIB_MIN_CHUNK_SIZE,
>                                             &block_list);
> --
> 2.27.0
>
>