From d89f9b3b57b970bc479c5ef8abe7f18549d0e90b Mon Sep 17 00:00:00 2001
From: Mohammed Rafi KC <rkavunga@redhat.com>
Date: Thu, 6 Jul 2017 13:26:42 +0530
Subject: [PATCH 557/557] mgtm/core : use sha hash function for volfile check
We are storing the entire volfile and using this to check
volfile change. With brick multiplexing there will be lot
of graphs per process which will increase the memory foot
print of the process. So instead of storing the entire
graph we could use sha256 and we can compare the hash to
see whether volfile change happened or not.
Also with Brick multiplexing, the direct comparison of vol
file is not correct. There are two problems.
Problem 1:
We are currently storing one single graph (the last
updated volfile) whereas, what we need is the entire
graph with all atttached bricks.
If we fix this issue, we have second problem
Problem 2:
With multiplexing we have a graph that contains multiple
bricks. But what we are checking as part of the reconfigure
is, comparing the entire graph with one single graph,
which will always fail.
Solution:
We create list in glusterfs_ctx_t that stores sha256 hash
of individual brick graphs. When a graph changes happens
we compare the stored hash and the current hash. If the
hash matches, then no need for reconfigure. Otherwise we
first do the reconfigure and then update the hash.
For now, gfapi has not changed this way. Meaning when gfapi
volfile fetch or reconfigure happens, we still store the
entire graph and compare, each memory.
This is fine, because libgfapi will not load brick graphs.
But changing the libgfapi will make the code similar in
both glusterfsd-mgmt and api. Also it helps to reduce some
memory.
>Change-Id: I9df917a771a52b95622ab8f63af34ec390163a77
>BUG: 1467986
>Signed-off-by: Mohammed Rafi KC <rkavunga@redhat.com>
>Reviewed-on: https://review.gluster.org/17709
>Smoke: Gluster Build System <jenkins@build.gluster.org>
>Reviewed-by: Pranith Kumar Karampuri <pkarampu@redhat.com>
>CentOS-regression: Gluster Build System <jenkins@build.gluster.org>
>Reviewed-by: Atin Mukherjee <amukherj@redhat.com>
>Reviewed-by: Amar Tumballi <amarts@redhat.com>
Change-Id: I9df917a771a52b95622ab8f63af34ec390163a77
BUG: 1457936
Signed-off-by: Mohammed Rafi KC <rkavunga@redhat.com>
Signed-off-by: Pranith Kumar Karampuri <pkarampu@redhat.com>
Reviewed-on: https://code.engineering.redhat.com/gerrit/111802
Reviewed-by: Atin Mukherjee <amukherj@redhat.com>
---
api/src/glfs-mgmt.c | 4 +-
glusterfsd/src/glusterfsd-mgmt.c | 98 +++++++++++++++--------
libglusterfs/src/common-utils.c | 12 +++
libglusterfs/src/common-utils.h | 5 ++
libglusterfs/src/ctx.c | 1 +
libglusterfs/src/glusterfs.h | 9 +++
libglusterfs/src/graph.c | 145 ++++++++++++++++++++++++++++++++---
libglusterfs/src/mem-types.h | 1 +
libglusterfs/src/xlator.c | 26 +++++++
libglusterfs/src/xlator.h | 11 ++-
xlators/protocol/server/src/server.c | 3 +
11 files changed, 268 insertions(+), 47 deletions(-)
diff --git a/api/src/glfs-mgmt.c b/api/src/glfs-mgmt.c
index 0109764..0b60092 100644
--- a/api/src/glfs-mgmt.c
+++ b/api/src/glfs-mgmt.c
@@ -648,8 +648,8 @@ glfs_mgmt_getspec_cbk (struct rpc_req *req, struct iovec *iov, int count,
* return -1(or -ve) =======> Some Internal Error occurred during the operation
*/
- ret = glusterfs_volfile_reconfigure (fs->oldvollen, tmpfp, fs->ctx,
- fs->oldvolfile);
+ ret = gf_volfile_reconfigure (fs->oldvollen, tmpfp, fs->ctx,
+ fs->oldvolfile);
if (ret == 0) {
gf_msg_debug ("glusterfsd-mgmt", 0, "No need to re-load "
"volfile, reconfigure done");
diff --git a/glusterfsd/src/glusterfsd-mgmt.c b/glusterfsd/src/glusterfsd-mgmt.c
index 365706e..bde49ba 100644
--- a/glusterfsd/src/glusterfsd-mgmt.c
+++ b/glusterfsd/src/glusterfsd-mgmt.c
@@ -1763,12 +1763,6 @@ out:
return ret;
}
-
-/* XXX: move these into @ctx */
-static char *oldvolfile = NULL;
-static int oldvollen;
-
-
int
mgmt_getspec_cbk (struct rpc_req *req, struct iovec *iov, int count,
void *myframe)
@@ -1779,7 +1773,10 @@ mgmt_getspec_cbk (struct rpc_req *req, struct iovec *iov, int count,
int ret = 0, locked = 0;
ssize_t size = 0;
FILE *tmpfp = NULL;
- char *volfilebuf = NULL;
+ char *volfile_id = NULL;
+ gf_volfile_t *volfile_obj = NULL;
+ gf_volfile_t *volfile_tmp = NULL;
+ char sha256_hash[SHA256_DIGEST_LENGTH] = {0, };
frame = myframe;
ctx = frame->this->ctx;
@@ -1806,14 +1803,29 @@ mgmt_getspec_cbk (struct rpc_req *req, struct iovec *iov, int count,
ret = 0;
size = rsp.op_ret;
+ glusterfs_compute_sha256 ((const unsigned char *) rsp.spec, size,
+ sha256_hash);
+
+ volfile_id = frame->local;
+
LOCK (&ctx->volfile_lock);
{
locked = 1;
- if (size == oldvollen && (memcmp (oldvolfile, rsp.spec, size) == 0)) {
- gf_log (frame->this->name, GF_LOG_INFO,
- "No change in volfile, continuing");
- goto out;
+ list_for_each_entry (volfile_obj, &ctx->volfile_list,
+ volfile_list) {
+ if (!strcmp (volfile_id, volfile_obj->vol_id)) {
+ if (!strncmp (sha256_hash,
+ volfile_obj->volfile_checksum,
+ sizeof (volfile_obj->volfile_checksum))) {
+ gf_log (frame->this->name, GF_LOG_INFO,
+ "No change in volfile,"
+ "continuing");
+ goto out;
+ }
+ volfile_tmp = volfile_obj;
+ break;
+ }
}
tmpfp = tmpfile ();
@@ -1837,21 +1849,19 @@ mgmt_getspec_cbk (struct rpc_req *req, struct iovec *iov, int count,
* return -1(or -ve) =======> Some Internal Error occurred during the operation
*/
- ret = glusterfs_volfile_reconfigure (oldvollen, tmpfp, ctx, oldvolfile);
+ ret = glusterfs_volfile_reconfigure (tmpfp, ctx);
if (ret == 0) {
gf_log ("glusterfsd-mgmt", GF_LOG_DEBUG,
"No need to re-load volfile, reconfigure done");
- if (oldvolfile)
- volfilebuf = GF_REALLOC (oldvolfile, size);
- else
- volfilebuf = GF_CALLOC (1, size, gf_common_mt_char);
- if (!volfilebuf) {
+ if (!volfile_tmp) {
ret = -1;
+ gf_log ("mgmt", GF_LOG_ERROR, "Graph "
+ "reconfigure succeeded with out having "
+ "checksum.");
goto out;
}
- oldvolfile = volfilebuf;
- oldvollen = size;
- memcpy (oldvolfile, rsp.spec, size);
+ strncpy (volfile_tmp->volfile_checksum, sha256_hash,
+ sizeof (volfile_tmp->volfile_checksum));
goto out;
}
@@ -1867,19 +1877,23 @@ mgmt_getspec_cbk (struct rpc_req *req, struct iovec *iov, int count,
if (ret)
goto out;
- if (oldvolfile)
- volfilebuf = GF_REALLOC (oldvolfile, size);
- else
- volfilebuf = GF_CALLOC (1, size, gf_common_mt_char);
+ if (!volfile_tmp) {
+ volfile_tmp = GF_CALLOC (1, sizeof (gf_volfile_t),
+ gf_common_volfile_t);
+ if (!volfile_tmp) {
+ ret = -1;
+ goto out;
+ }
- if (!volfilebuf) {
- ret = -1;
- goto out;
+ INIT_LIST_HEAD (&volfile_tmp->volfile_list);
+ list_add (&volfile_tmp->volfile_list,
+ &ctx->volfile_list);
+ snprintf (volfile_tmp->vol_id,
+ sizeof (volfile_tmp->vol_id), "%s",
+ volfile_id);
}
-
- oldvolfile = volfilebuf;
- oldvollen = size;
- memcpy (oldvolfile, rsp.spec, size);
+ strncpy (volfile_tmp->volfile_checksum, sha256_hash,
+ sizeof (volfile_tmp->volfile_checksum));
}
UNLOCK (&ctx->volfile_lock);
@@ -1896,7 +1910,11 @@ out:
if (locked)
UNLOCK (&ctx->volfile_lock);
- STACK_DESTROY (frame->root);
+ if (frame) {
+ GF_FREE (frame->local);
+ frame->local = NULL;
+ STACK_DESTROY (frame->root);
+ }
free (rsp.spec);
@@ -1943,6 +1961,15 @@ glusterfs_volfile_fetch_one (glusterfs_ctx_t *ctx, char *volfile_id)
req.key = volfile_id;
req.flags = 0;
+ /*
+ * We are only storing one variable in local, hence using the same
+ * variable. If multiple local variable is required, create a struct.
+ */
+ frame->local = gf_strdup (volfile_id);
+ if (!frame->local) {
+ ret = -1;
+ goto out;
+ }
dict = dict_new ();
if (!dict) {
@@ -1992,6 +2019,13 @@ out:
GF_FREE (req.xdata.xdata_val);
if (dict)
dict_unref (dict);
+ if (ret && frame) {
+ /* Free the frame->local fast, because we have not used memget
+ */
+ GF_FREE (frame->local);
+ frame->local = NULL;
+ STACK_DESTROY (frame->root);
+ }
return ret;
}
diff --git a/libglusterfs/src/common-utils.c b/libglusterfs/src/common-utils.c
index c123f70..6c02039 100644
--- a/libglusterfs/src/common-utils.c
+++ b/libglusterfs/src/common-utils.c
@@ -4926,3 +4926,15 @@ gf_getgrouplist (const char *user, gid_t group, gid_t **groups)
}
return ret;
}
+
+int
+glusterfs_compute_sha256 (const unsigned char *content, size_t size,
+ char *sha256_hash) {
+ SHA256_CTX sha256;
+
+ SHA256_Init (&sha256);
+ SHA256_Update (&sha256, (const unsigned char *) (content), size);
+ SHA256_Final ((unsigned char *) sha256_hash, &sha256);
+
+ return 0;
+}
diff --git a/libglusterfs/src/common-utils.h b/libglusterfs/src/common-utils.h
index 4765b5e..89d71a6 100644
--- a/libglusterfs/src/common-utils.h
+++ b/libglusterfs/src/common-utils.h
@@ -906,4 +906,9 @@ close_fds_except (int *fdv, size_t count);
int
gf_getgrouplist (const char *user, gid_t group, gid_t **groups);
+
+int
+glusterfs_compute_sha256 (const unsigned char *content, size_t size,
+ char *sha256_hash);
+
#endif /* _COMMON_UTILS_H */
diff --git a/libglusterfs/src/ctx.c b/libglusterfs/src/ctx.c
index 1f2f1a1..35f1928 100644
--- a/libglusterfs/src/ctx.c
+++ b/libglusterfs/src/ctx.c
@@ -32,6 +32,7 @@ glusterfs_ctx_new ()
INIT_LIST_HEAD (&ctx->graphs);
INIT_LIST_HEAD (&ctx->mempool_list);
+ INIT_LIST_HEAD (&ctx->volfile_list);
ctx->daemon_pipe[0] = -1;
ctx->daemon_pipe[1] = -1;
diff --git a/libglusterfs/src/glusterfs.h b/libglusterfs/src/glusterfs.h
index 43acf4c..c58cae1 100644
--- a/libglusterfs/src/glusterfs.h
+++ b/libglusterfs/src/glusterfs.h
@@ -28,6 +28,7 @@
#include <sys/poll.h>
#include <pthread.h>
#include <limits.h> /* For PATH_MAX */
+#include <openssl/sha.h>
#include "glusterfs-fops.h" /* generated XDR values for FOPs */
@@ -513,9 +514,17 @@ struct _glusterfs_ctx {
struct gf_ctx_tw *tw; /* refcounted timer_wheel */
gf_lock_t volfile_lock;
+ struct list_head volfile_list;
};
typedef struct _glusterfs_ctx glusterfs_ctx_t;
+typedef struct {
+ char volfile_checksum[SHA256_DIGEST_LENGTH];
+ char vol_id[NAME_MAX+1];
+ struct list_head volfile_list;
+
+} gf_volfile_t;
+
glusterfs_ctx_t *glusterfs_ctx_new (void);
struct gf_flock {
diff --git a/libglusterfs/src/graph.c b/libglusterfs/src/graph.c
index 4c6321e..126f318 100644
--- a/libglusterfs/src/graph.c
+++ b/libglusterfs/src/graph.c
@@ -814,8 +814,70 @@ out:
* return -1(or -ve) =======> Some Internal Error occurred during the operation
*/
int
-glusterfs_volfile_reconfigure (int oldvollen, FILE *newvolfile_fp,
- glusterfs_ctx_t *ctx, const char *oldvolfile)
+glusterfs_volfile_reconfigure (FILE *newvolfile_fp, glusterfs_ctx_t *ctx)
+{
+ glusterfs_graph_t *oldvolfile_graph = NULL;
+ glusterfs_graph_t *newvolfile_graph = NULL;
+
+ int ret = -1;
+
+ if (!ctx) {
+ gf_msg ("glusterfsd-mgmt", GF_LOG_ERROR, 0, LG_MSG_CTX_NULL,
+ "ctx is NULL");
+ goto out;
+ }
+
+ oldvolfile_graph = ctx->active;
+ if (!oldvolfile_graph) {
+ ret = 1;
+ goto out;
+ }
+
+ newvolfile_graph = glusterfs_graph_construct (newvolfile_fp);
+
+ if (!newvolfile_graph) {
+ goto out;
+ }
+
+ glusterfs_graph_prepare (newvolfile_graph, ctx,
+ ctx->cmd_args.volume_name);
+
+ if (!is_graph_topology_equal (oldvolfile_graph,
+ newvolfile_graph)) {
+
+ ret = 1;
+ gf_msg_debug ("glusterfsd-mgmt", 0, "Graph topology not "
+ "equal(should call INIT)");
+ goto out;
+ }
+
+ gf_msg_debug ("glusterfsd-mgmt", 0, "Only options have changed in the"
+ " new graph");
+
+ ret = glusterfs_graph_reconfigure (oldvolfile_graph,
+ newvolfile_graph);
+ if (ret) {
+ gf_msg_debug ("glusterfsd-mgmt", 0, "Could not reconfigure "
+ "new options in old graph");
+ goto out;
+ }
+
+ ret = 0;
+out:
+
+ if (newvolfile_graph)
+ glusterfs_graph_destroy (newvolfile_graph);
+
+ return ret;
+}
+
+/* This function need to remove. This added to support gfapi volfile
+ * reconfigure.
+ */
+
+int
+gf_volfile_reconfigure (int oldvollen, FILE *newvolfile_fp,
+ glusterfs_ctx_t *ctx, const char *oldvolfile)
{
glusterfs_graph_t *oldvolfile_graph = NULL;
glusterfs_graph_t *newvolfile_graph = NULL;
@@ -837,9 +899,9 @@ glusterfs_volfile_reconfigure (int oldvollen, FILE *newvolfile_fp,
if (!ctx) {
gf_msg ("glusterfsd-mgmt", GF_LOG_ERROR, 0, LG_MSG_CTX_NULL,
- "ctx is NULL");
- goto out;
- }
+ "ctx is NULL");
+ goto out;
+ }
oldvolfile_graph = ctx->active;
if (!oldvolfile_graph) {
@@ -890,7 +952,7 @@ glusterfs_volfile_reconfigure (int oldvollen, FILE *newvolfile_fp,
goto out;
}
- glusterfs_graph_prepare (newvolfile_graph, ctx,
+ glusterfs_graph_prepare (newvolfile_graph, ctx,
ctx->cmd_args.volume_name);
if (!is_graph_topology_equal (oldvolfile_graph,
@@ -934,7 +996,6 @@ out:
return ret;
}
-
int
glusterfs_graph_reconfigure (glusterfs_graph_t *oldgraph,
glusterfs_graph_t *newgraph)
@@ -1038,19 +1099,56 @@ glusterfs_graph_attach (glusterfs_graph_t *orig_graph, char *path,
FILE *fp;
glusterfs_graph_t *graph;
xlator_t *xl;
- char *volfile_id;
+ char *volfile_id = NULL;
+ char *volfile_content = NULL;
+ struct stat stbuf = {0,};
+ size_t file_len = -1;
+ gf_volfile_t *volfile_obj = NULL;
+ int ret = -1;
+ char sha256_hash[SHA256_DIGEST_LENGTH] = {0, };
if (!orig_graph) {
return -EINVAL;
}
+ ret = sys_stat (path, &stbuf);
+ if (ret < 0) {
+ gf_log (THIS->name, GF_LOG_ERROR, "Unable to stat %s (%s)",
+ path, strerror (errno));
+ return -EINVAL;
+ }
+
+ file_len = stbuf.st_size;
+ if (file_len) {
+ volfile_content = GF_CALLOC (file_len+1, sizeof (char),
+ gf_common_mt_char);
+ if (!volfile_content) {
+ return -ENOMEM;
+ }
+ }
+
fp = fopen (path, "r");
if (!fp) {
gf_log (THIS->name, GF_LOG_WARNING,
"oops, %s disappeared on us", path);
+ GF_FREE (volfile_content);
+ return -EIO;
+ }
+
+ ret = fread (volfile_content, sizeof (char), file_len, fp);
+ if (ret == file_len) {
+ glusterfs_compute_sha256 ((const unsigned char *) volfile_content,
+ file_len, sha256_hash);
+ } else {
+ gf_log (THIS->name, GF_LOG_ERROR,
+ "read failed on path %s. File size=%"GF_PRI_SIZET
+ "read size=%d", path, file_len, ret);
+ GF_FREE (volfile_content);
return -EIO;
}
+ GF_FREE (volfile_content);
+
graph = glusterfs_graph_construct (fp);
fclose(fp);
if (!graph) {
@@ -1085,9 +1183,34 @@ glusterfs_graph_attach (glusterfs_graph_t *orig_graph, char *path,
}
/* TBD: memory leaks everywhere */
- glusterfs_graph_prepare (graph, this->ctx, xl->name);
- glusterfs_graph_init (graph);
- glusterfs_xlator_link (orig_graph->top, graph->top);
+ if (glusterfs_graph_prepare (graph, this->ctx, xl->name)) {
+ gf_log (this->name, GF_LOG_WARNING,
+ "failed to prepare graph for xlator %s", xl->name);
+ return -EIO;
+ } else if (glusterfs_graph_init (graph)) {
+ gf_log (this->name, GF_LOG_WARNING,
+ "failed to initialize graph for xlator %s", xl->name);
+ return -EIO;
+ } else if (glusterfs_xlator_link (orig_graph->top, graph->top)) {
+ gf_log (this->name, GF_LOG_WARNING,
+ "failed to link the graphs for xlator %s ", xl->name);
+ return -EIO;
+ }
+
+ if (!volfile_obj) {
+ volfile_obj = GF_CALLOC (1, sizeof (gf_volfile_t),
+ gf_common_volfile_t);
+ if (!volfile_obj) {
+ return -EIO;
+ }
+ }
+
+ INIT_LIST_HEAD (&volfile_obj->volfile_list);
+ snprintf (volfile_obj->vol_id, sizeof (volfile_obj->vol_id),
+ "%s", xl->volfile_id);
+ strncpy (volfile_obj->volfile_checksum, sha256_hash,
+ sizeof (volfile_obj->volfile_checksum));
+ list_add (&volfile_obj->volfile_list, &this->ctx->volfile_list);
return 0;
}
diff --git a/libglusterfs/src/mem-types.h b/libglusterfs/src/mem-types.h
index ef7dcd0..ac3f878 100644
--- a/libglusterfs/src/mem-types.h
+++ b/libglusterfs/src/mem-types.h
@@ -169,6 +169,7 @@ enum gf_common_mem_types_ {
/*lock migration*/
gf_common_mt_lock_mig,
gf_common_mt_pthread_t,
+ gf_common_volfile_t,
gf_common_mt_end
};
#endif
diff --git a/libglusterfs/src/xlator.c b/libglusterfs/src/xlator.c
index dc5cfaa..ec710c6 100644
--- a/libglusterfs/src/xlator.c
+++ b/libglusterfs/src/xlator.c
@@ -1178,3 +1178,29 @@ copy_opts_to_child (xlator_t *src, xlator_t *dst, char *glob)
return dict_foreach_fnmatch (src->options, glob,
_copy_opt_to_child, dst);
}
+
+int
+glusterfs_delete_volfile_checksum (glusterfs_ctx_t *ctx,
+ const char *volfile_id) {
+
+ gf_volfile_t *volfile_tmp = NULL;
+ gf_volfile_t *volfile_obj = NULL;
+
+ list_for_each_entry (volfile_tmp, &ctx->volfile_list,
+ volfile_list) {
+ if (!strcmp (volfile_id, volfile_tmp->vol_id)) {
+ list_del_init (&volfile_tmp->volfile_list);
+ volfile_obj = volfile_tmp;
+ break;
+ }
+ }
+
+ if (volfile_obj) {
+ GF_FREE (volfile_obj);
+ } else {
+ gf_log (THIS->name, GF_LOG_ERROR, "failed to get volfile "
+ "checksum for volfile id %s.", volfile_id);
+ }
+
+ return 0;
+}
diff --git a/libglusterfs/src/xlator.h b/libglusterfs/src/xlator.h
index 508146f..d94045d 100644
--- a/libglusterfs/src/xlator.h
+++ b/libglusterfs/src/xlator.h
@@ -1039,8 +1039,11 @@ enum gf_hdsk_event_notify_op {
gf_boolean_t
is_graph_topology_equal (glusterfs_graph_t *graph1, glusterfs_graph_t *graph2);
int
-glusterfs_volfile_reconfigure (int oldvollen, FILE *newvolfile_fp,
- glusterfs_ctx_t *ctx, const char *oldvolfile);
+glusterfs_volfile_reconfigure (FILE *newvolfile_fp, glusterfs_ctx_t *ctx);
+
+int
+gf_volfile_reconfigure (int oldvollen, FILE *newvolfile_fp,
+ glusterfs_ctx_t *ctx, const char *oldvolfile);
int
loc_touchup (loc_t *loc, const char *name);
@@ -1057,4 +1060,8 @@ xlator_subvolume_count (xlator_t *this);
int
copy_opts_to_child (xlator_t *src, xlator_t *dst, char *glob);
+int
+glusterfs_delete_volfile_checksum (glusterfs_ctx_t *ctx,
+ const char *volfile_id);
+
#endif /* _XLATOR_H */
diff --git a/xlators/protocol/server/src/server.c b/xlators/protocol/server/src/server.c
index 5d4d860..b3dc417 100644
--- a/xlators/protocol/server/src/server.c
+++ b/xlators/protocol/server/src/server.c
@@ -1575,6 +1575,9 @@ notify (xlator_t *this, int32_t event, void *data, ...)
break;
}
}
+ if (victim_found)
+ glusterfs_delete_volfile_checksum (ctx,
+ victim->volfile_id);
UNLOCK (&ctx->volfile_lock);
if (victim_found)
(*trav_p) = (*trav_p)->next;
--
1.8.3.1