[Libguestfs] [PATCH nbdkit 2/2] vddk: Relax thread model to PARALLEL and implement a disk handle pool.
Richard W.M. Jones
rjones at redhat.com
Thu Aug 6 16:24:18 UTC 2020
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/
-------------- next part --------------
>From c5db5189442a6398e5561a27ad9e72a76fd42851 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones at redhat.com>
Date: Thu, 6 Aug 2020 12:50:59 +0100
Subject: [PATCH] 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 | 223 +++++++++++++++++++++-------
2 files changed, 174 insertions(+), 56 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..95f24b83 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,38 @@ 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_ConnectEx, 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;
+struct handle;
+struct entry {
+ VixDiskLibConnection conn;
+ VixDiskLibHandle disk;
+ bool in_use; /* true if in use by any thread */
+ struct handle *h; /* points back to owning nbdkit handle */
+};
+DEFINE_VECTOR_TYPE(pool, struct entry)
+
/* The per-connection handle. */
-struct vddk_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 connections and disk handles. Do not access this
+ * directly, use GET_DISK_FOR_CURRENT_SCOPE macro to get an unused
+ * handle.
+ */
+ pthread_mutex_t pool_lock;
+ pthread_cond_t pool_cond;
+ pool pool;
};
static inline VixDiskLibConnectParams *
@@ -531,17 +557,27 @@ 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;
+ struct handle *h;
- 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->pool_lock, NULL);
+ pthread_cond_init (&h->pool_cond, NULL);
+
+ /* We have to reserve this vector to ensure that it is not
+ * reallocated, as that would make the pointer returned by get_entry
+ * in another thread invalid.
+ */
+ if (pool_reserve (&h->pool, pool_max) == -1) {
+ nbdkit_error ("realloc: %m");
+ goto err0;
+ }
+
h->params = allocate_connect_params ();
if (h->params == NULL) {
nbdkit_error ("allocate VixDiskLibConnectParams: %m");
@@ -568,6 +604,32 @@ vddk_open (int readonly)
h->params->specType = VIXDISKLIB_SPEC_VMX;
}
+ h->flags = 0;
+ if (readonly)
+ h->flags |= VIXDISKLIB_FLAG_OPEN_READ_ONLY;
+ if (single_link)
+ h->flags |= VIXDISKLIB_FLAG_OPEN_SINGLE_LINK;
+ if (unbuffered)
+ h->flags |= VIXDISKLIB_FLAG_OPEN_UNBUFFERED;
+
+ return h;
+
+ err0:
+ free (h->pool.ptr);
+ pthread_cond_destroy (&h->pool_cond);
+ pthread_mutex_destroy (&h->pool_lock);
+ free (h);
+ return NULL;
+}
+
+/* Get a VDDK connection and disk handle on demand. */
+static int
+open_disk (struct handle *h,
+ VixDiskLibConnection *conn_rtn, VixDiskLibHandle *disk_rtn)
+{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&open_close_lock);
+ VixError err;
+
/* XXX Some documentation suggests we should call
* VixDiskLib_PrepareForAccess here. It may be required for
* Advanced Transport modes, but I could not make it work with
@@ -576,62 +638,105 @@ vddk_open (int readonly)
DEBUG_CALL ("VixDiskLib_ConnectEx",
"h->params, %d, %s, %s, &connection",
- readonly,
+ h->readonly,
snapshot_moref ? : "NULL",
transport_modes ? : "NULL");
err = VixDiskLib_ConnectEx (h->params,
- readonly,
+ h->readonly,
snapshot_moref,
transport_modes,
- &h->connection);
+ conn_rtn);
if (err != VIX_OK) {
VDDK_ERROR (err, "VixDiskLib_ConnectEx");
- goto err1;
+ *conn_rtn = NULL;
+ return -1;
}
- flags = 0;
- if (readonly)
- flags |= VIXDISKLIB_FLAG_OPEN_READ_ONLY;
- if (single_link)
- 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);
+ "connection, %s, %d, &handle", filename, h->flags);
+ err = VixDiskLib_Open (*conn_rtn, filename, h->flags, disk_rtn);
if (err != VIX_OK) {
VDDK_ERROR (err, "VixDiskLib_Open: %s", filename);
- goto err2;
+ *disk_rtn = NULL;
+ return -1;
}
- nbdkit_debug ("transport mode: %s",
- VixDiskLib_GetTransportMode (h->handle));
+ nbdkit_debug ("transport mode: %s", VixDiskLib_GetTransportMode (*disk_rtn));
+ return 0;
+}
+
+static struct entry *
+get_disk (struct handle *h)
+{
+ const unsigned max = h->readonly ? pool_max : 1;
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&h->pool_lock);
+ VixDiskLibConnection conn;
+ VixDiskLibHandle disk;
+ size_t i;
- return h;
+ again:
+ /* See if there's a handle in the pool which is not in use. */
+ for (i = 0; i < h->pool.size; ++i) {
+ if (!h->pool.ptr[i].in_use) {
+ h->pool.ptr[i].in_use = true;
+ return &h->pool.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->pool.size >= max) {
+ pthread_cond_wait (&h->pool_cond, &h->pool_lock);
+ goto again;
+ }
- err2:
- DEBUG_CALL ("VixDiskLib_Disconnect", "connection");
- VixDiskLib_Disconnect (h->connection);
- err1:
- free_connect_params (h->params);
- err0:
- free (h);
- return NULL;
+ /* Open another handle and add it to the pool. Note that
+ * pool_append cannot fail because we reserved space in
+ * vddk_open.
+ */
+ if (open_disk (h, &conn, &disk) == -1)
+ return NULL;
+ pool_append (&h->pool, (struct entry){ conn, disk, true, h });
+ return &h->pool.ptr[h->pool.size-1];
}
+static void
+put_disk (struct entry **p)
+{
+ if (*p) {
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&(*p)->h->pool_lock);
+ assert ((*p)->in_use);
+ (*p)->in_use = false;
+ pthread_cond_signal (&(*p)->h->pool_cond);
+ }
+}
+
+/* Wrap get/put_disk in nicer macro. */
+#define GET_DISK_FOR_CURRENT_SCOPE(h, name) \
+ __attribute__((cleanup (put_disk))) \
+ struct entry *name##_h = get_disk (h); \
+ if (name##_h == NULL) return -1; \
+ VixDiskLibHandle name = name##_h->disk
+
/* 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);
- DEBUG_CALL ("VixDiskLib_Disconnect", "connection");
- VixDiskLib_Disconnect (h->connection);
+ for (i = 0; i < h->pool.size; ++i) {
+ DEBUG_CALL ("VixDiskLib_Close", "handle");
+ VixDiskLib_Close (h->pool.ptr[i].disk);
+ DEBUG_CALL ("VixDiskLib_Disconnect", "connection");
+ VixDiskLib_Disconnect (h->pool.ptr[i].conn);
+ }
+ free (h->pool.ptr);
free_connect_params (h->params);
+ pthread_cond_destroy (&h->pool_cond);
+ pthread_mutex_destroy (&h->pool_lock);
free (h);
}
@@ -639,13 +744,14 @@ vddk_close (void *handle)
static int64_t
vddk_get_size (void *handle)
{
- struct vddk_handle *h = handle;
+ struct handle *h = handle;
+ GET_DISK_FOR_CURRENT_SCOPE (h, disk);
VixDiskLibInfo *info;
VixError err;
uint64_t size;
DEBUG_CALL ("VixDiskLib_GetInfo", "handle, &info");
- err = VixDiskLib_GetInfo (h->handle, &info);
+ err = VixDiskLib_GetInfo (disk, &info);
if (err != VIX_OK) {
VDDK_ERROR (err, "VixDiskLib_GetInfo");
return -1;
@@ -687,7 +793,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_DISK_FOR_CURRENT_SCOPE (h, disk);
VixError err;
/* Align to sectors. */
@@ -706,7 +813,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 (disk, offset, count, buf);
if (err != VIX_OK) {
VDDK_ERROR (err, "VixDiskLib_Read");
return -1;
@@ -726,7 +833,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_DISK_FOR_CURRENT_SCOPE (h, disk);
VixError err;
/* Align to sectors. */
@@ -745,7 +853,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 (disk, offset, count, buf);
if (err != VIX_OK) {
VDDK_ERROR (err, "VixDiskLib_Write");
return -1;
@@ -761,7 +869,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_DISK_FOR_CURRENT_SCOPE (h, disk);
VixError err;
/* The Flush call was not available in VDDK < 6.0 so this is simply
@@ -771,7 +880,7 @@ vddk_flush (void *handle, uint32_t flags)
return 0;
DEBUG_CALL ("VixDiskLib_Flush", "handle");
- err = VixDiskLib_Flush (h->handle);
+ err = VixDiskLib_Flush (disk);
if (err != VIX_OK) {
VDDK_ERROR (err, "VixDiskLib_Flush");
return -1;
@@ -783,7 +892,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_DISK_FOR_CURRENT_SCOPE (h, disk);
VixError err;
VixDiskLibBlockList *block_list;
@@ -808,7 +918,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 (disk,
0, VIXDISKLIB_MIN_CHUNK_SIZE,
VIXDISKLIB_MIN_CHUNK_SIZE,
&block_list);
@@ -864,7 +974,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_DISK_FOR_CURRENT_SCOPE (h, disk);
bool req_one = flags & NBDKIT_FLAG_REQ_ONE;
uint64_t position, end, start_sector;
@@ -896,7 +1007,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 (disk,
start_sector, nr_sectors,
VIXDISKLIB_MIN_CHUNK_SIZE,
&block_list);
--
2.27.0
More information about the Libguestfs
mailing list