path: root/xlators
diff options
authorKotresh HR <>2016-03-07 11:45:07 +0530
committerRaghavendra G <>2016-07-27 03:13:33 -0700
commit5a32c26bb59bdd20bdfc9ea00ce90c7d1c64aa04 (patch)
tree924854993ddd704b246ea3d6f3e7aeb3a53e5684 /xlators
parentf32fd3b0807e9eeeb3e7deb664459493a099010f (diff)
changelog/rpc: Fix rpc_clnt_t mem leaks
Backport of PROBLEM: 1. Freeing up rpc_clnt object might lead to crashes. Well, it was not a necessity to free rpc-clnt object till now because all the existing use cases needs to reconnect back on disconnects. Hence timer code was not taking ref on rpc-clnt object. Glusterd had some use-cases that led to crash due to ping-timer and they fixed only those code paths that involve ping-timer. Now, since changelog has an use-case where rpc-clnt need to be freed up, we need to fix timer code to take refs 2. In changelog, because of issue 1, only mydata was being freed which is incorrect. And there are races where rpc-clnt object would access the freed mydata which would lead to crashes. Since changelog xlator resides on brick side and is long living process, if multiple libgfchangelog consumers register to changelog and disconnect/reconnect mulitple times, it would result in leak of 'rpc-clnt' object for every connect/disconnect. SOLUTION: 1. Handle ref/unref of 'rpc_clnt' structure in timer functions properly. 2. In changelog, unref 'rpc_clnt' in RPC_CLNT_DISCONNECT after disabling timers and free mydata on RPC_CLNT_DESTROY. RPC SETUP IN CHANGELOG: 1. changelog xlator initiates rpc server say 'changelog_rpc_server' 2. libgfchangelog initiates one rpc server say 'libgfchangelog_rpc_server' 3. libgfchangelog initiates rpc client and connects to 'changelog_rpc_server' 4. In return changelog_rpc_server initiates a rpc client and connects back to 'libgfchangelog_rpc_server' REF/UNREF HANDLING IN TIMER FUNCTIONS: Let's say rpc clnt refcount = 1 1. Take the ref before reigstering callback to timer queue >>>> rpc_clnt_ref (say ref count becomes = 2) 2. Register a callback to timer say 'callback1' 3. If register fails: >>>> rpc_clnt_unref (ref count = 1) 4. On timer expiration, 'callback1' gets called. So unref rpc clnt at the end in 'callback1'. This is corresponding to ref taken in step 1 >>>> rpc_clnt_unref (ref count = 1) 5. The cycle from step-1 to step-4 continues....until timer cancel event happens 6. timer cancel of say 'callback1' If timer cancel fails: Do nothing, Step-4 would have unrefd If timer cancel succeeds: >>>> rpc_clnt_unref (ref count = 1) Change-Id: I91389bc511b8b1a17824941970ee8d2c29a74a09 BUG: 1359363 Signed-off-by: Kotresh HR <> (cherry picked from commit 637ce9e2e27e9f598a4a6c5a04cd339efaa62076) Reviewed-on: CentOS-regression: Gluster Build System <> Smoke: Gluster Build System <> NetBSD-regression: NetBSD Build System <> Reviewed-by: Raghavendra G <>
Diffstat (limited to 'xlators')
4 files changed, 18 insertions, 1 deletions
diff --git a/xlators/features/changelog/src/changelog-ev-handle.c b/xlators/features/changelog/src/changelog-ev-handle.c
index bcce28e..8097d63 100644
--- a/xlators/features/changelog/src/changelog-ev-handle.c
+++ b/xlators/features/changelog/src/changelog-ev-handle.c
@@ -157,6 +157,13 @@ changelog_rpc_notify (struct rpc_clnt *rpc,
rpc_clnt_disable (crpc->rpc);
+ /* rpc_clnt_disable doesn't unref the rpc. It just marks
+ * the rpc as disabled and cancels reconnection timer.
+ * Hence unref the rpc object to free it.
+ */
+ rpc_clnt_unref (crpc->rpc);
selection = &priv->ev_selection;
LOCK (&crpc->lock);
@@ -170,6 +177,8 @@ changelog_rpc_notify (struct rpc_clnt *rpc,
+ /* Free up mydata */
+ changelog_rpc_clnt_unref (crpc);
@@ -251,7 +260,9 @@ get_client (changelog_clnt_t *c_clnt, struct list_head **next)
if (*next == &c_clnt->active)
goto unblock;
crpc = list_entry (*next, changelog_rpc_clnt_t, list);
+ /* ref rpc as DISCONNECT might unref the rpc asynchronously */
changelog_rpc_clnt_ref (crpc);
+ rpc_clnt_ref (crpc->rpc);
*next = (*next)->next;
@@ -265,6 +276,7 @@ put_client (changelog_clnt_t *c_clnt, changelog_rpc_clnt_t *crpc)
LOCK (&c_clnt->active_lock);
+ rpc_clnt_unref (crpc->rpc);
changelog_rpc_clnt_unref (crpc);
UNLOCK (&c_clnt->active_lock);
diff --git a/xlators/features/changelog/src/changelog-rpc.c b/xlators/features/changelog/src/changelog-rpc.c
index 005ac15..69a0db1 100644
--- a/xlators/features/changelog/src/changelog-rpc.c
+++ b/xlators/features/changelog/src/changelog-rpc.c
@@ -199,7 +199,10 @@ changelog_rpc_clnt_init (xlator_t *this,
goto error_return;
INIT_LIST_HEAD (&crpc->list);
- crpc->ref = 0;
+ /* Take a ref, the last unref will be on RPC_CLNT_DESTROY
+ * which comes as a result of last rpc_clnt_unref.
+ */
+ crpc->ref = 1;
changelog_set_disconnect_flag (crpc, _gf_false);
crpc->filter = rpc_req->filter;
diff --git a/xlators/mgmt/glusterd/src/glusterd-rebalance.c b/xlators/mgmt/glusterd/src/glusterd-rebalance.c
index 2781b5d..dc2ac37 100644
--- a/xlators/mgmt/glusterd/src/glusterd-rebalance.c
+++ b/xlators/mgmt/glusterd/src/glusterd-rebalance.c
@@ -149,6 +149,7 @@ __glusterd_defrag_notify (struct rpc_clnt *rpc, void *mydata,
glusterd_store_perform_node_state_store (volinfo);
+ rpc_clnt_reconnect_cleanup (&defrag->rpc->conn);
glusterd_defrag_rpc_put (defrag);
if (defrag->cbk_fn)
defrag->cbk_fn (volinfo,
diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c b/xlators/mgmt/glusterd/src/glusterd-utils.c
index 655cc04..b9145b5 100644
--- a/xlators/mgmt/glusterd/src/glusterd-utils.c
+++ b/xlators/mgmt/glusterd/src/glusterd-utils.c
@@ -10491,6 +10491,7 @@ glusterd_rpc_clnt_unref (glusterd_conf_t *conf, rpc_clnt_t *rpc)
GF_ASSERT (conf);
GF_ASSERT (rpc);
synclock_unlock (&conf->big_lock);
+ (void) rpc_clnt_reconnect_cleanup (&rpc->conn);
ret = rpc_clnt_unref (rpc);
synclock_lock (&conf->big_lock);