Fix #2553: Prevent the Deadlock by aligning the lockorder

Overview:
Every once in a while the /etc/pve directory freezes. ("ls" and "df"
does not work) Therefore the most pve components do not work anymore.
(webinterface is not answering, shell commands do not work) This
mostly happens during snapshots, which happen frequently in my case.
The workaround / temporary solution is to restart the pve-cluster
service.

Steps to reproduce:
Make frequent snapshots/snapshot-deletes on a default installation.
The /etc/pve direcetory will freeze at some point.

Cause;
When a snapshot is made, it eventually invokes memdb_rename
(memdb.c:1103), which at first locks the memdb->mutex at memdb.c:1122
and then invokes the methods vmlist_different_vm_exists
(memdb.c:1147) or vmlist_register_vm (memdb.c:1233).  These methods
are defined in status.c and want to lock the mutex lock of
(status.c:689 and status.c:669.

The deadlock appears when cfs_create_guest_conf_propertiy_msg aquires
the status.c mutex lock while memdb_rename aquires the memdb.c mutex
lock at the same time. Then cfs_create_guest_conf_propertiy_msg wants
to lock the memdb.c lock at memdb_read (which is held by
memdb_rename) and vmlist_different_vm_exists or vmlist_register_vm
wants to lock the status.c lock (which is held by
cfs_create_guest_conf_propertiy_msg). Both methods are waiting for
each other to unlock their locks -> deadlock.

Fix:
Fix by aligning the lockorder of the memdb and status mutex lock
calls.

Lock &memdb->mutex in memdb_read and refer to a new method
"memdb_read_nolock" in memdb.c which doesn't handle locks by itself.
This method then handles the stuff which was originally in
memdb_read. Therefore everything except
cfs_create_guest_conf_property_msg uses memdb_read (which handles the
locking itself), and cfs_create_guest_conf_property_msg prelocks
&memdb->mutex and invokes memdb_read_nolock.

Signed-off-by: Kevin Greßlehner <kevin_gressi@live.at>
[ added more info from bug report & fixed indentation/line endings ]
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

diff --git a/data/src/memdb.c b/data/src/memdb.c
index efc99ea73f776920873acb5cc03fe5ad9b01eb1f..56930723c9dcc316a0c56c6870fb45acc16e0ed7 100644 (file)
--- a/data/src/memdb.c
+++ b/data/src/memdb.c
@@ -659,32 +659,44 @@ int memdb_mkdir(
        return ret;
 }
 
-int 
-memdb_read(
-       memdb_t *memdb, 
-       const char *path, 
+// Original memdb_read without locking - Caller MUST handle the locking
+int
+memdb_read_nolock(
+       memdb_t *memdb,
+       const char *path,
        gpointer *data_ret)
 {
        g_return_val_if_fail(memdb != NULL, -EINVAL);
        g_return_val_if_fail(path != NULL, -EINVAL);
        g_return_val_if_fail(data_ret != NULL, -EINVAL);
 
-       memdb_tree_entry_t *te, *parent; 
-
-       g_mutex_lock (&memdb->mutex);
+       memdb_tree_entry_t *te, *parent;
 
        if ((te = memdb_lookup_path(memdb, path, &parent))) {
                if (te->type == DT_REG) {
                        *data_ret = g_memdup(te->data.value, te->size);
                        guint32 size = te->size;
-                       g_mutex_unlock (&memdb->mutex);
                        return size;
                }
        }
 
+       return -ENOENT;
+}
+
+int
+memdb_read(
+       memdb_t *memdb,
+       const char *path,
+       gpointer *data_ret)
+{
+       int res;
+       g_mutex_lock (&memdb->mutex);
+
+       res = memdb_read_nolock(memdb, path, data_ret);
+
        g_mutex_unlock (&memdb->mutex);
 
-       return -ENOENT;
+       return res;
 }
 
 static int 

diff --git a/data/src/memdb.h b/data/src/memdb.h
index 60df035710551c270e20c44852e91ed9f4a14a7a..76593585fa25fcbeccb3da9855a3b6b5a9416271 100644 (file)
--- a/data/src/memdb.h
+++ b/data/src/memdb.h
@@ -162,6 +162,12 @@ memdb_read(
        const char *path,
        gpointer *data_ret);
 
+int
+memdb_read_nolock(
+        memdb_t *memdb,
+        const char *path,
+        gpointer *data_ret);
+
 int
 memdb_create(
        memdb_t *memdb,

diff --git a/data/src/status.c b/data/src/status.c
index 491082c8763f5e5bcd45a798bf74d416ed4510c6..d6dccc13ea484e0aba0efd0e0ba98f0f4b28e4ed 100644 (file)
--- a/data/src/status.c
+++ b/data/src/status.c
@@ -883,6 +883,9 @@ cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *pro
        int res = 0;
        GString *path = NULL;
 
+       // Prelock &memdb->mutex in order to enable the usage of memdb_read_nolock
+       // to prevent Deadlocks as in #2553
+       g_mutex_lock (&memdb->mutex);
        g_mutex_lock (&mutex);
 
        g_string_printf(str,"{\n");
@@ -900,7 +903,8 @@ cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *pro
 
                if (!vminfo_to_path(vminfo, path)) goto err;
 
-               int size = memdb_read(memdb, path->str, &tmp);
+               // use memdb_read_nolock because lock is handled here
+               int size = memdb_read_nolock(memdb, path->str, &tmp);
                if (tmp == NULL) goto err;
                if (size <= prop_len) goto ret;
 
@@ -924,7 +928,8 @@ cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *pro
 
                        g_free(tmp); // no-op if already null
                        tmp = NULL;
-                       int size = memdb_read(memdb, path->str, &tmp);
+                       // use memdb_read_nolock because lock is handled here
+                       int size = memdb_read_nolock(memdb, path->str, &tmp);
                        if (tmp == NULL || size <= prop_len) continue;
 
                        char *val = _get_property_value(tmp, size, prop, prop_len);
@@ -945,7 +950,7 @@ ret:
        }
        g_string_append_printf(str,"\n}\n");
        g_mutex_unlock (&mutex);
-
+       g_mutex_unlock (&memdb->mutex);
        return res;
 err:
        res = -EIO;