Richard W.M. Jones
2020-Aug-06 13:16 UTC
[Libguestfs] [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.
Richard W.M. Jones
2020-Aug-06 13:16 UTC
[Libguestfs] [PATCH nbdkit 1/2] vddk: Relax threading model: SERIALIZE_ALL_REQUESTS -> SERIALIZE_REQUESTS.
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
Richard W.M. Jones
2020-Aug-06 13:16 UTC
[Libguestfs] [PATCH nbdkit 2/2] vddk: Relax thread model to PARALLEL and implement a disk handle pool.
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
Richard W.M. Jones
2020-Aug-06 14:35 UTC
Re: [Libguestfs] [PATCH nbdkit NOT WORKING 0/2] vddk: Relax threading model.
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
Nir Soffer
2020-Aug-06 15:31 UTC
Re: [Libguestfs] [PATCH nbdkit 1/2] vddk: Relax threading model: SERIALIZE_ALL_REQUESTS -> SERIALIZE_REQUESTS.
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 > >
Nir Soffer
2020-Aug-06 15:46 UTC
Re: [Libguestfs] [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 > >
Maybe Matching Threads
- Re: [PATCH nbdkit 1/2] vddk: Relax threading model: SERIALIZE_ALL_REQUESTS -> SERIALIZE_REQUESTS.
- [PATCH nbdkit] vddk: Relax threading model and enable multi-conn.
- [PATCH nbdkit 2/2] vddk: Relax thread model to PARALLEL and implement a disk handle pool.
- Re: [PATCH nbdkit 2/2] vddk: Relax thread model to PARALLEL and implement a disk handle pool.
- [PATCH nbdkit NOT WORKING 0/2] vddk: Relax threading model.