Libguestfs - Aug 2020 - Re: [PATCH nbdkit] vddk: Relax threading model and enable multi-conn.

In theory this patch depends on this series:
https://www.redhat.com/archives/libguestfs/2020-August/msg00021.html

In practice I believe they're independent of each other, but the above
series makes it easier to test.

Rich.

See comment in code and
https://www.redhat.com/archives/libguestfs/2020-August/msg00023.html
---
 plugins/vddk/vddk.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index 31e58f66..95e030d4 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -511,17 +511,29 @@ 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.
+ *
+ * We also enabled multi-conn for readonly connections since it
+ * appears possible for clients to open multiple handles even if they
+ * point to the same server/disk.
  */
-#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 {
   VixDiskLibConnectParams *params; /* connection parameters */
   VixDiskLibConnection connection; /* connection */
   VixDiskLibHandle handle;         /* disk handle */
+  int readonly;
 };
 
 static inline VixDiskLibConnectParams *
@@ -557,6 +569,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;
@@ -567,6 +580,7 @@ vddk_open (int readonly)
     return NULL;
   }
 
+  h->readonly = readonly;
   h->params = allocate_connect_params ();
   if (h->params == NULL) {
     nbdkit_error ("allocate VixDiskLibConnectParams: %m");
@@ -649,6 +663,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");
@@ -659,6 +674,14 @@ vddk_close (void *handle)
   free (h);
 }
 
+/* Allow multi-conn - see comment about threading above. */
+static int
+vddk_can_multi_conn (void *handle)
+{
+  struct vddk_handle *h = handle;
+  return h->readonly;
+}
+
 /* Get the file size. */
 static int64_t
 vddk_get_size (void *handle)
@@ -986,6 +1009,7 @@ static struct nbdkit_plugin plugin = {
   .after_fork        = vddk_after_fork,
   .open              = vddk_open,
   .close             = vddk_close,
+  .can_multi_conn    = vddk_can_multi_conn,
   .get_size          = vddk_get_size,
   .pread             = vddk_pread,
   .pwrite            = vddk_pwrite,
-- 
2.27.0

On 8/5/20 7:48 AM, Richard W.M. Jones wrote:
> See comment in code and
> https://www.redhat.com/archives/libguestfs/2020-August/msg00023.html
> ---
>   plugins/vddk/vddk.c | 32 ++++++++++++++++++++++++++++----
>   1 file changed, 28 insertions(+), 4 deletions(-)
I'll have to trust your testing, but code-wise, this looks safe.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org