[Libguestfs] [PATCH nbdkit 2/2] vddk: Relax thread model to PARALLEL and implement a disk handle pool.
Nir Soffer
nsoffer at redhat.com
Thu Aug 6 15:46:06 UTC 2020
On Thu, Aug 6, 2020, 16:16 Richard W.M. Jones <rjones at 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20200806/a0a574e8/attachment.htm>
More information about the Libguestfs
mailing list