<div dir="auto"><div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Aug 6, 2020, 16:16 Richard W.M. Jones <<a href="mailto:rjones@redhat.com">rjones@redhat.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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) * <a href="https://code.vmware.com/docs/11750/virtual-disk-development-kit-programming-guide/GUID-6BE903E8-DC70-46D9-98E4-E34A2002C2AD.html" rel="noreferrer noreferrer" target="_blank">https://code.vmware.com/docs/11750/virtual-disk-development-kit-programming-guide/GUID-6BE903E8-DC70-46D9-98E4-E34A2002C2AD.html</a> * * 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 */ </blockquote></div></div><div dir="auto"> </div><div dir="auto">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.</div><div dir="auto"> </div><div dir="auto">With multiple nbd connections we got 60% improvement. I expect to see similar results if we use multiple connection+handle pairs.</div><div dir="auto"> </div><div dir="auto">Can you try to move the connection into the vddk_handle struct?</div><div dir="auto"> </div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> - 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 </blockquote></div></div></div>