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

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
>
>

On Thu, Aug 06, 2020 at 06:46:06PM +0300, Nir Soffer
wrote:
> 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?
It's a good point, and would also explain why multi-conn was faster
than this.  I'll give that a go later.

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 06, 2020 at 06:46:06PM +0300, Nir Soffer
wrote:
> 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.
Nice theory, but it didn't work.  Again more handles = slower,
broadly speaking:

16 coroutines, 16 handles:

real	1m36.920s
user	0m0.112s
sys	0m1.641s

8 / 8:

real	1m27.946s
user	0m0.110s
sys	0m1.652s

4 / 4:

real	1m23.027s
user	0m0.100s
sys	0m1.600s

2 / 2:

real	1m6.089s
user	0m0.102s
sys	0m1.658s

1 / 1:

real	1m9.540s
user	0m0.083s
sys	0m1.669s

The patch I'm using now is attached.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Thu, Aug 6, 2020 at 7:24 PM Richard W.M. Jones <rjones@redhat.com>
wrote:
>
> On Thu, Aug 06, 2020 at 06:46:06PM +0300, Nir Soffer wrote:
> > 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.
>
> Nice theory, but it didn't work.  Again more handles = slower,
> broadly speaking:
>
> 16 coroutines, 16 handles:
>
> real    1m36.920s
> user    0m0.112s
> sys     0m1.641s
>
> 8 / 8:
>
> real    1m27.946s
> user    0m0.110s
> sys     0m1.652s
>
> 4 / 4:
>
> real    1m23.027s
> user    0m0.100s
> sys     0m1.600s
>
> 2 / 2:
>
> real    1m6.089s
> user    0m0.102s
> sys     0m1.658s
>
> 1 / 1:
>
> real    1m9.540s
> user    0m0.083s
> sys     0m1.669s
>
> The patch I'm using now is attached.
There are 2 issues:

1. We create the connection and handle with uninitialized memory, and later
   with memory used by the previous connection/handle. This may not matter if
   the library initializes all fields of the struct, but it is safer
to zero the memory.

2. We create the handlers in request threads, so most likely not in
the same thread.
   This may be ok, but risky.

I moved the creation of all connections and handles to populate_pool(), called
from vddk_open(). This may create more connections and handles than needed
if the client does not use multiple inflight requests, but this is not
important now.

The code compiles but I cannot test it. Does it make any difference?

Nir

>
> Rich.
>
> --
> Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-df lists disk usage of guests without needing to install any
> software inside the virtual machine.  Supports Linux and Windows.
> http://people.redhat.com/~rjones/virt-df/