From 3d0953aa99eed434a1977de3131b264c48fca64b Mon Sep 17 00:00:00 2001 From: Prasanna Kumar Kalever Date: Mon, 18 Sep 2017 19:49:10 +0530 Subject: rpc: switch to MT-safe block RPC routines blockResponse * block_delete_1(blockDelete *argp, CLIENT *clnt) { static blockResponse clnt_res; <<<<<<-------- Same memory is used by everyone memset((char *)&clnt_res, 0, sizeof(clnt_res)); <<<<<---- Here memset is happening if (clnt_call (clnt, BLOCK_DELETE, (xdrproc_t) xdr_blockDelete, (caddr_t) argp, (xdrproc_t) xdr_blockResponse, (caddr_t) &clnt_res, TIMEOUT) != RPC_SUCCESS) { return (NULL); } return (&clnt_res); <<<<<---- ptr to this memory is returned. } So while Thread-1 is returned "return (&clnt_res);" another thread could be doing "memset((char *)&clnt_res, 0, sizeof(clnt_res));" This seem to be a day-1 gluster-blockd bug from the looks of it. Change-Id: I3fc76d7814c4fe5b286577586ec44d752dcc73f0 Signed-off-by: Prasanna Kumar Kalever Signed-off-by: Pranith Kumar K --- cli/gluster-block.c | 28 ++++------ rpc/block_svc_routines.c | 143 +++++++++++++++++++++++++++++++++++++++-------- rpc/rpcl/Makefile.am | 8 +-- utils/utils.h | 12 ++++ 4 files changed, 149 insertions(+), 42 deletions(-) diff --git a/cli/gluster-block.c b/cli/gluster-block.c index 85b0e80..aca7de5 100644 --- a/cli/gluster-block.c +++ b/cli/gluster-block.c @@ -59,7 +59,7 @@ glusterBlockCliRPC_1(void *cobj, clioperations opt) blockInfoCli *info_obj; blockListCli *list_obj; blockModifyCli *modify_obj; - blockResponse *reply = NULL; + blockResponse reply = {0,}; char errMsg[2048] = {0}; @@ -103,8 +103,7 @@ glusterBlockCliRPC_1(void *cobj, clioperations opt) switch(opt) { case CREATE_CLI: create_obj = cobj; - reply = block_create_cli_1(create_obj, clnt); - if (!reply) { + if (block_create_cli_1(create_obj, &reply, clnt) != RPC_SUCCESS) { LOG("cli", GB_LOG_ERROR, "%sblock %s create on volume %s with hosts %s failed\n", clnt_sperror(clnt, "block_create_cli_1"), create_obj->block_name, @@ -114,8 +113,7 @@ glusterBlockCliRPC_1(void *cobj, clioperations opt) break; case DELETE_CLI: delete_obj = cobj; - reply = block_delete_cli_1(delete_obj, clnt); - if (!reply) { + if (block_delete_cli_1(delete_obj, &reply, clnt) != RPC_SUCCESS) { LOG("cli", GB_LOG_ERROR, "%sblock %s delete on volume %s failed", clnt_sperror(clnt, "block_delete_cli_1"), delete_obj->block_name, delete_obj->volume); @@ -124,8 +122,7 @@ glusterBlockCliRPC_1(void *cobj, clioperations opt) break; case INFO_CLI: info_obj = cobj; - reply = block_info_cli_1(info_obj, clnt); - if (!reply) { + if (block_info_cli_1(info_obj, &reply, clnt) != RPC_SUCCESS) { LOG("cli", GB_LOG_ERROR, "%sblock %s info on volume %s failed", clnt_sperror(clnt, "block_info_cli_1"), info_obj->block_name, info_obj->volume); @@ -134,8 +131,7 @@ glusterBlockCliRPC_1(void *cobj, clioperations opt) break; case LIST_CLI: list_obj = cobj; - reply = block_list_cli_1(list_obj, clnt); - if (!reply) { + if (block_list_cli_1(list_obj, &reply, clnt) != RPC_SUCCESS) { LOG("cli", GB_LOG_ERROR, "%sblock list on volume %s failed", clnt_sperror(clnt, "block_list_cli_1"), list_obj->volume); goto out; @@ -143,8 +139,7 @@ glusterBlockCliRPC_1(void *cobj, clioperations opt) break; case MODIFY_CLI: modify_obj = cobj; - reply = block_modify_cli_1(modify_obj, clnt); - if (!reply) { + if (block_modify_cli_1(modify_obj, &reply, clnt) != RPC_SUCCESS) { LOG("cli", GB_LOG_ERROR, "%sblock modify on volume %s failed", clnt_sperror(clnt, "block_modify_cli_1"), modify_obj->volume); goto out; @@ -153,9 +148,9 @@ glusterBlockCliRPC_1(void *cobj, clioperations opt) } out: - if (reply) { - ret = reply->exit; - MSG("%s", reply->out); + if (reply.out) { + ret = reply.exit; + MSG("%s", reply.out); } else if (errMsg[0]) { LOG("cli", GB_LOG_ERROR, "%s", errMsg); MSG("%s\n", errMsg); @@ -165,8 +160,9 @@ glusterBlockCliRPC_1(void *cobj, clioperations opt) ret = -1; } - if (clnt && reply) { - if (!clnt_freeres(clnt, (xdrproc_t)xdr_blockResponse, (char *)reply)) { + if (clnt) { + if (reply.out && !clnt_freeres(clnt, (xdrproc_t)xdr_blockResponse, + (char *)&reply)) { LOG("cli", GB_LOG_ERROR, "%s", clnt_sperror(clnt, "clnt_freeres failed")); } diff --git a/rpc/block_svc_routines.c b/rpc/block_svc_routines.c index d47a841..51b56ec 100644 --- a/rpc/block_svc_routines.c +++ b/rpc/block_svc_routines.c @@ -363,7 +363,7 @@ glusterBlockCallRPC_1(char *host, void *cobj, int ret = -1; int sockfd; int errsv = 0; - blockResponse *reply = NULL; + blockResponse reply = {0,}; struct sockaddr_in *sain = NULL; @@ -386,8 +386,7 @@ glusterBlockCallRPC_1(char *host, void *cobj, strcpy(((blockCreate *)cobj)->ipaddr, host); *rpc_sent = TRUE; - reply = block_create_1((blockCreate *)cobj, clnt); - if (!reply) { + if (block_create_1((blockCreate *)cobj, &reply, clnt) != RPC_SUCCESS) { LOG("mgmt", GB_LOG_ERROR, "%son host %s", clnt_sperror(clnt, "block remote create failed"), host); goto out; @@ -395,8 +394,7 @@ glusterBlockCallRPC_1(char *host, void *cobj, break; case DELETE_SRV: *rpc_sent = TRUE; - reply = block_delete_1((blockDelete *)cobj, clnt); - if (!reply) { + if (block_delete_1((blockDelete *)cobj, &reply, clnt) != RPC_SUCCESS) { LOG("mgmt", GB_LOG_ERROR, "%son host %s", clnt_sperror(clnt, "block remote delete failed"), host); goto out; @@ -404,8 +402,7 @@ glusterBlockCallRPC_1(char *host, void *cobj, break; case MODIFY_SRV: *rpc_sent = TRUE; - reply = block_modify_1((blockModify *)cobj, clnt); - if (!reply) { + if (block_modify_1((blockModify *)cobj, &reply, clnt) != RPC_SUCCESS) { LOG("mgmt", GB_LOG_ERROR, "%son host %s", clnt_sperror(clnt, "block remote modify failed"), host); goto out; @@ -417,16 +414,15 @@ glusterBlockCallRPC_1(char *host, void *cobj, goto out; } - if (reply) { - if (GB_STRDUP(*out, reply->out) < 0) { - goto out; - } - ret = reply->exit; + if (GB_STRDUP(*out, reply.out) < 0) { + goto out; } + ret = reply.exit; out: - if (clnt && reply) { - if (!clnt_freeres(clnt, (xdrproc_t)xdr_blockResponse, (char *)reply)) { + if (clnt) { + if (reply.out && !clnt_freeres(clnt, (xdrproc_t)xdr_blockResponse, + (char *)&reply)) { LOG("mgmt", GB_LOG_ERROR, "%s", clnt_sperror(clnt, "clnt_freeres failed")); @@ -1467,7 +1463,7 @@ blockModifyCliFormatResponse (blockModifyCli *blk, struct blockModify *mobj, } blockResponse * -block_modify_cli_1_svc(blockModifyCli *blk, struct svc_req *rqstp) +block_modify_cli_1_svc_st(blockModifyCli *blk, struct svc_req *rqstp) { int ret = -1; static blockModify mobj = {0}; @@ -1781,7 +1777,7 @@ blockCreateCliFormatResponse(struct glfs *glfs, blockCreateCli *blk, } blockResponse * -block_create_cli_1_svc(blockCreateCli *blk, struct svc_req *rqstp) +block_create_cli_1_svc_st(blockCreateCli *blk, struct svc_req *rqstp) { int errCode = -1; uuid_t uuid; @@ -2048,7 +2044,7 @@ out: blockResponse * -block_create_1_svc(blockCreate *blk, struct svc_req *rqstp) +block_create_1_svc_st(blockCreate *blk, struct svc_req *rqstp) { char *tmp = NULL; char *backstore = NULL; @@ -2276,7 +2272,7 @@ blockDeleteCliFormatResponse(blockDeleteCli *blk, int errCode, char *errMsg, } blockResponse * -block_delete_cli_1_svc(blockDeleteCli *blk, struct svc_req *rqstp) +block_delete_cli_1_svc_st(blockDeleteCli *blk, struct svc_req *rqstp) { blockRemoteDeleteResp *savereply = NULL; MetaInfo *info = NULL; @@ -2377,7 +2373,7 @@ block_delete_cli_1_svc(blockDeleteCli *blk, struct svc_req *rqstp) blockResponse * -block_delete_1_svc(blockDelete *blk, struct svc_req *rqstp) +block_delete_1_svc_st(blockDelete *blk, struct svc_req *rqstp) { int ret; char *iqn = NULL; @@ -2445,7 +2441,7 @@ block_delete_1_svc(blockDelete *blk, struct svc_req *rqstp) blockResponse * -block_modify_1_svc(blockModify *blk, struct svc_req *rqstp) +block_modify_1_svc_st(blockModify *blk, struct svc_req *rqstp) { int ret; char *authattr = NULL; @@ -2570,7 +2566,7 @@ block_modify_1_svc(blockModify *blk, struct svc_req *rqstp) blockResponse * -block_list_cli_1_svc(blockListCli *blk, struct svc_req *rqstp) +block_list_cli_1_svc_st(blockListCli *blk, struct svc_req *rqstp) { blockResponse *reply; struct glfs *glfs; @@ -2815,7 +2811,7 @@ blockInfoCliFormatResponse(blockInfoCli *blk, int errCode, } blockResponse * -block_info_cli_1_svc(blockInfoCli *blk, struct svc_req *rqstp) +block_info_cli_1_svc_st(blockInfoCli *blk, struct svc_req *rqstp) { blockResponse *reply; struct glfs *glfs; @@ -2884,3 +2880,106 @@ block_info_cli_1_svc(blockInfoCli *blk, struct svc_req *rqstp) return reply; } + + +bool_t +block_create_1_svc(blockCreate *blk, blockResponse *reply, struct svc_req *rqstp) +{ + int ret; + + GB_RPC_CALL(create, blk, reply, rqstp, ret); + return ret; +} + + +bool_t +block_delete_1_svc(blockDelete *blk, blockResponse *reply, struct svc_req *rqstp) +{ + int ret; + + GB_RPC_CALL(delete, blk, reply, rqstp, ret); + return ret; +} + + +bool_t +block_modify_1_svc(blockModify *blk, blockResponse *reply, struct svc_req *rqstp) +{ + int ret; + + GB_RPC_CALL(modify, blk, reply, rqstp, ret); + return ret; +} + + +bool_t +block_create_cli_1_svc(blockCreateCli *blk, blockResponse *reply, + struct svc_req *rqstp) +{ + int ret; + + GB_RPC_CALL(create_cli, blk, reply, rqstp, ret); + return ret; +} + + +bool_t +block_modify_cli_1_svc(blockModifyCli *blk, blockResponse *reply, + struct svc_req *rqstp) +{ + int ret; + + GB_RPC_CALL(modify_cli, blk, reply, rqstp, ret); + return ret; +} + + +bool_t +block_list_cli_1_svc(blockListCli *blk, blockResponse *reply, + struct svc_req *rqstp) +{ + int ret; + + GB_RPC_CALL(list_cli, blk, reply, rqstp, ret); + return ret; +} + + +bool_t +block_info_cli_1_svc(blockInfoCli *blk, blockResponse *reply, + struct svc_req *rqstp) +{ + int ret; + + GB_RPC_CALL(info_cli, blk, reply, rqstp, ret); + return ret; +} + + +bool_t +block_delete_cli_1_svc(blockDeleteCli *blk, blockResponse *reply, + struct svc_req *rqstp) +{ + int ret; + + GB_RPC_CALL(delete_cli, blk, reply, rqstp, ret); + return ret; +} + + +int +gluster_block_1_freeresult (SVCXPRT *transp, xdrproc_t xdr_result, caddr_t result) +{ + xdr_free (xdr_result, result); + + return 1; +} + + +int +gluster_block_cli_1_freeresult (SVCXPRT *transp, xdrproc_t xdr_result, caddr_t result) +{ + xdr_free (xdr_result, result); + + return 1; +} diff --git a/rpc/rpcl/Makefile.am b/rpc/rpcl/Makefile.am index 38b1dbf..08a40ea 100644 --- a/rpc/rpcl/Makefile.am +++ b/rpc/rpcl/Makefile.am @@ -10,17 +10,17 @@ DISTCLEANFILES = Makefile.in $(BUILT_SOURCES) CLEANFILES = *~ $(BUILT_SOURCES) block.h: block.x - rpcgen -h -o $(top_builddir)/rpc/rpcl/$@ $^ + rpcgen -hM -o $(top_builddir)/rpc/rpcl/$@ $^ block_xdr.c: block.x - rpcgen -c -o $(top_builddir)/rpc/rpcl/$@ $^ + rpcgen -cM -o $(top_builddir)/rpc/rpcl/$@ $^ block_clnt.c: block.x - rpcgen -l -o $(top_builddir)/rpc/rpcl/$@ $^ + rpcgen -lM -o $(top_builddir)/rpc/rpcl/$@ $^ $(SED) -i 's|TIMEOUT = { 25, 0 }|TIMEOUT = { 300, 0 }|' $(top_builddir)/rpc/rpcl/$@ block_svc.c: block.x - rpcgen -m -o $(top_builddir)/rpc/rpcl/$@ $^ + rpcgen -mM -o $(top_builddir)/rpc/rpcl/$@ $^ dist-hook: find $(distdir) -type f \ diff --git a/utils/utils.h b/utils/utils.h index d3b1027..ce1ef52 100644 --- a/utils/utils.h +++ b/utils/utils.h @@ -278,6 +278,18 @@ extern struct gbConf gbConf; GB_FREE(tmp); \ } while (0) +# define GB_RPC_CALL(op, blk, reply, rqstp, ret) \ + do { \ + blockResponse *resp = block_##op##_1_svc_st(blk, rqstp); \ + if (resp) { \ + memcpy(reply, resp, sizeof(*reply)); \ + GB_FREE(resp); \ + ret = true; \ + } else { \ + ret = false; \ + } \ + } while (0) + # define CALLOC(x) \ calloc(1, x) -- cgit