[PATCH nbdkit NOT WORKING 0/2] vddk: Relax threading model.
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-... + * + * 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
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-... + * + * 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 06, 2020 at 06:31:39PM +0300, Nir Soffer wrote:
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-... + * + * 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 agree it's not within the letter of the rules, as it says in the comment. However it does appear to work. Rich.
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
-- 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/
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-... * * 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
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-... * * 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/
On Sat, Aug 8, 2020, 00:01 Nir Soffer <nsoffer@redhat.com> wrote:
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?
Richard, did you test this?
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/
On Sat, Aug 08, 2020 at 12:01:23AM +0300, Nir Soffer wrote:
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?
I've had a look at this code now, but unfortunately we cannot do it this way. Opening a VDDK handle is *slow* - by which I mean it takes several seconds (3.1 seconds on my two back-to-back machines over gigabit ethernet). Opening 8 of them for any connection is not going to fly, or even a smaller pool. (There were other problems which are easily fixable, such as in_use always being true, but the above problem is the main one.) 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
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
participants (2)
-
Nir Soffer -
Richard W.M. Jones