glusterd: performance improvement

Issue:
In the glusertd_op_stage_create_volume(), fetching of values
from the dict is done, whereas same values are fetched
by glusterd_check_brick_order() which is called from
that function. This leads to unnecssary performance overhead.

Fix:
Instead of fetching the values again, passing the
values to the glusterd_check_brick_order()
if it's fethced before, else a NULL is passed
and then only fetching is done here.
Also, few changes are made to the code to reduce
the cost of operations such as 'fast fail' for
false conditions and a bit of code clean up.

Fixes: #1397

Change-Id: Ic7b523adbca8eb63ef9eb29c206e3b19e05c0815
Signed-off-by: nik-redhat <nladha@redhat.com>

4 files changed, 83 insertions, 69 deletions

diff --git a/xlators/mgmt/glusterd/src/glusterd-brick-ops.c b/xlators/mgmt/glusterd/src/glusterd-brick-ops.c
index 6d1a1e98848..5b33ebb0723 100644
--- a/xlators/mgmt/glusterd/src/glusterd-brick-ops.c
+++ b/xlators/mgmt/glusterd/src/glusterd-brick-ops.c
@@ -1359,14 +1359,14 @@ glusterd_op_stage_add_brick(dict_t *dict, char **op_errstr, dict_t *rsp_dict)
 
     ret = dict_get_strn(dict, "volname", SLEN("volname"), &volname);
     if (ret) {
-        gf_msg(THIS->name, GF_LOG_ERROR, errno, GD_MSG_DICT_GET_FAILED,
+        gf_msg(this->name, GF_LOG_ERROR, errno, GD_MSG_DICT_GET_FAILED,
                "Unable to get volume name");
         goto out;
     }
 
     ret = glusterd_volinfo_find(volname, &volinfo);
     if (ret) {
-        gf_msg(THIS->name, GF_LOG_ERROR, 0, GD_MSG_VOL_NOT_FOUND,
+        gf_msg(this->name, GF_LOG_ERROR, 0, GD_MSG_VOL_NOT_FOUND,
                "Unable to find volume: %s", volname);
         goto out;
     }
@@ -1378,13 +1378,7 @@ glusterd_op_stage_add_brick(dict_t *dict, char **op_errstr, dict_t *rsp_dict)
     ret = dict_get_int32n(dict, "replica-count", SLEN("replica-count"),
                           &replica_count);
     if (ret) {
-        gf_msg_debug(THIS->name, 0, "Unable to get replica count");
-    }
-
-    ret = dict_get_int32n(dict, "arbiter-count", SLEN("arbiter-count"),
-                          &arbiter_count);
-    if (ret) {
-        gf_msg_debug(THIS->name, 0, "No arbiter count present in the dict");
+        gf_msg_debug(this->name, 0, "Unable to get replica count");
     }
 
     if (replica_count > 0) {
@@ -1400,18 +1394,18 @@ glusterd_op_stage_add_brick(dict_t *dict, char **op_errstr, dict_t *rsp_dict)
 
     glusterd_add_peers_to_auth_list(volname);
 
-    if (glusterd_is_volume_replicate(volinfo)) {
+    if (replica_count && glusterd_is_volume_replicate(volinfo)) {
         /* Do not allow add-brick for stopped volumes when replica-count
          * is being increased.
          */
-        if (conf->op_version >= GD_OP_VERSION_3_7_10 && replica_count &&
-            GLUSTERD_STATUS_STOPPED == volinfo->status) {
+        if (GLUSTERD_STATUS_STOPPED == volinfo->status &&
+            conf->op_version >= GD_OP_VERSION_3_7_10) {
             ret = -1;
             snprintf(msg, sizeof(msg),
                      " Volume must not be in"
                      " stopped state when replica-count needs to "
                      " be increased.");
-            gf_msg(THIS->name, GF_LOG_ERROR, 0, GD_MSG_BRICK_ADD_FAIL, "%s",
+            gf_msg(this->name, GF_LOG_ERROR, 0, GD_MSG_BRICK_ADD_FAIL, "%s",
                    msg);
             *op_errstr = gf_strdup(msg);
             goto out;
@@ -1419,25 +1413,31 @@ glusterd_op_stage_add_brick(dict_t *dict, char **op_errstr, dict_t *rsp_dict)
         /* op-version check for replica 2 to arbiter conversion. If we
          * don't have this check, an older peer added as arbiter brick
          * will not have the  arbiter xlator in its volfile. */
-        if ((conf->op_version < GD_OP_VERSION_3_8_0) && (arbiter_count == 1) &&
-            (replica_count == 3)) {
-            ret = -1;
-            snprintf(msg, sizeof(msg),
-                     "Cluster op-version must "
-                     "be >= 30800 to add arbiter brick to a "
-                     "replica 2 volume.");
-            gf_msg(THIS->name, GF_LOG_ERROR, 0, GD_MSG_BRICK_ADD_FAIL, "%s",
-                   msg);
-            *op_errstr = gf_strdup(msg);
-            goto out;
+        if ((replica_count == 3) && (conf->op_version < GD_OP_VERSION_3_8_0)) {
+            ret = dict_get_int32n(dict, "arbiter-count", SLEN("arbiter-count"),
+                                  &arbiter_count);
+            if (ret) {
+                gf_msg_debug(this->name, 0,
+                             "No arbiter count present in the dict");
+            } else if (arbiter_count == 1) {
+                ret = -1;
+                snprintf(msg, sizeof(msg),
+                         "Cluster op-version must "
+                         "be >= 30800 to add arbiter brick to a "
+                         "replica 2 volume.");
+                gf_msg(this->name, GF_LOG_ERROR, 0, GD_MSG_BRICK_ADD_FAIL, "%s",
+                       msg);
+                *op_errstr = gf_strdup(msg);
+                goto out;
+            }
         }
         /* Do not allow increasing replica count for arbiter volumes. */
-        if (replica_count && volinfo->arbiter_count) {
+        if (volinfo->arbiter_count) {
             ret = -1;
             snprintf(msg, sizeof(msg),
                      "Increasing replica count "
                      "for arbiter volumes is not supported.");
-            gf_msg(THIS->name, GF_LOG_ERROR, 0, GD_MSG_BRICK_ADD_FAIL, "%s",
+            gf_msg(this->name, GF_LOG_ERROR, 0, GD_MSG_BRICK_ADD_FAIL, "%s",
                    msg);
             *op_errstr = gf_strdup(msg);
             goto out;
@@ -1451,7 +1451,7 @@ glusterd_op_stage_add_brick(dict_t *dict, char **op_errstr, dict_t *rsp_dict)
      * doing this check at the originator node is sufficient.
      */
 
-    if (is_origin_glusterd(dict) && !is_force) {
+    if (!is_force && is_origin_glusterd(dict)) {
         ret = 0;
         if (volinfo->type == GF_CLUSTER_TYPE_REPLICATE) {
             gf_msg_debug(this->name, 0,
@@ -1459,15 +1459,18 @@ glusterd_op_stage_add_brick(dict_t *dict, char **op_errstr, dict_t *rsp_dict)
                          "found. Checking brick order.");
             if (replica_count)
                 ret = glusterd_check_brick_order(dict, msg, volinfo->type,
+                                                 &volname, &bricks, &count,
                                                  replica_count);
             else
                 ret = glusterd_check_brick_order(dict, msg, volinfo->type,
+                                                 &volname, &bricks, &count,
                                                  volinfo->replica_count);
         } else if (volinfo->type == GF_CLUSTER_TYPE_DISPERSE) {
             gf_msg_debug(this->name, 0,
                          "Disperse cluster type"
                          " found. Checking brick order.");
-            ret = glusterd_check_brick_order(dict, msg, volinfo->type,
+            ret = glusterd_check_brick_order(dict, msg, volinfo->type, &volname,
+                                             &bricks, &count,
                                              volinfo->disperse_count);
         }
         if (ret) {
@@ -1496,7 +1499,7 @@ glusterd_op_stage_add_brick(dict_t *dict, char **op_errstr, dict_t *rsp_dict)
                 if (len < 0) {
                     strcpy(msg, "<error>");
                 }
-                gf_msg(THIS->name, GF_LOG_ERROR, 0, GD_MSG_BRICK_ADD_FAIL, "%s",
+                gf_msg(this->name, GF_LOG_ERROR, 0, GD_MSG_BRICK_ADD_FAIL, "%s",
                        msg);
                 *op_errstr = gf_strdup(msg);
                 goto out;
@@ -1528,7 +1531,7 @@ glusterd_op_stage_add_brick(dict_t *dict, char **op_errstr, dict_t *rsp_dict)
                  "Volume name %s rebalance is in "
                  "progress. Please retry after completion",
                  volname);
-        gf_msg(THIS->name, GF_LOG_ERROR, 0, GD_MSG_OIP_RETRY_LATER, "%s", msg);
+        gf_msg(this->name, GF_LOG_ERROR, 0, GD_MSG_OIP_RETRY_LATER, "%s", msg);
         *op_errstr = gf_strdup(msg);
         ret = -1;
         goto out;
@@ -1546,18 +1549,22 @@ glusterd_op_stage_add_brick(dict_t *dict, char **op_errstr, dict_t *rsp_dict)
         msg[0] = '\0';
     }
 
-    ret = dict_get_int32n(dict, "count", SLEN("count"), &count);
-    if (ret) {
-        gf_msg("glusterd", GF_LOG_ERROR, errno, GD_MSG_DICT_GET_FAILED,
-               "Unable to get count");
-        goto out;
+    if (!count) {
+        ret = dict_get_int32n(dict, "count", SLEN("count"), &count);
+        if (ret) {
+            gf_msg(this->name, GF_LOG_ERROR, errno, GD_MSG_DICT_GET_FAILED,
+                   "Unable to get count");
+            goto out;
+        }
     }
 
-    ret = dict_get_strn(dict, "bricks", SLEN("bricks"), &bricks);
-    if (ret) {
-        gf_msg(THIS->name, GF_LOG_ERROR, errno, GD_MSG_DICT_GET_FAILED,
-               "Unable to get bricks");
-        goto out;
+    if (!bricks) {
+        ret = dict_get_strn(dict, "bricks", SLEN("bricks"), &bricks);
+        if (ret) {
+            gf_msg(this->name, GF_LOG_ERROR, errno, GD_MSG_DICT_GET_FAILED,
+                   "Unable to get bricks");
+            goto out;
+        }
     }
 
     if (bricks) {
@@ -1576,7 +1583,7 @@ glusterd_op_stage_add_brick(dict_t *dict, char **op_errstr, dict_t *rsp_dict)
                      "brick path %s is "
                      "too long",
                      brick);
-            gf_msg(THIS->name, GF_LOG_ERROR, 0, GD_MSG_BRKPATH_TOO_LONG, "%s",
+            gf_msg(this->name, GF_LOG_ERROR, 0, GD_MSG_BRKPATH_TOO_LONG, "%s",
                    msg);
             *op_errstr = gf_strdup(msg);
 
@@ -1587,7 +1594,7 @@ glusterd_op_stage_add_brick(dict_t *dict, char **op_errstr, dict_t *rsp_dict)
         ret = glusterd_brickinfo_new_from_brick(brick, &brickinfo, _gf_true,
                                                 NULL);
         if (ret) {
-            gf_msg(THIS->name, GF_LOG_ERROR, 0, GD_MSG_BRICK_NOT_FOUND,
+            gf_msg(this->name, GF_LOG_ERROR, 0, GD_MSG_BRICK_NOT_FOUND,
                    "Add-brick: Unable"
                    " to get brickinfo");
             goto out;
@@ -1657,7 +1664,7 @@ out:
     GF_FREE(str_ret);
     GF_FREE(all_bricks);
 
-    gf_msg_debug(THIS->name, 0, "Returning %d", ret);
+    gf_msg_debug(this->name, 0, "Returning %d", ret);
 
     return ret;
 }
diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c b/xlators/mgmt/glusterd/src/glusterd-utils.c
index 7d38b0a42d7..2f3fc218093 100644
--- a/xlators/mgmt/glusterd/src/glusterd-utils.c
+++ b/xlators/mgmt/glusterd/src/glusterd-utils.c
@@ -14632,7 +14632,8 @@ glusterd_compare_addrinfo(struct addrinfo *first, struct addrinfo *next)
  */
 int32_t
 glusterd_check_brick_order(dict_t *dict, char *err_str, int32_t type,
-                           int32_t sub_count)
+                           char **volname, char **brick_list,
+                           int32_t *brick_count, int32_t sub_count)
 {
     int ret = -1;
     int i = 0;
@@ -14643,12 +14644,9 @@ glusterd_check_brick_order(dict_t *dict, char *err_str, int32_t type,
     addrinfo_list_t *ai_list_tmp1 = NULL;
     addrinfo_list_t *ai_list_tmp2 = NULL;
     char *brick = NULL;
-    char *brick_list = NULL;
     char *brick_list_dup = NULL;
     char *brick_list_ptr = NULL;
     char *tmpptr = NULL;
-    char *volname = NULL;
-    int32_t brick_count = 0;
     struct addrinfo *ai_info = NULL;
     char brick_addr[128] = {
         0,
@@ -14676,32 +14674,38 @@ glusterd_check_brick_order(dict_t *dict, char *err_str, int32_t type,
     ai_list->info = NULL;
     CDS_INIT_LIST_HEAD(&ai_list->list);
 
-    ret = dict_get_strn(dict, "volname", SLEN("volname"), &volname);
-    if (ret) {
-        gf_msg(this->name, GF_LOG_ERROR, 0, GD_MSG_DICT_GET_FAILED,
-               "Unable to get volume name");
-        goto out;
+    if (!(*volname)) {
+        ret = dict_get_strn(dict, "volname", SLEN("volname"), &(*volname));
+        if (ret) {
+            gf_msg(this->name, GF_LOG_ERROR, 0, GD_MSG_DICT_GET_FAILED,
+                   "Unable to get volume name");
+            goto out;
+        }
     }
 
-    ret = dict_get_strn(dict, "bricks", SLEN("bricks"), &brick_list);
-    if (ret) {
-        gf_msg(this->name, GF_LOG_ERROR, 0, GD_MSG_DICT_GET_FAILED,
-               "Bricks check : Could not "
-               "retrieve bricks list");
-        goto out;
+    if (!(*brick_list)) {
+        ret = dict_get_strn(dict, "bricks", SLEN("bricks"), &(*brick_list));
+        if (ret) {
+            gf_msg(this->name, GF_LOG_ERROR, 0, GD_MSG_DICT_GET_FAILED,
+                   "Bricks check : Could not "
+                   "retrieve bricks list");
+            goto out;
+        }
     }
 
-    ret = dict_get_int32n(dict, "count", SLEN("count"), &brick_count);
-    if (ret) {
-        gf_msg(this->name, GF_LOG_ERROR, 0, GD_MSG_DICT_GET_FAILED,
-               "Bricks check : Could not "
-               "retrieve brick count");
-        goto out;
+    if (!(*brick_count)) {
+        ret = dict_get_int32n(dict, "count", SLEN("count"), &(*brick_count));
+        if (ret) {
+            gf_msg(this->name, GF_LOG_ERROR, 0, GD_MSG_DICT_GET_FAILED,
+                   "Bricks check : Could not "
+                   "retrieve brick count");
+            goto out;
+        }
     }
 
-    brick_list_dup = brick_list_ptr = gf_strdup(brick_list);
+    brick_list_dup = brick_list_ptr = gf_strdup(*brick_list);
     /* Resolve hostnames and get addrinfo */
-    while (i < brick_count) {
+    while (i < *brick_count) {
         ++i;
         brick = strtok_r(brick_list_dup, " \n", &tmpptr);
         brick_list_dup = tmpptr;
@@ -14738,7 +14742,7 @@ glusterd_check_brick_order(dict_t *dict, char *err_str, int32_t type,
     ai_list_tmp1 = cds_list_entry(ai_list->list.next, addrinfo_list_t, list);
 
     /* Check for bad brick order */
-    while (i < brick_count) {
+    while (i < *brick_count) {
         ++i;
         ai_info = ai_list_tmp1->info;
         ai_list_tmp1 = cds_list_entry(ai_list_tmp1->list.next, addrinfo_list_t,
diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.h b/xlators/mgmt/glusterd/src/glusterd-utils.h
index 05346916968..bf6ac295e26 100644
--- a/xlators/mgmt/glusterd/src/glusterd-utils.h
+++ b/xlators/mgmt/glusterd/src/glusterd-utils.h
@@ -859,6 +859,7 @@ glusterd_add_shd_to_dict(glusterd_volinfo_t *volinfo, dict_t *dict,
                          int32_t count);
 int32_t
 glusterd_check_brick_order(dict_t *dict, char *err_str, int32_t type,
+                           char **volname, char **bricks, int32_t *brick_count,
                            int32_t sub_count);
 
 #endif
diff --git a/xlators/mgmt/glusterd/src/glusterd-volume-ops.c b/xlators/mgmt/glusterd/src/glusterd-volume-ops.c
index cafdffb63c4..814ab14fb27 100644
--- a/xlators/mgmt/glusterd/src/glusterd-volume-ops.c
+++ b/xlators/mgmt/glusterd/src/glusterd-volume-ops.c
@@ -1002,7 +1002,8 @@ glusterd_op_stage_create_volume(dict_t *dict, char **op_errstr,
                 gf_msg_debug(this->name, 0,
                              "Replicate cluster type "
                              "found. Checking brick order.");
-                ret = glusterd_check_brick_order(dict, msg, type,
+                ret = glusterd_check_brick_order(dict, msg, type, &volname,
+                                                 &bricks, &brick_count,
                                                  replica_count);
             } else if (type == GF_CLUSTER_TYPE_DISPERSE) {
                 ret = dict_get_int32n(dict, "disperse-count",
@@ -1016,7 +1017,8 @@ glusterd_op_stage_create_volume(dict_t *dict, char **op_errstr,
                 gf_msg_debug(this->name, 0,
                              "Disperse cluster type"
                              " found. Checking brick order.");
-                ret = glusterd_check_brick_order(dict, msg, type,
+                ret = glusterd_check_brick_order(dict, msg, type, &volname,
+                                                 &bricks, &brick_count,
                                                  disperse_count);
             }
             if (ret) {