clnt/rpc: ref leak during disconnect.

During disconnect cleanup, we are not cancelling reconnect
timer, which causes a ref leak each time when a disconnect
happen.

Change-Id: I9d05d1f368d080e04836bf6a0bb018bf8f7b5b8a
updates: bz#1659708
Signed-off-by: Mohammed Rafi KC <rkavunga@redhat.com>

3 files changed, 47 insertions, 12 deletions

diff --git a/libglusterfs/src/timer.c b/libglusterfs/src/timer.c
index d882543b08b..2643c07820b 100644
--- a/libglusterfs/src/timer.c
+++ b/libglusterfs/src/timer.c
@@ -75,13 +75,13 @@ gf_timer_call_cancel(glusterfs_ctx_t *ctx, gf_timer_t *event)
     if (ctx == NULL || event == NULL) {
         gf_msg_callingfn("timer", GF_LOG_ERROR, EINVAL, LG_MSG_INVALID_ARG,
                          "invalid argument");
-        return 0;
+        return -1;
     }
 
     if (ctx->cleanup_started) {
         gf_msg_callingfn("timer", GF_LOG_INFO, 0, LG_MSG_CTX_CLEANUP_STARTED,
                          "ctx cleanup started");
-        return 0;
+        return -1;
     }
 
     LOCK(&ctx->lock);
@@ -93,10 +93,9 @@ gf_timer_call_cancel(glusterfs_ctx_t *ctx, gf_timer_t *event)
     if (!reg) {
         /* This can happen when cleanup may have just started and
          * gf_timer_registry_destroy() sets ctx->timer to NULL.
-         * Just bail out as success as gf_timer_proc() takes
-         * care of cleaning up the events.
+         * gf_timer_proc() takes care of cleaning up the events.
          */
-        return 0;
+        return -1;
     }
 
     LOCK(&reg->lock);
@@ -203,6 +202,13 @@ gf_timer_proc(void *data)
         list_for_each_entry_safe(event, tmp, &reg->active, list)
         {
             list_del(&event->list);
+            /* TODO Possible resource leak
+             * Before freeing the event, we need to call the respective
+             * event functions and free any resources.
+             * For example, In case of rpc_clnt_reconnect, we need to
+             * unref rpc object which was taken when added to timer
+             * wheel.
+             */
             GF_FREE(event);
         }
     }
diff --git a/rpc/rpc-lib/src/rpc-clnt.c b/rpc/rpc-lib/src/rpc-clnt.c
index 3f7bb3ca513..6f47515a48b 100644
--- a/rpc/rpc-lib/src/rpc-clnt.c
+++ b/rpc/rpc-lib/src/rpc-clnt.c
@@ -495,6 +495,7 @@ rpc_clnt_connection_cleanup(rpc_clnt_connection_t *conn)
     int unref = 0;
     int ret = 0;
     gf_boolean_t timer_unref = _gf_false;
+    gf_boolean_t reconnect_unref = _gf_false;
 
     if (!conn) {
         goto out;
@@ -514,6 +515,12 @@ rpc_clnt_connection_cleanup(rpc_clnt_connection_t *conn)
                 timer_unref = _gf_true;
             conn->timer = NULL;
         }
+        if (conn->reconnect) {
+            ret = gf_timer_call_cancel(clnt->ctx, conn->reconnect);
+            if (!ret)
+                reconnect_unref = _gf_true;
+            conn->reconnect = NULL;
+        }
 
         conn->connected = 0;
         conn->disconnected = 1;
@@ -533,6 +540,8 @@ rpc_clnt_connection_cleanup(rpc_clnt_connection_t *conn)
     if (timer_unref)
         rpc_clnt_unref(clnt);
 
+    if (reconnect_unref)
+        rpc_clnt_unref(clnt);
 out:
     return 0;
 }
@@ -830,7 +839,7 @@ rpc_clnt_handle_disconnect(struct rpc_clnt *clnt, rpc_clnt_connection_t *conn)
     pthread_mutex_lock(&conn->lock);
     {
         if (!conn->rpc_clnt->disabled && (conn->reconnect == NULL)) {
-            ts.tv_sec = 10;
+            ts.tv_sec = 3;
             ts.tv_nsec = 0;
 
             rpc_clnt_ref(clnt);
diff --git a/xlators/mgmt/glusterd/src/glusterd-snapshot-utils.c b/xlators/mgmt/glusterd/src/glusterd-snapshot-utils.c
index 1ece374b2c2..cf17fcbff16 100644
--- a/xlators/mgmt/glusterd/src/glusterd-snapshot-utils.c
+++ b/xlators/mgmt/glusterd/src/glusterd-snapshot-utils.c
@@ -3364,6 +3364,25 @@ out:
     return ret;
 }
 
+int
+glusterd_is_path_mounted(const char *path)
+{
+    FILE *mtab = NULL;
+    struct mntent *part = NULL;
+    int is_mounted = 0;
+
+    if ((mtab = setmntent("/etc/mtab", "r")) != NULL) {
+        while ((part = getmntent(mtab)) != NULL) {
+            if ((part->mnt_fsname != NULL) &&
+                (strcmp(part->mnt_dir, path)) == 0) {
+                is_mounted = 1;
+                break;
+            }
+        }
+        endmntent(mtab);
+    }
+    return is_mounted;
+}
 /* This function will do unmount for snaps.
  */
 int32_t
@@ -3388,14 +3407,11 @@ glusterd_snap_unmount(xlator_t *this, glusterd_volinfo_t *volinfo)
             continue;
         }
 
-        /* Fetch the brick mount path from the brickinfo->path */
-        ret = glusterd_get_brick_root(brickinfo->path, &brick_mount_path);
+        ret = glusterd_find_brick_mount_path(brickinfo->path,
+                                             &brick_mount_path);
         if (ret) {
-            gf_msg(this->name, GF_LOG_INFO, 0, GD_MSG_BRICK_PATH_UNMOUNTED,
+            gf_msg(this->name, GF_LOG_ERROR, 0, GD_MSG_BRK_MNTPATH_GET_FAIL,
                    "Failed to find brick_mount_path for %s", brickinfo->path);
-            /* There is chance that brick path is already
-             * unmounted. */
-            ret = 0;
             goto out;
         }
         /* unmount cannot be done when the brick process is still in
@@ -3440,6 +3456,10 @@ glusterd_umount(const char *path)
     GF_ASSERT(this);
     GF_ASSERT(path);
 
+    if (!glusterd_is_path_mounted(path)) {
+        return 0;
+    }
+
     runinit(&runner);
     snprintf(msg, sizeof(msg), "umount path %s", path);
     runner_add_args(&runner, _PATH_UMOUNT, "-f", path, NULL);