From fad234b5a62df48b7abc726549f2abb6b0af7c04 Mon Sep 17 00:00:00 2001
From: Mohit Agrawal <moagrawa@redhat.com>
Date: Tue, 16 Oct 2018 07:50:47 +0530
Subject: [PATCH 402/404] core: glusterfsd keeping fd open in index xlator
Problem: At the time of processing GF_EVENT_PARENT_DOWN
at brick xlator, it forwards the event to next xlator
only while xlator ensures no stub is in progress.
At io-thread xlator it decreases stub_cnt before the process
a stub and notify EVENT to next xlator
Solution: Introduce a new counter to save stub_cnt and decrease
the counter after process the stub completely at io-thread
xlator.
To avoid brick crash at the time of call xlator_mem_cleanup
move only brick xlator if detach brick name has found in
the graph
Note: Thanks to pranith for sharing a simple reproducer to
reproduce the same
> fixes bz#1637934
> Change-Id: I1a694a001f7a5417e8771e3adf92c518969b6baa
> (Cherry pick from commit 7bf95631b52bd05b06122180f8bd4aa62c70b1a9)
> (Reviewed on upstream link https://review.gluster.org/#/c/glusterfs/+/21379/)
Change-Id: I54b8ebb19819f9bbcbdd1448474ab084c0fd2eb6
BUG: 1631372
Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>
Reviewed-on: https://code.engineering.redhat.com/gerrit/152908
Tested-by: RHGS Build Bot <nigelb@redhat.com>
Reviewed-by: Sunil Kumar Heggodu Gopala Acharya <sheggodu@redhat.com>
---
glusterfsd/src/glusterfsd-mgmt.c | 15 +----
tests/bugs/glusterd/brick-mux-fd-cleanup.t | 78 +++++++++++++++++++++++++
xlators/performance/io-threads/src/io-threads.c | 23 ++++----
xlators/performance/io-threads/src/io-threads.h | 3 +-
4 files changed, 94 insertions(+), 25 deletions(-)
create mode 100644 tests/bugs/glusterd/brick-mux-fd-cleanup.t
diff --git a/glusterfsd/src/glusterfsd-mgmt.c b/glusterfsd/src/glusterfsd-mgmt.c
index cbd436a..e3fceeb 100644
--- a/glusterfsd/src/glusterfsd-mgmt.c
+++ b/glusterfsd/src/glusterfsd-mgmt.c
@@ -270,6 +270,7 @@ xlator_mem_cleanup (xlator_t *this) {
top = glusterfsd_ctx->active->first;
LOCK (&ctx->volfile_lock);
/* TODO here we have leak for xlator node in a graph */
+ /* Need to move only top xlator from a graph */
for (trav_p = &top->children; *trav_p; trav_p = &(*trav_p)->next) {
victim = (*trav_p)->xlator;
if (victim->call_cleanup && !strcmp (victim->name, this->name)) {
@@ -277,20 +278,6 @@ xlator_mem_cleanup (xlator_t *this) {
break;
}
}
- /* TODO Sometime brick xlator is not moved from graph so followed below
- approach to move brick xlator from a graph, will move specific brick
- xlator from graph only while inode table and mem_acct are cleaned up
- */
- trav_p = &top->children;
- while (*trav_p) {
- victim = (*trav_p)->xlator;
- if (victim->call_cleanup && !victim->itable && !victim->mem_acct) {
- (*trav_p) = (*trav_p)->next;
- } else {
- trav_p = &(*trav_p)->next;
- }
- }
- UNLOCK (&ctx->volfile_lock);
}
}
diff --git a/tests/bugs/glusterd/brick-mux-fd-cleanup.t b/tests/bugs/glusterd/brick-mux-fd-cleanup.t
new file mode 100644
index 0000000..de11c17
--- /dev/null
+++ b/tests/bugs/glusterd/brick-mux-fd-cleanup.t
@@ -0,0 +1,78 @@
+#!/bin/bash
+
+. $(dirname $0)/../../include.rc
+. $(dirname $0)/../../volume.rc
+
+#This .t tests that the fds from client are closed on brick when gluster volume
+#stop is executed in brick-mux setup.
+
+cleanup;
+TEST glusterd
+TEST pidof glusterd
+
+function keep_fd_open {
+#This function has to be run as background job because opening the fd in
+#foreground and running commands is leading to flush calls on these fds
+#which is making it very difficult to create the race where fds will be left
+#open even after the brick dies.
+ exec 5>$M1/a
+ exec 6>$M1/b
+ while [ -f $M0/a ]; do sleep 1; done
+}
+
+function count_open_files {
+ local brick_pid="$1"
+ local pattern="$2"
+ ls -l /proc/$brick_pid/fd | grep -i "$pattern" | wc -l
+}
+
+TEST $CLI volume set all cluster.brick-multiplex on
+TEST $CLI volume create $V0 replica 2 $H0:$B0/${V0}{0,1}
+TEST $CLI volume create $V1 replica 2 $H0:$B0/${V1}{2,3}
+#Have same configuration on both bricks so that they are multiplexed
+#Delay flush fop for a second
+TEST $CLI volume heal $V0 disable
+TEST $CLI volume heal $V1 disable
+TEST $CLI volume set $V0 delay-gen posix
+TEST $CLI volume set $V0 delay-gen.enable flush
+TEST $CLI volume set $V0 delay-gen.delay-percentage 100
+TEST $CLI volume set $V0 delay-gen.delay-duration 1000000
+TEST $CLI volume set $V1 delay-gen posix
+TEST $CLI volume set $V1 delay-gen.enable flush
+TEST $CLI volume set $V1 delay-gen.delay-percentage 100
+TEST $CLI volume set $V1 delay-gen.delay-duration 1000000
+
+TEST $CLI volume start $V0
+TEST $CLI volume start $V1
+
+TEST $GFS -s $H0 --volfile-id=$V0 --direct-io-mode=enable $M0
+TEST $GFS -s $H0 --volfile-id=$V1 --direct-io-mode=enable $M1
+
+TEST touch $M0/a
+keep_fd_open &
+TEST $CLI volume profile $V1 start
+brick_pid=$(get_brick_pid $V1 $H0 $B0/${V1}2)
+TEST count_open_files $brick_pid "$B0/${V1}2/a"
+TEST count_open_files $brick_pid "$B0/${V1}2/b"
+TEST count_open_files $brick_pid "$B0/${V1}3/a"
+TEST count_open_files $brick_pid "$B0/${V1}3/b"
+
+#If any other flush fops are introduced into the system other than the one at
+#cleanup it interferes with the race, so test for it
+EXPECT "^0$" echo "$($CLI volume profile $V1 info incremental | grep -i flush | wc -l)"
+#Stop the volume
+TEST $CLI volume stop $V1
+
+#Wait for cleanup resources or volume V1
+EXPECT_WITHIN $GRAPH_SWITCH_TIMEOUT "^0$" count_open_files $brick_pid "$B0/${V1}2/a"
+EXPECT_WITHIN $GRAPH_SWITCH_TIMEOUT "^0$" count_open_files $brick_pid "$B0/${V1}2/b"
+EXPECT_WITHIN $GRAPH_SWITCH_TIMEOUT "^0$" count_open_files $brick_pid "$B0/${V1}3/a"
+EXPECT_WITHIN $GRAPH_SWITCH_TIMEOUT "^0$" count_open_files $brick_pid "$B0/${V1}3/b"
+
+TEST rm -f $M0/a #Exit keep_fd_open()
+wait
+
+EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M0
+EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M1
+
+cleanup
diff --git a/xlators/performance/io-threads/src/io-threads.c b/xlators/performance/io-threads/src/io-threads.c
index 41d48ab..2944c7d 100644
--- a/xlators/performance/io-threads/src/io-threads.c
+++ b/xlators/performance/io-threads/src/io-threads.c
@@ -120,7 +120,7 @@ __iot_dequeue (iot_conf_t *conf, int *pri)
if (!stub)
return NULL;
- GF_ATOMIC_DEC(conf->queue_size);
+ conf->queue_size--;
conf->queue_sizes[*pri]--;
return stub;
@@ -153,7 +153,8 @@ __iot_enqueue (iot_conf_t *conf, call_stub_t *stub, int pri)
}
list_add_tail (&stub->list, &ctx->reqs);
- GF_ATOMIC_INC(conf->queue_size);
+ conf->queue_size++;
+ GF_ATOMIC_INC(conf->stub_cnt);
conf->queue_sizes[pri]++;
}
@@ -182,7 +183,7 @@ iot_worker (void *data)
conf->ac_iot_count[pri]--;
pri = -1;
}
- while (GF_ATOMIC_GET(conf->queue_size) == 0) {
+ while (conf->queue_size == 0) {
if (conf->down) {
bye = _gf_true;/*Avoid sleep*/
break;
@@ -220,8 +221,10 @@ iot_worker (void *data)
}
pthread_mutex_unlock (&conf->mutex);
- if (stub) /* guard against spurious wakeups */
+ if (stub) { /* guard against spurious wakeups */
call_resume (stub);
+ GF_ATOMIC_DEC(conf->stub_cnt);
+ }
stub = NULL;
if (bye)
@@ -816,7 +819,7 @@ __iot_workers_scale (iot_conf_t *conf)
gf_msg_debug (conf->this->name, 0,
"scaled threads to %d (queue_size=%d/%d)",
conf->curr_count,
- GF_ATOMIC_GET(conf->queue_size), scale);
+ conf->queue_size, scale);
} else {
break;
}
@@ -1030,7 +1033,7 @@ init (xlator_t *this)
bool, out);
conf->this = this;
- GF_ATOMIC_INIT(conf->queue_size, 0);
+ GF_ATOMIC_INIT(conf->stub_cnt, 0);
for (i = 0; i < IOT_PRI_MAX; i++) {
INIT_LIST_HEAD (&conf->clients[i]);
@@ -1075,7 +1078,7 @@ notify (xlator_t *this, int32_t event, void *data, ...)
{
iot_conf_t *conf = this->private;
xlator_t *victim = data;
- uint64_t queue_size = 0;
+ uint64_t stub_cnt = 0;
struct timespec sleep_till = {0, };
if (GF_EVENT_PARENT_DOWN == event) {
@@ -1083,14 +1086,14 @@ notify (xlator_t *this, int32_t event, void *data, ...)
clock_gettime(CLOCK_REALTIME, &sleep_till);
sleep_till.tv_sec += 1;
/* Wait for draining stub from queue before notify PARENT_DOWN */
- queue_size = GF_ATOMIC_GET(conf->queue_size);
+ stub_cnt = GF_ATOMIC_GET(conf->stub_cnt);
pthread_mutex_lock(&conf->mutex);
{
- while (queue_size) {
+ while (stub_cnt) {
(void)pthread_cond_timedwait(&conf->cond, &conf->mutex,
&sleep_till);
- queue_size = GF_ATOMIC_GET(conf->queue_size);
+ stub_cnt = GF_ATOMIC_GET(conf->stub_cnt);
}
}
pthread_mutex_unlock(&conf->mutex);
diff --git a/xlators/performance/io-threads/src/io-threads.h b/xlators/performance/io-threads/src/io-threads.h
index 7a6973c..57a136e 100644
--- a/xlators/performance/io-threads/src/io-threads.h
+++ b/xlators/performance/io-threads/src/io-threads.h
@@ -75,7 +75,8 @@ struct iot_conf {
int32_t ac_iot_limit[IOT_PRI_MAX];
int32_t ac_iot_count[IOT_PRI_MAX];
int queue_sizes[IOT_PRI_MAX];
- gf_atomic_t queue_size;
+ int32_t queue_size;
+ gf_atomic_t stub_cnt;
pthread_attr_t w_attr;
gf_boolean_t least_priority; /*Enable/Disable least-priority */
--
1.8.3.1