Richard W.M. Jones
2021-Dec-16 10:31 UTC
[Libguestfs] [PATCH nbdkit BROKEN 0/2] vddk: Support multi-conn, but only open one VDDK connection
This is broken as noted in the first commit message. I'm only posting it to document and archive the approach. We would like to enable multi-conn in the VDDK plugin for its potential performance benefits, and also to avoid the hack that we had to add to virt-v2v: https://github.com/libguestfs/virt-v2v/commit/bb0e698360470cb4ff5992e8e01a3165f56fe41e One way I thought of doing this would be to advertise multi-conn, but internally in the plugin only ever open a single VDDK connection. Multiple calls to .open would open the VDDK connection on the first call and increment a counter in other cases. Multiple calls to .close would close the connection only on the last close. However it doesn't actually work. For some reason our old enemy QueryAllocatedBlocks (QAB) suddenly becomes many times (about 18-20 times) slower when you do this. Of course it's closed source so we can never work out why, but my speculation is that QAB has a mutex that prevents it from overlapping with any other call, and that it only works well when called linearly across the disk. When nbdcopy sees multi-conn it will launch 4 threads reading in parallel across different offsets which would break both of these assumptions. I observed similar huge slowdowns (7x) in QAB when opening multiple read-only connections to the same disk using the current code. Anyway, for whatever reason it doesn't work, so this is just for archiving. Rich.
Richard W.M. Jones
2021-Dec-16 10:31 UTC
[Libguestfs] [PATCH nbdkit BROKEN 1/2] BROKEN vddk: Support multi-conn, but only open one VDDK connection
***
BROKEN: Increases the time taken to do an nbdcopy by about 66%. All
of the extra time is taken in QueryAllocatedBlocks even though we make
the same number of calls before and after. It seems as if
QueryAllocatedBlocks really cannot be overlapped with other operations
even on the same connection.
***
On the NBD side, support multi-conn. However actually opening
multiple VDDK connections causes severe slowdowns:
https://github.com/libguestfs/virt-v2v/commit/bb0e698360470cb4ff5992e8e01a3165f56fe41e
So instead only ever hold a single VDDK handle open. It is opened
when the first NBD connection is made and closed when the last NBD
connection is closed. There is only a single VDDK worker thread for
the same reason.
The change is mostly mechanical - we get rid of the per-connection
handle, store everything as globals, and make sure we only connect
once to VDDK on first connection and disconnect once on final
disconnection.
We can then advertise multi-conn.
---
plugins/vddk/vddk.h | 30 +++----
plugins/vddk/vddk.c | 196 ++++++++++++++++++++++++------------------
plugins/vddk/worker.c | 69 ++++++++-------
3 files changed, 157 insertions(+), 138 deletions(-)
diff --git a/plugins/vddk/vddk.h b/plugins/vddk/vddk.h
index d99b6f4b..162e05c6 100644
--- a/plugins/vddk/vddk.h
+++ b/plugins/vddk/vddk.h
@@ -75,6 +75,10 @@ extern int vddk_debug_extents;
extern int vddk_debug_datapath;
extern int vddk_debug_stats;
+extern VixDiskLibHandle handle;
+
+extern pthread_t worker_thread;
+
#define STUB(fn,ret,args) extern ret (*fn) args
#define OPTIONAL_STUB(fn,ret,args) STUB(fn,ret,args)
#include "vddk-stubs.h"
@@ -143,24 +147,10 @@ struct command {
DEFINE_VECTOR_TYPE(command_queue, struct command *)
-/* The per-connection handle. */
-struct vddk_handle {
- VixDiskLibConnectParams *params; /* connection parameters */
- VixDiskLibConnection connection; /* connection */
- VixDiskLibHandle handle; /* disk handle */
-
- pthread_t thread; /* background thread for asynch work */
-
- /* Command queue of commands sent to the background thread. Use
- * send_command_and_wait to add a command. Only the background
- * thread must make VDDK API calls (apart from opening and closing).
- * The lock protects all of these fields.
- */
- pthread_mutex_t commands_lock; /* lock */
- command_queue commands; /* command queue */
- pthread_cond_t commands_cond; /* condition (queue size 0 -> 1) */
- uint64_t id; /* next command ID */
-};
+extern pthread_mutex_t commands_lock;
+extern command_queue commands;
+extern pthread_cond_t commands_cond;
+extern uint64_t next_command_id;
/* reexec.c */
extern bool noreexec;
@@ -188,7 +178,7 @@ extern void trim (char *str);
/* worker.c */
extern const char *command_type_string (enum command_type type);
-extern int send_command_and_wait (struct vddk_handle *h, struct command *cmd);
-extern void *vddk_worker_thread (void *handle);
+extern int send_command_and_wait (struct command *cmd);
+extern void *vddk_worker_thread (void *);
#endif /* NBDKIT_VDDK_H */
diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index 31e5e23b..09a84908 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -95,6 +95,21 @@ bool unbuffered; /* unbuffered */
const char *username; /* user */
const char *vmx_spec; /* vm */
+VixDiskLibHandle handle; /* disk handle */
+
+pthread_t worker_thread; /* background thread for asynch work */
+
+/* Command queue of commands sent to the background thread. Use
+ * send_command_and_wait to add a command. Only the background
+ * thread must make VDDK API calls (apart from opening and closing).
+ * The lock protects all of these fields.
+ */
+pthread_mutex_t commands_lock = PTHREAD_MUTEX_INITIALIZER;
+command_queue commands = empty_vector; /* command queue */
+pthread_cond_t commands_cond = PTHREAD_COND_INITIALIZER;
+ /* condition (queue size 0 -> 1) */
+uint64_t next_command_id; /* next command ID */
+
/* Unload the plugin. */
static void
vddk_unload (void)
@@ -511,15 +526,29 @@ vddk_dump_plugin (void)
* Before nbdkit 1.22 we used SERIALIZE_ALL_REQUESTS. In nbdkit
* 1.22-1.28 we changed this to SERIALIZE_REQUESTS and added a mutex
* around calls to VixDiskLib_Open and VixDiskLib_Close. In nbdkit
- * 1.30 and above we assign a background thread per connection to do
- * asynch operations and use the PARALLEL model. We still need the
- * lock around Open and Close.
+ * 1.30 and above we assign a background thread to asynch operations
+ * and use the PARALLEL model. We only connect to VDDK at most once.
+ * We still need the lock around Open and Close.
*/
#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
/* Lock protecting open/close calls - see above. */
static pthread_mutex_t open_close_lock = PTHREAD_MUTEX_INITIALIZER;
+static int nr_opened; /* number of handles opened */
+
+static VixDiskLibConnectParams *params; /* connection parameters */
+static VixDiskLibConnection connection; /* connection */
+
+static int
+vddk_can_multi_conn (void *h)
+{
+ /* We support multiple NBD connections, but we will only open one
+ * VDDK connection.
+ */
+ return 1;
+}
+
static inline VixDiskLibConnectParams *
allocate_connect_params (void)
{
@@ -537,64 +566,53 @@ allocate_connect_params (void)
}
static inline void
-free_connect_params (VixDiskLibConnectParams *params)
+free_connect_params (VixDiskLibConnectParams *p)
{
/* Only use FreeConnectParams if AllocateConnectParams was
* originally called. Otherwise use free.
*/
if (VixDiskLib_AllocateConnectParams != NULL) {
VDDK_CALL_START (VixDiskLib_FreeConnectParams, "params")
- VixDiskLib_FreeConnectParams (params);
+ VixDiskLib_FreeConnectParams (p);
VDDK_CALL_END (VixDiskLib_FreeConnectParams, 0);
}
else
- free (params);
+ free (p);
}
-/* Create the per-connection handle. */
-static void *
-vddk_open (int readonly)
+/* Really open the connection. */
+static int
+do_open (int readonly)
{
- ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&open_close_lock);
- struct vddk_handle *h;
VixError err;
uint32_t flags;
const char *transport_mode;
int pterr;
- h = calloc (1, sizeof *h);
- if (h == NULL) {
- nbdkit_error ("calloc: %m");
- return NULL;
- }
- h->commands = (command_queue) empty_vector;
- pthread_mutex_init (&h->commands_lock, NULL);
- pthread_cond_init (&h->commands_cond, NULL);
-
- h->params = allocate_connect_params ();
- if (h->params == NULL) {
+ params = allocate_connect_params ();
+ if (params == NULL) {
nbdkit_error ("allocate VixDiskLibConnectParams: %m");
goto err0;
}
if (is_remote) {
- h->params->vmxSpec = (char *) vmx_spec;
- h->params->serverName = (char *) server_name;
+ params->vmxSpec = (char *) vmx_spec;
+ params->serverName = (char *) server_name;
if (cookie == NULL) {
- h->params->credType = VIXDISKLIB_CRED_UID;
- h->params->creds.uid.userName = (char *) username;
- h->params->creds.uid.password = password;
+ params->credType = VIXDISKLIB_CRED_UID;
+ params->creds.uid.userName = (char *) username;
+ params->creds.uid.password = password;
}
else {
- h->params->credType = VIXDISKLIB_CRED_SESSIONID;
- h->params->creds.sessionId.cookie = (char *) cookie;
- h->params->creds.sessionId.userName = (char *) username;
- h->params->creds.sessionId.key = password;
+ params->credType = VIXDISKLIB_CRED_SESSIONID;
+ params->creds.sessionId.cookie = (char *) cookie;
+ params->creds.sessionId.userName = (char *) username;
+ params->creds.sessionId.key = password;
}
- h->params->thumbPrint = (char *) thumb_print;
- h->params->port = port;
- h->params->nfcHostPort = nfc_host_port;
- h->params->specType = VIXDISKLIB_SPEC_VMX;
+ params->thumbPrint = (char *) thumb_print;
+ params->port = port;
+ params->nfcHostPort = nfc_host_port;
+ params->specType = VIXDISKLIB_SPEC_VMX;
}
/* XXX We should call VixDiskLib_PrepareForAccess here. It disables
@@ -603,15 +621,15 @@ vddk_open (int readonly)
*/
VDDK_CALL_START (VixDiskLib_ConnectEx,
- "h->params, %d, %s, %s, &connection",
+ "params, %d, %s, %s, &connection",
readonly,
snapshot_moref ? : "NULL",
transport_modes ? : "NULL")
- err = VixDiskLib_ConnectEx (h->params,
+ err = VixDiskLib_ConnectEx (params,
readonly,
snapshot_moref,
transport_modes,
- &h->connection);
+ &connection);
VDDK_CALL_END (VixDiskLib_ConnectEx, 0);
if (err != VIX_OK) {
VDDK_ERROR (err, "VixDiskLib_ConnectEx");
@@ -634,7 +652,7 @@ vddk_open (int readonly)
VDDK_CALL_START (VixDiskLib_Open,
"connection, %s, %d, &handle", filename,
flags)
- err = VixDiskLib_Open (h->connection, filename, flags,
&h->handle);
+ err = VixDiskLib_Open (connection, filename, flags, &handle);
VDDK_CALL_END (VixDiskLib_Open, 0);
if (err != VIX_OK) {
VDDK_ERROR (err, "VixDiskLib_Open: %s", filename);
@@ -642,73 +660,89 @@ vddk_open (int readonly)
}
VDDK_CALL_START (VixDiskLib_GetTransportMode, "handle")
- transport_mode = VixDiskLib_GetTransportMode (h->handle);
+ transport_mode = VixDiskLib_GetTransportMode (handle);
VDDK_CALL_END (VixDiskLib_GetTransportMode, 0);
nbdkit_debug ("transport mode: %s", transport_mode);
/* Start the background thread which actually does the asynchronous
* work.
*/
- pterr = pthread_create (&h->thread, NULL, vddk_worker_thread, h);
+ pterr = pthread_create (&worker_thread, NULL, vddk_worker_thread, NULL);
if (pterr != 0) {
errno = pterr;
nbdkit_error ("pthread_create: %m");
goto err3;
}
- return h;
+ return 0;
err3:
VDDK_CALL_START (VixDiskLib_Close, "handle")
- VixDiskLib_Close (h->handle);
+ VixDiskLib_Close (handle);
VDDK_CALL_END (VixDiskLib_Close, 0);
err2:
VDDK_CALL_START (VixDiskLib_Disconnect, "connection")
- VixDiskLib_Disconnect (h->connection);
+ VixDiskLib_Disconnect (connection);
VDDK_CALL_END (VixDiskLib_Disconnect, 0);
err1:
- free_connect_params (h->params);
+ free_connect_params (params);
err0:
- pthread_mutex_destroy (&h->commands_lock);
- pthread_cond_destroy (&h->commands_cond);
- free (h);
- return NULL;
+ return -1;
}
-/* Free up the per-connection handle. */
+/* Really close the connection. */
static void
-vddk_close (void *handle)
+do_close (void)
{
- ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&open_close_lock);
- struct vddk_handle *h = handle;
struct command stop_cmd = { .type = STOP };
- send_command_and_wait (h, &stop_cmd);
- pthread_join (h->thread, NULL);
+ send_command_and_wait (&stop_cmd);
+ pthread_join (worker_thread, NULL);
VDDK_CALL_START (VixDiskLib_Close, "handle")
- VixDiskLib_Close (h->handle);
+ VixDiskLib_Close (handle);
VDDK_CALL_END (VixDiskLib_Close, 0);
VDDK_CALL_START (VixDiskLib_Disconnect, "connection")
- VixDiskLib_Disconnect (h->connection);
+ VixDiskLib_Disconnect (connection);
VDDK_CALL_END (VixDiskLib_Disconnect, 0);
- free_connect_params (h->params);
- pthread_mutex_destroy (&h->commands_lock);
- pthread_cond_destroy (&h->commands_cond);
- command_queue_reset (&h->commands);
- free (h);
+ free_connect_params (params);
+ command_queue_reset (&commands);
+}
+
+static void *
+vddk_open (int readonly)
+{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&open_close_lock);
+
+ if (nr_opened == 0) {
+ if (do_open (readonly) == -1)
+ return NULL;
+ }
+
+ nr_opened++;
+ return NBDKIT_HANDLE_NOT_NEEDED;
+}
+
+static void
+vddk_close (void *h)
+{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&open_close_lock);
+
+ if (nr_opened == 1)
+ do_close ();
+
+ nr_opened--;
}
/* Get the file size. */
static int64_t
-vddk_get_size (void *handle)
+vddk_get_size (void *h)
{
- struct vddk_handle *h = handle;
uint64_t size;
struct command get_size_cmd = { .type = GET_SIZE, .ptr = &size };
- if (send_command_and_wait (h, &get_size_cmd) == -1)
+ if (send_command_and_wait (&get_size_cmd) == -1)
return -1;
return (int64_t) size;
@@ -718,13 +752,13 @@ vddk_get_size (void *handle)
* we are always able to do FUA / flush.
*/
static int
-vddk_can_fua (void *handle)
+vddk_can_fua (void *h)
{
return NBDKIT_FUA_NATIVE;
}
static int
-vddk_can_flush (void *handle)
+vddk_can_flush (void *h)
{
return 1;
}
@@ -734,10 +768,9 @@ vddk_can_flush (void *handle)
* Note that reads have to be aligned to sectors (XXX).
*/
static int
-vddk_pread (void *handle, void *buf, uint32_t count, uint64_t offset,
+vddk_pread (void *h, void *buf, uint32_t count, uint64_t offset,
uint32_t flags)
{
- struct vddk_handle *h = handle;
struct command read_cmd = {
.type = READ,
.ptr = buf,
@@ -745,20 +778,19 @@ vddk_pread (void *handle, void *buf, uint32_t count,
uint64_t offset,
.offset = offset,
};
- return send_command_and_wait (h, &read_cmd);
+ return send_command_and_wait (&read_cmd);
}
-static int vddk_flush (void *handle, uint32_t flags);
+static int vddk_flush (void *h, uint32_t flags);
/* Write data to the file.
*
* Note that writes have to be aligned to sectors (XXX).
*/
static int
-vddk_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset,
+vddk_pwrite (void *h, const void *buf, uint32_t count, uint64_t offset,
uint32_t flags)
{
- struct vddk_handle *h = handle;
const bool fua = flags & NBDKIT_FLAG_FUA;
struct command write_cmd = {
.type = WRITE,
@@ -767,7 +799,7 @@ vddk_pwrite (void *handle, const void *buf, uint32_t count,
uint64_t offset,
.offset = offset,
};
- if (send_command_and_wait (h, &write_cmd) == -1)
+ if (send_command_and_wait (&write_cmd) == -1)
return -1;
if (fua) {
@@ -780,32 +812,29 @@ vddk_pwrite (void *handle, const void *buf, uint32_t
count, uint64_t offset,
/* Flush data to the file. */
static int
-vddk_flush (void *handle, uint32_t flags)
+vddk_flush (void *h, uint32_t flags)
{
- struct vddk_handle *h = handle;
struct command flush_cmd = { .type = FLUSH };
- return send_command_and_wait (h, &flush_cmd);
+ return send_command_and_wait (&flush_cmd);
}
static int
-vddk_can_extents (void *handle)
+vddk_can_extents (void *h)
{
- struct vddk_handle *h = handle;
int ret;
struct command can_extents_cmd = { .type = CAN_EXTENTS, .ptr = &ret };
- if (send_command_and_wait (h, &can_extents_cmd) == -1)
+ if (send_command_and_wait (&can_extents_cmd) == -1)
return -1;
return ret;
}
static int
-vddk_extents (void *handle, uint32_t count, uint64_t offset, uint32_t flags,
+vddk_extents (void *h, uint32_t count, uint64_t offset, uint32_t flags,
struct nbdkit_extents *extents)
{
- struct vddk_handle *h = handle;
bool req_one = flags & NBDKIT_FLAG_REQ_ONE;
struct command extents_cmd = {
.type = EXTENTS,
@@ -815,7 +844,7 @@ vddk_extents (void *handle, uint32_t count, uint64_t offset,
uint32_t flags,
.req_one = req_one,
};
- return send_command_and_wait (h, &extents_cmd);
+ return send_command_and_wait (&extents_cmd);
}
static struct nbdkit_plugin plugin = {
@@ -830,6 +859,7 @@ static struct nbdkit_plugin plugin = {
.dump_plugin = vddk_dump_plugin,
.get_ready = vddk_get_ready,
.after_fork = vddk_after_fork,
+ .can_multi_conn = vddk_can_multi_conn,
.open = vddk_open,
.close = vddk_close,
.get_size = vddk_get_size,
diff --git a/plugins/vddk/worker.c b/plugins/vddk/worker.c
index c6e2fd22..4bb7d8d7 100644
--- a/plugins/vddk/worker.c
+++ b/plugins/vddk/worker.c
@@ -70,20 +70,20 @@ command_type_string (enum command_type type)
* On error, calls nbdkit_error and returns -1.
*/
int
-send_command_and_wait (struct vddk_handle *h, struct command *cmd)
+send_command_and_wait (struct command *cmd)
{
/* Add the command to the command queue. */
{
- ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&h->commands_lock);
- cmd->id = h->id++;
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&commands_lock);
+ cmd->id = next_command_id++;
- if (command_queue_append (&h->commands, cmd) == -1)
+ if (command_queue_append (&commands, cmd) == -1)
/* On error command_queue_append will call nbdkit_error. */
return -1;
/* Signal the caller if it could be sleeping on an empty queue. */
- if (h->commands.len == 1)
- pthread_cond_signal (&h->commands_cond);
+ if (commands.len == 1)
+ pthread_cond_signal (&commands_cond);
/* This will be used to signal command completion back to us. */
pthread_mutex_init (&cmd->mutex, NULL);
@@ -132,13 +132,13 @@ complete_command (void *vp, VixError result)
/* Wait for any asynchronous commands to complete. */
static int
-do_stop (struct command *cmd, struct vddk_handle *h)
+do_stop (struct command *cmd)
{
VixError err;
/* Because we assume VDDK >= 6.5, VixDiskLib_Wait must exist. */
VDDK_CALL_START (VixDiskLib_Wait, "handle")
- err = VixDiskLib_Wait (h->handle);
+ err = VixDiskLib_Wait (handle);
VDDK_CALL_END (VixDiskLib_Wait, 0);
if (err != VIX_OK) {
VDDK_ERROR (err, "VixDiskLib_Wait");
@@ -152,14 +152,14 @@ do_stop (struct command *cmd, struct vddk_handle *h)
/* Get size command. */
static int64_t
-do_get_size (struct command *cmd, struct vddk_handle *h)
+do_get_size (struct command *cmd)
{
VixError err;
VixDiskLibInfo *info;
uint64_t size;
VDDK_CALL_START (VixDiskLib_GetInfo, "handle, &info")
- err = VixDiskLib_GetInfo (h->handle, &info);
+ err = VixDiskLib_GetInfo (handle, &info);
VDDK_CALL_END (VixDiskLib_GetInfo, 0);
if (err != VIX_OK) {
VDDK_ERROR (err, "VixDiskLib_GetInfo");
@@ -203,7 +203,7 @@ do_get_size (struct command *cmd, struct vddk_handle *h)
}
static int
-do_read (struct command *cmd, struct vddk_handle *h)
+do_read (struct command *cmd)
{
VixError err;
uint32_t count = cmd->count;
@@ -226,7 +226,7 @@ do_read (struct command *cmd, struct vddk_handle *h)
"handle, %" PRIu64 " sectors, "
"%" PRIu32 " sectors, buffer, callback,
%" PRIu64,
offset, count, cmd->id)
- err = VixDiskLib_ReadAsync (h->handle, offset, count, buf,
+ err = VixDiskLib_ReadAsync (handle, offset, count, buf,
complete_command, cmd);
VDDK_CALL_END (VixDiskLib_ReadAsync, count * VIXDISKLIB_SECTOR_SIZE);
if (err != VIX_ASYNC) {
@@ -238,7 +238,7 @@ do_read (struct command *cmd, struct vddk_handle *h)
}
static int
-do_write (struct command *cmd, struct vddk_handle *h)
+do_write (struct command *cmd)
{
VixError err;
uint32_t count = cmd->count;
@@ -261,7 +261,7 @@ do_write (struct command *cmd, struct vddk_handle *h)
"handle, %" PRIu64 " sectors, "
"%" PRIu32 " sectors, buffer, callback,
%" PRIu64,
offset, count, cmd->id)
- err = VixDiskLib_WriteAsync (h->handle, offset, count, buf,
+ err = VixDiskLib_WriteAsync (handle, offset, count, buf,
complete_command, cmd);
VDDK_CALL_END (VixDiskLib_WriteAsync, count * VIXDISKLIB_SECTOR_SIZE);
if (err != VIX_ASYNC) {
@@ -273,7 +273,7 @@ do_write (struct command *cmd, struct vddk_handle *h)
}
static int
-do_flush (struct command *cmd, struct vddk_handle *h)
+do_flush (struct command *cmd)
{
VixError err;
@@ -282,7 +282,7 @@ do_flush (struct command *cmd, struct vddk_handle *h)
* except to print them.
*/
VDDK_CALL_START (VixDiskLib_Wait, "handle")
- err = VixDiskLib_Wait (h->handle);
+ err = VixDiskLib_Wait (handle);
VDDK_CALL_END (VixDiskLib_Wait, 0);
if (err != VIX_OK)
VDDK_ERROR (err, "VixDiskLib_Wait");
@@ -295,7 +295,7 @@ do_flush (struct command *cmd, struct vddk_handle *h)
* here.
*/
VDDK_CALL_START (VixDiskLib_Flush, "handle")
- err = VixDiskLib_Flush (h->handle);
+ err = VixDiskLib_Flush (handle);
VDDK_CALL_END (VixDiskLib_Flush, 0);
if (err != VIX_OK) {
VDDK_ERROR (err, "VixDiskLib_Flush");
@@ -306,7 +306,7 @@ do_flush (struct command *cmd, struct vddk_handle *h)
}
static int
-do_can_extents (struct command *cmd, struct vddk_handle *h)
+do_can_extents (struct command *cmd)
{
VixError err;
VixDiskLibBlockList *block_list;
@@ -332,7 +332,7 @@ do_can_extents (struct command *cmd, struct vddk_handle *h)
VDDK_CALL_START (VixDiskLib_QueryAllocatedBlocks,
"handle, 0, %d sectors, %d sectors",
VIXDISKLIB_MIN_CHUNK_SIZE, VIXDISKLIB_MIN_CHUNK_SIZE)
- err = VixDiskLib_QueryAllocatedBlocks (h->handle,
+ err = VixDiskLib_QueryAllocatedBlocks (handle,
0, VIXDISKLIB_MIN_CHUNK_SIZE,
VIXDISKLIB_MIN_CHUNK_SIZE,
&block_list);
@@ -389,7 +389,7 @@ add_extent (struct nbdkit_extents *extents,
}
static int
-do_extents (struct command *cmd, struct vddk_handle *h)
+do_extents (struct command *cmd)
{
uint32_t count = cmd->count;
uint64_t offset = cmd->offset;
@@ -425,7 +425,7 @@ do_extents (struct command *cmd, struct vddk_handle *h)
"handle, %" PRIu64 " sectors, %"
PRIu64 " sectors, "
"%d sectors",
start_sector, nr_sectors, VIXDISKLIB_MIN_CHUNK_SIZE)
- err = VixDiskLib_QueryAllocatedBlocks (h->handle,
+ err = VixDiskLib_QueryAllocatedBlocks (handle,
start_sector, nr_sectors,
VIXDISKLIB_MIN_CHUNK_SIZE,
&block_list);
@@ -484,9 +484,8 @@ do_extents (struct command *cmd, struct vddk_handle *h)
* VDDK commands are issued.
*/
void *
-vddk_worker_thread (void *handle)
+vddk_worker_thread (void *unused)
{
- struct vddk_handle *h = handle;
bool stop = false;
while (!stop) {
@@ -496,21 +495,21 @@ vddk_worker_thread (void *handle)
/* Wait until we are sent at least one command. */
{
- ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&h->commands_lock);
- while (h->commands.len == 0)
- pthread_cond_wait (&h->commands_cond, &h->commands_lock);
- cmd = h->commands.ptr[0];
- command_queue_remove (&h->commands, 0);
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&commands_lock);
+ while (commands.len == 0)
+ pthread_cond_wait (&commands_cond, &commands_lock);
+ cmd = commands.ptr[0];
+ command_queue_remove (&commands, 0);
}
switch (cmd->type) {
case STOP:
- r = do_stop (cmd, h);
+ r = do_stop (cmd);
stop = true;
break;
case GET_SIZE: {
- int64_t size = do_get_size (cmd, h);
+ int64_t size = do_get_size (cmd);
if (size == -1)
r = -1;
else {
@@ -521,29 +520,29 @@ vddk_worker_thread (void *handle)
}
case READ:
- r = do_read (cmd, h);
+ r = do_read (cmd);
/* If async is true, don't retire this command now. */
async = r == 0;
break;
case WRITE:
- r = do_write (cmd, h);
+ r = do_write (cmd);
/* If async is true, don't retire this command now. */
async = r == 0;
break;
case FLUSH:
- r = do_flush (cmd, h);
+ r = do_flush (cmd);
break;
case CAN_EXTENTS:
- r = do_can_extents (cmd, h);
+ r = do_can_extents (cmd);
if (r >= 0)
*(int *)cmd->ptr = r;
break;
case EXTENTS:
- r = do_extents (cmd, h);
+ r = do_extents (cmd);
break;
default: abort (); /* impossible, but keeps GCC happy */
--
2.32.0
Richard W.M. Jones
2021-Dec-16 10:31 UTC
[Libguestfs] [PATCH nbdkit BROKEN 2/2] vddk: Only do QueryAllocatedBlocks test once
---
plugins/vddk/worker.c | 67 ++++++++++++++++++++++++-------------------
1 file changed, 38 insertions(+), 29 deletions(-)
diff --git a/plugins/vddk/worker.c b/plugins/vddk/worker.c
index 4bb7d8d7..a8ffc23e 100644
--- a/plugins/vddk/worker.c
+++ b/plugins/vddk/worker.c
@@ -305,9 +305,12 @@ do_flush (struct command *cmd)
return 0;
}
+static pthread_mutex_t extents_test_lock = PTHREAD_MUTEX_INITIALIZER;
+
static int
do_can_extents (struct command *cmd)
{
+ static int extents_test = -1;
VixError err;
VixDiskLibBlockList *block_list;
@@ -320,41 +323,47 @@ do_can_extents (struct command *cmd)
return 0;
}
- /* Suppress errors around this call. See:
- * https://bugzilla.redhat.com/show_bug.cgi?id=1709211#c7
- */
- error_suppression = 1;
-
/* However even when the call is available it rarely works well so
* the best thing we can do here is to try the call and if it's
* non-functional return false.
*/
- VDDK_CALL_START (VixDiskLib_QueryAllocatedBlocks,
- "handle, 0, %d sectors, %d sectors",
- VIXDISKLIB_MIN_CHUNK_SIZE, VIXDISKLIB_MIN_CHUNK_SIZE)
- err = VixDiskLib_QueryAllocatedBlocks (handle,
- 0, VIXDISKLIB_MIN_CHUNK_SIZE,
- VIXDISKLIB_MIN_CHUNK_SIZE,
- &block_list);
- VDDK_CALL_END (VixDiskLib_QueryAllocatedBlocks, 0);
- error_suppression = 0;
- if (err == VIX_OK) {
- VDDK_CALL_START (VixDiskLib_FreeBlockList, "block_list")
- VixDiskLib_FreeBlockList (block_list);
- VDDK_CALL_END (VixDiskLib_FreeBlockList, 0);
- }
- if (err != VIX_OK) {
- char *errmsg = VixDiskLib_GetErrorText (err, NULL);
- nbdkit_debug ("can_extents: "
- "VixDiskLib_QueryAllocatedBlocks test failed, "
- "extents support will be disabled: "
- "original error: %s",
- errmsg);
- VixDiskLib_FreeErrorText (errmsg);
- return 0;
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&extents_test_lock);
+ if (extents_test == -1) {
+ /* Suppress errors around this call. See:
+ * https://bugzilla.redhat.com/show_bug.cgi?id=1709211#c7
+ */
+ error_suppression = 1;
+
+ VDDK_CALL_START (VixDiskLib_QueryAllocatedBlocks,
+ "handle, 0, %d sectors, %d sectors",
+ VIXDISKLIB_MIN_CHUNK_SIZE, VIXDISKLIB_MIN_CHUNK_SIZE)
+ err = VixDiskLib_QueryAllocatedBlocks (handle,
+ 0, VIXDISKLIB_MIN_CHUNK_SIZE,
+ VIXDISKLIB_MIN_CHUNK_SIZE,
+ &block_list);
+ VDDK_CALL_END (VixDiskLib_QueryAllocatedBlocks, 0);
+ error_suppression = 0;
+ if (err == VIX_OK) {
+ VDDK_CALL_START (VixDiskLib_FreeBlockList, "block_list")
+ VixDiskLib_FreeBlockList (block_list);
+ VDDK_CALL_END (VixDiskLib_FreeBlockList, 0);
+
+ extents_test = 1; /* result = good */
+ }
+ else {
+ char *errmsg = VixDiskLib_GetErrorText (err, NULL);
+ nbdkit_debug ("can_extents: "
+ "VixDiskLib_QueryAllocatedBlocks test failed, "
+ "extents support will be disabled: "
+ "original error: %s",
+ errmsg);
+ VixDiskLib_FreeErrorText (errmsg);
+
+ extents_test = 0; /* result = bad */
+ }
}
- return 1;
+ return extents_test;
}
/* Add an extent to the list of extents. */
--
2.32.0