From 51090a4b3cb000d601083f12d1875547819fc03f Mon Sep 17 00:00:00 2001

From: Mohit Agrawal <moagrawal@redhat.com>

Date: Wed, 4 Mar 2020 09:17:26 +0530

Subject: [PATCH 447/449] core[brick_mux]: brick crashed when creating and

 deleting volumes over time

Problem: In brick_mux environment, while volumes are created/stopped in a loop

         after running a long time the main brick is crashed.The brick is crashed

         because the main brick process was not cleaned up memory for all objects

         at the time of detaching a volume.

         Below are the objects that are missed at the time of detaching a volume

         1) xlator object for a brick graph

         2) local_pool for posix_lock xlator

         3) rpc object cleanup at quota xlator

         4) inode leak at brick xlator

Solution: To avoid the crash resolve all leak at the time of detaching a brick

> Change-Id: Ibb6e46c5fba22b9441a88cbaf6b3278823235913

> updates: #977

> Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>

> (Cherry pick from commit e589d8de66d3325da8fbbbe44d1a5bd6335e08ab)

> (Reviewed on upstream link https://review.gluster.org/#/c/glusterfs/+/24209/)

BUG: 1790336

Change-Id: Ibb6e46c5fba22b9441a88cbaf6b3278823235913

Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>

Reviewed-on: https://code.engineering.redhat.com/gerrit/202782

Tested-by: RHGS Build Bot <nigelb@redhat.com>

Reviewed-by: Xavi Hernandez Juan <xhernandez@redhat.com>

---

 libglusterfs/src/glusterfs/glusterfs.h             |  1 +

 libglusterfs/src/graph.c                           |  1 +

 libglusterfs/src/graph.y                           |  2 +-

 libglusterfs/src/xlator.c                          | 29 ++++++++----

 xlators/features/changelog/src/changelog.c         |  1 +

 xlators/features/locks/src/posix.c                 |  4 ++

 xlators/features/quota/src/quota-enforcer-client.c | 14 +++++-

 xlators/features/quota/src/quota.c                 | 54 ++++++++++++++++++++--

 xlators/features/quota/src/quota.h                 |  3 ++

 xlators/protocol/server/src/server.c               | 12 +++--

 10 files changed, 103 insertions(+), 18 deletions(-)

diff --git a/libglusterfs/src/glusterfs/glusterfs.h b/libglusterfs/src/glusterfs/glusterfs.h

index 177a020..584846e 100644

--- a/libglusterfs/src/glusterfs/glusterfs.h

+++ b/libglusterfs/src/glusterfs/glusterfs.h

@@ -603,6 +603,7 @@ struct _glusterfs_graph {

     int used; /* Should be set when fuse gets

                         first CHILD_UP */

     uint32_t volfile_checksum;

+    pthread_mutex_t mutex;

 };

 typedef struct _glusterfs_graph glusterfs_graph_t;

diff --git a/libglusterfs/src/graph.c b/libglusterfs/src/graph.c

index bb5e67a..1cd92db 100644

--- a/libglusterfs/src/graph.c

+++ b/libglusterfs/src/graph.c

@@ -1092,6 +1092,7 @@ glusterfs_graph_destroy_residual(glusterfs_graph_t *graph)

     ret = xlator_tree_free_memacct(graph->first);

     list_del_init(&graph->list);

+    pthread_mutex_destroy(&graph->mutex);

     GF_FREE(graph);

     return ret;

diff --git a/libglusterfs/src/graph.y b/libglusterfs/src/graph.y

index 5b92985..5733515 100644

--- a/libglusterfs/src/graph.y

+++ b/libglusterfs/src/graph.y

@@ -541,7 +541,7 @@ glusterfs_graph_new ()

                 return NULL;

         INIT_LIST_HEAD (&graph->list);

-

+        pthread_mutex_init(&graph->mutex, NULL);

         gettimeofday (&graph->dob, NULL);

         return graph;

diff --git a/libglusterfs/src/xlator.c b/libglusterfs/src/xlator.c

index 108b96a..36cc32c 100644

--- a/libglusterfs/src/xlator.c

+++ b/libglusterfs/src/xlator.c

@@ -938,6 +938,8 @@ xlator_mem_cleanup(xlator_t *this)

     xlator_list_t **trav_p = NULL;

     xlator_t *top = NULL;

     xlator_t *victim = NULL;

+    glusterfs_graph_t *graph = NULL;

+    gf_boolean_t graph_cleanup = _gf_false;

     if (this->call_cleanup || !this->ctx)

         return;

@@ -945,6 +947,12 @@ xlator_mem_cleanup(xlator_t *this)

     this->call_cleanup = 1;

     ctx = this->ctx;

+    inode_table = this->itable;

+    if (inode_table) {

+        inode_table_destroy(inode_table);

+        this->itable = NULL;

+    }

+

     xlator_call_fini(trav);

     while (prev) {

@@ -953,12 +961,6 @@ xlator_mem_cleanup(xlator_t *this)

         prev = trav;

     }

-    inode_table = this->itable;

-    if (inode_table) {

-        inode_table_destroy(inode_table);

-        this->itable = NULL;

-    }

-

     if (this->fini) {

         this->fini(this);

     }

@@ -968,17 +970,28 @@ xlator_mem_cleanup(xlator_t *this)

     if (ctx->active) {

         top = 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)) {

+                graph_cleanup = _gf_true;

                 (*trav_p) = (*trav_p)->next;

                 break;

             }

         }

         UNLOCK(&ctx->volfile_lock);

     }

+

+    if (graph_cleanup) {

+        prev = this;

+        graph = ctx->active;

+        pthread_mutex_lock(&graph->mutex);

+        while (prev) {

+            trav = prev->next;

+            GF_FREE(prev);

+            prev = trav;

+        }

+        pthread_mutex_unlock(&graph->mutex);

+    }

 }

 void

diff --git a/xlators/features/changelog/src/changelog.c b/xlators/features/changelog/src/changelog.c

index ff06c09..b54112c 100644

--- a/xlators/features/changelog/src/changelog.c

+++ b/xlators/features/changelog/src/changelog.c

@@ -2872,6 +2872,7 @@ fini(xlator_t *this)

         if (priv->active || priv->rpc_active) {

             /* terminate RPC server/threads */

             changelog_cleanup_rpc(this, priv);

+            GF_FREE(priv->ev_dispatcher);

         }

         /* call barrier_disable to cancel timer */

         if (priv->barrier_enabled)

diff --git a/xlators/features/locks/src/posix.c b/xlators/features/locks/src/posix.c

index 9a14c64..50f1265 100644

--- a/xlators/features/locks/src/posix.c

+++ b/xlators/features/locks/src/posix.c

@@ -4102,6 +4102,10 @@ fini(xlator_t *this)

     if (!priv)

         return;

     this->private = NULL;

+    if (this->local_pool) {

+        mem_pool_destroy(this->local_pool);

+        this->local_pool = NULL;

+    }

     GF_FREE(priv->brickname);

     GF_FREE(priv);

diff --git a/xlators/features/quota/src/quota-enforcer-client.c b/xlators/features/quota/src/quota-enforcer-client.c

index 1a4c2e3..097439d 100644

--- a/xlators/features/quota/src/quota-enforcer-client.c

+++ b/xlators/features/quota/src/quota-enforcer-client.c

@@ -362,16 +362,28 @@ quota_enforcer_notify(struct rpc_clnt *rpc, void *mydata,

 {

     xlator_t *this = NULL;

     int ret = 0;

+    quota_priv_t *priv = NULL;

     this = mydata;

-

+    priv = this->private;

     switch (event) {

         case RPC_CLNT_CONNECT: {

+            pthread_mutex_lock(&priv->conn_mutex);

+            {

+                priv->conn_status = _gf_true;

+            }

+            pthread_mutex_unlock(&priv->conn_mutex);

             gf_msg_trace(this->name, 0, "got RPC_CLNT_CONNECT");

             break;

         }

         case RPC_CLNT_DISCONNECT: {

+            pthread_mutex_lock(&priv->conn_mutex);

+            {

+                priv->conn_status = _gf_false;

+                pthread_cond_signal(&priv->conn_cond);

+            }

+            pthread_mutex_unlock(&priv->conn_mutex);

             gf_msg_trace(this->name, 0, "got RPC_CLNT_DISCONNECT");

             break;

         }

diff --git a/xlators/features/quota/src/quota.c b/xlators/features/quota/src/quota.c

index a0c236d..d1123ce 100644

--- a/xlators/features/quota/src/quota.c

+++ b/xlators/features/quota/src/quota.c

@@ -5014,6 +5014,43 @@ quota_forget(xlator_t *this, inode_t *inode)

     return 0;

 }

+int

+notify(xlator_t *this, int event, void *data, ...)

+{

+    quota_priv_t *priv = NULL;

+    int ret = 0;

+    rpc_clnt_t *rpc = NULL;

+    gf_boolean_t conn_status = _gf_true;

+    xlator_t *victim = data;

+

+    priv = this->private;

+    if (!priv || !priv->is_quota_on)

+        goto out;

+

+    if (event == GF_EVENT_PARENT_DOWN) {

+        rpc = priv->rpc_clnt;

+        if (rpc) {

+            rpc_clnt_disable(rpc);

+            pthread_mutex_lock(&priv->conn_mutex);

+            {

+                conn_status = priv->conn_status;

+                while (conn_status) {

+                    (void)pthread_cond_wait(&priv->conn_cond,

+                                            &priv->conn_mutex);

+                    conn_status = priv->conn_status;

+                }

+            }

+            pthread_mutex_unlock(&priv->conn_mutex);

+            gf_log(this->name, GF_LOG_INFO,

+                   "Notify GF_EVENT_PARENT_DOWN for brick %s", victim->name);

+        }

+    }

+

+out:

+    ret = default_notify(this, event, data);

+    return ret;

+}

+

 int32_t

 init(xlator_t *this)

 {

@@ -5056,6 +5093,10 @@ init(xlator_t *this)

         goto err;

     }

+    pthread_mutex_init(&priv->conn_mutex, NULL);

+    pthread_cond_init(&priv->conn_cond, NULL);

+    priv->conn_status = _gf_false;

+

     if (priv->is_quota_on) {

         rpc = quota_enforcer_init(this, this->options);

         if (rpc == NULL) {

@@ -5169,20 +5210,22 @@ fini(xlator_t *this)

 {

     quota_priv_t *priv = NULL;

     rpc_clnt_t *rpc = NULL;

-    int i = 0, cnt = 0;

     priv = this->private;

     if (!priv)

         return;

     rpc = priv->rpc_clnt;

     priv->rpc_clnt = NULL;

-    this->private = NULL;

     if (rpc) {

-        cnt = GF_ATOMIC_GET(rpc->refcount);

-        for (i = 0; i < cnt; i++)

-            rpc_clnt_unref(rpc);

+        rpc_clnt_connection_cleanup(&rpc->conn);

+        rpc_clnt_unref(rpc);

     }

+

+    this->private = NULL;

     LOCK_DESTROY(&priv->lock);

+    pthread_mutex_destroy(&priv->conn_mutex);

+    pthread_cond_destroy(&priv->conn_cond);

+

     GF_FREE(priv);

     if (this->local_pool) {

         mem_pool_destroy(this->local_pool);

@@ -5314,6 +5357,7 @@ struct volume_options options[] = {

 xlator_api_t xlator_api = {

     .init = init,

     .fini = fini,

+    .notify = notify,

     .reconfigure = reconfigure,

     .mem_acct_init = mem_acct_init,

     .op_version = {1}, /* Present from the initial version */

diff --git a/xlators/features/quota/src/quota.h b/xlators/features/quota/src/quota.h

index a5a99ca..e51ffd4 100644

--- a/xlators/features/quota/src/quota.h

+++ b/xlators/features/quota/src/quota.h

@@ -217,6 +217,9 @@ struct quota_priv {

     char *volume_uuid;

     uint64_t validation_count;

     int32_t quotad_conn_status;

+    pthread_mutex_t conn_mutex;

+    pthread_cond_t conn_cond;

+    gf_boolean_t conn_status;

 };

 typedef struct quota_priv quota_priv_t;

diff --git a/xlators/protocol/server/src/server.c b/xlators/protocol/server/src/server.c

index a5f09fe..54d9c0f 100644

--- a/xlators/protocol/server/src/server.c

+++ b/xlators/protocol/server/src/server.c

@@ -409,7 +409,13 @@ server_call_xlator_mem_cleanup(xlator_t *this, char *victim_name)

     arg = calloc(1, sizeof(*arg));

     arg->this = this;

-    arg->victim_name = gf_strdup(victim_name);

+    arg->victim_name = strdup(victim_name);

+    if (!arg->victim_name) {

+        gf_smsg(this->name, GF_LOG_CRITICAL, ENOMEM, LG_MSG_NO_MEMORY,

+                "Memory allocation is failed");

+        return;

+    }

+

     th_ret = gf_thread_create_detached(&th_id, server_graph_janitor_threads,

                                        arg, "graphjanitor");

     if (th_ret) {

@@ -417,7 +423,7 @@ server_call_xlator_mem_cleanup(xlator_t *this, char *victim_name)

                "graph janitor Thread"

                " creation is failed for brick %s",

                victim_name);

-        GF_FREE(arg->victim_name);

+        free(arg->victim_name);

         free(arg);

     }

 }

@@ -628,7 +634,7 @@ server_graph_janitor_threads(void *data)

     }

 out:

-    GF_FREE(arg->victim_name);

+    free(arg->victim_name);

     free(arg);

     return NULL;

 }

-- 

1.8.3.1