Libguestfs - Jun 2014 - Re: [PATCH] library: per-handle locking support

Introduces private pool of muteces (per_handle_locks) guarded by read-write
mutex (locks_lock) using guestfs__per_handle_lock_lock(g) and
guestfs__per_handle_lock_unlock(g), where g is current valid guestfs_h handle.
Above two are used in generator/c.ml at begin and end of each non-deamon call.

New lock can be created using guestfs__per_handle_lock_add(g) and
destroyed with guestfs__per_handle_lock_remove(g). These two are called
automatically too. First one is called by guestfs_create and second by
guestfs_destroy.
---
 generator/c.ml  |  10 ++++
 src/Makefile.am |   1 +
 src/handle.c    |   4 ++
 src/locking.c   | 162 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/locking.h   |  45 ++++++++++++++++
 5 files changed, 222 insertions(+)
 create mode 100644 src/locking.c
 create mode 100644 src/locking.h

diff --git a/generator/c.ml b/generator/c.ml
index ee276dc..e67053e 100644
--- a/generator/c.ml
+++ b/generator/c.ml
@@ -788,6 +788,9 @@ and generate_internal_actions_h ()    pr "#define
GUESTFS_INTERNAL_ACTIONS_H_\n";
   pr "\n";
 
+  pr "#include \"locking.h\"\n";
+  pr "\n";
+
   List.iter (
     fun { c_name = c_name; style = style } ->
       generate_prototype ~single_line:true ~newline:true ~handle:"g"
@@ -1569,6 +1572,9 @@ and generate_client_actions hash ()  
     handle_null_optargs optargs c_name;
 
+    pr "  guestfs___per_handle_lock_lock (g);\n";
+    pr "\n";
+
     pr "  int trace_flag = g->trace;\n";
     pr "  struct trace_buffer trace_buffer;\n";
     (match ret with
@@ -1617,6 +1623,10 @@ and generate_client_actions hash ()        trace_return
name style "r";
     );
     pr "\n";
+
+    pr "  guestfs___per_handle_lock_unlock (g);\n";
+    pr "\n";
+
     pr "  return r;\n";
     pr "}\n";
     pr "\n"
diff --git a/src/Makefile.am b/src/Makefile.am
index 3d06203..f1f42d0 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -118,6 +118,7 @@ libguestfs_la_SOURCES = \
 	libvirt-auth.c \
 	libvirt-domain.c \
 	listfs.c \
+	locking.c \
 	lpj.c \
 	match.c \
 	osinfo.c \
diff --git a/src/handle.c b/src/handle.c
index 719bbcd..8397157 100644
--- a/src/handle.c
+++ b/src/handle.c
@@ -84,6 +84,8 @@ guestfs_create_flags (unsigned flags, ...)
   g = calloc (1, sizeof (*g));
   if (!g) return NULL;
 
+  guestfs___per_handle_lock_add (g);
+
   g->state = CONFIG;
 
   g->conn = NULL;
@@ -305,6 +307,8 @@ guestfs_close (guestfs_h *g)
     return;
   }
 
+  guestfs___per_handle_lock_remove (g);
+
   /* Remove the handle from the handles list. */
   if (g->close_on_exit) {
     gl_lock_lock (handles_lock);
diff --git a/src/locking.c b/src/locking.c
new file mode 100644
index 0000000..2a35fce
--- /dev/null
+++ b/src/locking.c
@@ -0,0 +1,162 @@
+/* libguestfs
+ * Copyright (C) 2009-2014 Red Hat Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include <errno.h>
+
+#include "glthread/lock.h"
+#include "ignore-value.h"
+
+#include "guestfs.h"
+#include "guestfs-internal.h"
+#include "guestfs-internal-actions.h"
+#include "guestfs_protocol.h"
+
+#include "locking.h"
+
+static gl_lock_t guestfs___lookup_lock_or_abort (guestfs_h *g, int *pos);
+static void guestfs___per_handle_lock_is_not_held (guestfs_h *g);
+static void ___shrink_lock_arrays (void);
+
+
+gl_lock_define (static, *per_handle_locks)
+gl_rwlock_define_initialized (static, locks_lock)
+
+/* items in handles array bijectively corresponds to items in per_handle_locks
+ */
+static guestfs_h ** handles;
+static size_t n_handles;
+
+
+static gl_lock_t
+guestfs___lookup_lock_or_abort (guestfs_h *g, int *pos)
+{
+  // this function should be called with locks_lock already held
+  for (size_t i = 0; i < n_handles; i++) {
+    if (handles[i] == g) {
+      if (pos != NULL) {
+        *pos = i;
+      }
+      return per_handle_locks[i];
+    }
+  }
+  // handle not found. aborting
+  error (g, _("guestfs lock handle not found"));
+  abort ();
+}
+
+static void
+guestfs___per_handle_lock_is_not_held (guestfs_h *g)
+{
+  // this function should be called with locks_lock already held
+
+  int i;
+  gl_lock_t l = guestfs___lookup_lock_or_abort (g, &i);
+
+  gl_lock_lock (l);
+  gl_lock_unlock (l);
+}
+
+void
+guestfs___per_handle_lock_add (guestfs_h *g)
+{
+  gl_rwlock_wrlock (locks_lock);
+
+  n_handles++;
+  per_handle_locks = realloc (per_handle_locks,
+      sizeof (*per_handle_locks) * n_handles);
+  handles = realloc (handles, sizeof (*handles) * n_handles);
+
+  gl_lock_init (per_handle_locks[n_handles - 1]);
+  handles[n_handles - 1] = g;
+
+  gl_rwlock_unlock (locks_lock);
+}
+
+static void
+___shrink_lock_arrays (void)
+{
+  // locks_lock should be held
+
+  int i;
+  guestfs___lookup_lock_or_abort (NULL, &i);
+
+  memmove (handles + i, handles + i + 1, (n_handles - i - 1)
+      * sizeof (*handles));
+  memmove (per_handle_locks + i, per_handle_locks + i + 1,
+      (n_handles - i - 1) * sizeof (*per_handle_locks));
+  if (handles == NULL || per_handle_locks == NULL) {
+    abort ();
+  }
+}
+
+void
+guestfs___per_handle_lock_remove (guestfs_h *g)
+{
+  // locks_lock prevents per_handle_locks manipulation within this module
+  // take write lock, since we're going to update guarded per_handle_locks
+  gl_rwlock_wrlock (locks_lock);
+
+  // ensure we don't get into data race when removing lock
+  guestfs___per_handle_lock_is_not_held (g);
+
+  int i;
+  gl_lock_t l = guestfs___lookup_lock_or_abort (g, &i);
+  gl_lock_destroy (l);
+  handles[i] = NULL;
+  // fill in hole we've created
+  ___shrink_lock_arrays ();
+
+  // and realloc
+  n_handles--;
+  if (n_handles > 0) {
+    per_handle_locks = realloc (per_handle_locks,
+        sizeof (*per_handle_locks) * n_handles);
+    handles = realloc (handles, sizeof (*handles) * n_handles);
+  }
+
+  gl_rwlock_unlock (locks_lock);
+}
+
+void
+guestfs___per_handle_lock_lock (guestfs_h *g)
+{
+  gl_rwlock_rdlock (locks_lock);
+
+  gl_lock_t l = guestfs___lookup_lock_or_abort(g, NULL);
+  gl_lock_lock(l);
+
+  gl_rwlock_unlock (locks_lock);
+}
+
+void
+guestfs___per_handle_lock_unlock (guestfs_h *g)
+{
+  gl_rwlock_rdlock (locks_lock);
+
+  gl_lock_t l = guestfs___lookup_lock_or_abort(g, NULL);
+  gl_lock_unlock(l);
+
+  gl_rwlock_unlock (locks_lock);
+}
+
diff --git a/src/locking.h b/src/locking.h
new file mode 100644
index 0000000..fc7bc46
--- /dev/null
+++ b/src/locking.h
@@ -0,0 +1,45 @@
+/* libguestfs
+ * Copyright (C) 2009-2014 Red Hat Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#ifndef _LOCKING_H
+#define _LOCKING_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <config.h>
+
+#include "glthread/lock.h"
+
+#include "guestfs.h"
+#include "guestfs-internal.h"
+
+extern void guestfs___per_handle_lock_add (guestfs_h *g);
+
+extern void guestfs___per_handle_lock_remove (guestfs_h *g);
+
+extern void guestfs___per_handle_lock_lock (guestfs_h *g);
+
+extern void guestfs___per_handle_lock_unlock (guestfs_h *g);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _LOCKING_H */
-- 
1.9.3

On Thu, May 29, 2014 at 12:21:39PM +0200, Maros Zatko
wrote:
> Introduces private pool of muteces (per_handle_locks) guarded by read-write
> mutex (locks_lock) using guestfs__per_handle_lock_lock(g) and
> guestfs__per_handle_lock_unlock(g), where g is current valid guestfs_h
handle.
> Above two are used in generator/c.ml at begin and end of each non-deamon
call.
> 
> New lock can be created using guestfs__per_handle_lock_add(g) and
> destroyed with guestfs__per_handle_lock_remove(g). These two are called
> automatically too. First one is called by guestfs_create and second by
> guestfs_destroy.
I'm a bit confused about why there is a pool of locks.  Why not keep
the lock struct in the handle (guestfs_h)?

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

On 05/29/2014 01:04 PM, Richard W.M. Jones wrote:
> On Thu, May 29, 2014 at 12:21:39PM +0200, Maros Zatko wrote:
>> Introduces private pool of muteces (per_handle_locks) guarded by
read-write
>> mutex (locks_lock) using guestfs__per_handle_lock_lock(g) and
>> guestfs__per_handle_lock_unlock(g), where g is current valid guestfs_h
handle.
>> Above two are used in generator/c.ml at begin and end of each
non-deamon call.
>>
>> New lock can be created using guestfs__per_handle_lock_add(g) and
>> destroyed with guestfs__per_handle_lock_remove(g). These two are called
>> automatically too. First one is called by guestfs_create and second by
>> guestfs_destroy.
> I'm a bit confused about why there is a pool of locks.  Why not keep
> the lock struct in the handle (guestfs_h)?
Yes, why not (obviously).
>
> Rich.
>
First I've put recursive mutex into handle. This worked.
Next, due to previous discussion about this recursive manner, I've tried 
it with ordinary ones.
This doesn't work. make check hangs on tests/qemu, other tests try to 
double acquire lock and hangs,
same goes with guestfish. I found interesting that you can run 
guestfish, add a disk but you can't launch it.
Unless you set LIBGUESTFS_DEBUG or TRACE flag then it hangs immediately :)

Do we want to have it recursive or investigate more for exact races and 
have fine grained locking?
Ideas?

Patches will follow.

- maros