From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: pve-devel@lists.proxmox.com
Subject: [pve-devel] [PATCH cluster] pmxcfs: protect CPG operations with mutex
Date: Wed, 30 Sep 2020 13:21:31 +0200 [thread overview]
Message-ID: <20200930112131.2044392-1-f.gruenbichler@proxmox.com> (raw)
cpg_mcast_joined (and transitively, cpg_join/leave) are not thread-safe.
pmxcfs triggers such operations via FUSE and CPG dispatch callbacks,
which are running in concurrent threads.
accordingly, we need to protect these operations with a mutex, otherwise
they might return CS_OK without actually doing what they were supposed
to do (which in turn can lead to the dfsm taking a wrong turn and
getting stuck in a supposedly short-lived state, blocking access via
FUSE and getting whole clusters fenced).
huge thanks to Alexandre Derumier for providing the initial bug report
and quite a lot of test runs while debugging this issue.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Notes:
we could recycle sync_mutex, but that makes it harder to reason
about securing all code paths. it also protects non CPG operations
as part of the sync messsage queue handling, so mixing those up is
non-ideal.
@Alexandre: this is a slightly different approach compared to the test
build from yesterday, so if you want to test this as well it would
be very welcome :)
data/src/dfsm.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/data/src/dfsm.c b/data/src/dfsm.c
index 172d877..17a3ba4 100644
--- a/data/src/dfsm.c
+++ b/data/src/dfsm.c
@@ -107,6 +107,7 @@ struct dfsm {
cpg_callbacks_t *cpg_callbacks;
dfsm_callbacks_t *dfsm_callbacks;
cpg_handle_t cpg_handle;
+ GMutex cpg_mutex;
struct cpg_name cpg_group_name;
uint32_t nodeid;
uint32_t pid;
@@ -204,7 +205,9 @@ dfsm_send_message_full(
cs_error_t result;
int retries = 0;
loop:
+ g_mutex_lock (&dfsm->cpg_mutex);
result = cpg_mcast_joined(dfsm->cpg_handle, CPG_TYPE_AGREED, iov, len);
+ g_mutex_unlock (&dfsm->cpg_mutex);
if (retry && result == CS_ERR_TRY_AGAIN) {
nanosleep(&tvreq, NULL);
++retries;
@@ -1250,7 +1253,9 @@ dfsm_new(
if (!(dfsm->msg_queue = g_sequence_new(NULL)))
goto err;
-
+
+ g_mutex_init(&dfsm->cpg_mutex);
+
dfsm->log_domain = log_domain;
dfsm->data = data;
dfsm->mode = DFSM_MODE_START;
@@ -1424,7 +1429,9 @@ dfsm_join(dfsm_t *dfsm)
struct timespec tvreq = { .tv_sec = 0, .tv_nsec = 100000000 };
int retries = 0;
loop:
+ g_mutex_lock (&dfsm->cpg_mutex);
result = cpg_join(dfsm->cpg_handle, &dfsm->cpg_group_name);
+ g_mutex_unlock (&dfsm->cpg_mutex);
if (result == CS_ERR_TRY_AGAIN) {
nanosleep(&tvreq, NULL);
++retries;
@@ -1453,7 +1460,9 @@ dfsm_leave (dfsm_t *dfsm)
struct timespec tvreq = { .tv_sec = 0, .tv_nsec = 100000000 };
int retries = 0;
loop:
+ g_mutex_lock (&dfsm->cpg_mutex);
result = cpg_leave(dfsm->cpg_handle, &dfsm->cpg_group_name);
+ g_mutex_unlock (&dfsm->cpg_mutex);
if (result == CS_ERR_TRY_AGAIN) {
nanosleep(&tvreq, NULL);
++retries;
@@ -1509,6 +1518,8 @@ dfsm_destroy(dfsm_t *dfsm)
g_mutex_clear (&dfsm->sync_mutex);
g_cond_clear (&dfsm->sync_cond);
+
+ g_mutex_clear (&dfsm->cpg_mutex);
if (dfsm->results)
g_hash_table_destroy(dfsm->results);
--
2.20.1
next reply other threads:[~2020-09-30 11:22 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-30 11:21 Fabian Grünbichler [this message]
2020-09-30 11:47 ` [pve-devel] applied: " Thomas Lamprecht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200930112131.2044392-1-f.gruenbichler@proxmox.com \
--to=f.gruenbichler@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox