glusterd: fix client io-threads option for replicate volumes

Backport of https://review.gluster.org/#/c/18430/

Problem:
Commit ff075a3d6f9b142911d25c27fd209838782bfff0 disabled loading
client-io-threads for replicate volumes (it was set to on by default in
commit e068c1997314046658dd502e9118dab32decf879) due to performance
issues but in doing so, inadvertently failed to load the xlator even if
the user explicitly enabled the option using the volume set command.
This was despite returning returning sucess for the volume set.

Fix:
Modify the check in perfxl_option_handler() and add checks in volume
create/add-brick/remove-brick code paths, tying it all to
GD_OP_VERSION_3_12_2.

Change-Id: Ib612973a999a7da818cc926f5c2601b1f0794fcf
BUG: 1499158
Signed-off-by: Ravishankar N <ravishankar@redhat.com>

8 files changed, 143 insertions, 35 deletions

diff --git a/libglusterfs/src/globals.h b/libglusterfs/src/globals.h
index a03cf640821..365183d1562 100644
--- a/libglusterfs/src/globals.h
+++ b/libglusterfs/src/globals.h
@@ -38,7 +38,7 @@
  */
 #define GD_OP_VERSION_MIN  1 /* MIN is the fresh start op-version, mostly
                                 should not change */
-#define GD_OP_VERSION_MAX  GD_OP_VERSION_3_12_0 /* MAX VERSION is the maximum
+#define GD_OP_VERSION_MAX  GD_OP_VERSION_3_12_2 /* MAX VERSION is the maximum
                                                   count in VME table, should
                                                   keep changing with
                                                   introduction of newer
@@ -88,6 +88,8 @@
 
 #define GD_OP_VERSION_3_12_0   31200 /* Op-version for GlusterFS 3.12.0 */
 
+#define GD_OP_VERSION_3_12_2   31202 /* Op-version for GlusterFS 3.12.2 */
+
 #define GD_OP_VER_PERSISTENT_AFR_XATTRS GD_OP_VERSION_3_6_0
 
 #include "xlator.h"
diff --git a/tests/bugs/replicate/bug-1498570-client-iot-graph-check.t b/tests/bugs/replicate/bug-1498570-client-iot-graph-check.t
new file mode 100644
index 00000000000..2b3b3040228
--- /dev/null
+++ b/tests/bugs/replicate/bug-1498570-client-iot-graph-check.t
@@ -0,0 +1,48 @@
+#!/bin/bash
+. $(dirname $0)/../../include.rc
+. $(dirname $0)/../../volume.rc
+. $(dirname $0)/../../afr.rc
+
+TESTS_EXPECTED_IN_LOOP=21
+function reset_cluster
+{
+        cleanup
+        TEST glusterd
+        TEST pidof glusterd
+
+}
+function check_iot_option
+{
+        local enabled=$1
+        local is_loaded_in_graph=$2
+
+        EXPECT "$enabled" volume_get_field $V0 client-io-threads
+        IOT_STRING="volume\ $V0-io-threads"
+        grep "$IOT_STRING" $GLUSTERD_WORKDIR/vols/$V0/trusted-$V0.tcp-fuse.vol
+        TEST ret=$?
+        EXPECT_NOT "$is_loaded_in_graph" echo $ret
+}
+
+reset_cluster
+TEST $CLI volume create $V0 $H0:$B0/${V0}{0,1}
+check_iot_option on 1
+
+reset_cluster
+TEST $CLI volume create $V0 replica 2 $H0:$B0/${V0}{0,1}
+check_iot_option off 0
+
+reset_cluster
+TEST $CLI volume create $V0 $H0:$B0/${V0}0
+TEST $CLI volume add-brick $V0 replica 2 $H0:$B0/${V0}1
+check_iot_option off 0
+TEST $CLI volume remove-brick $V0 replica 1 $H0:$B0/${V0}1 force
+check_iot_option on 1
+
+reset_cluster
+TEST $CLI volume create $V0 replica 3 $H0:$B0/${V0}{0..5}
+TEST $CLI volume set $V0 client-io-threads on
+check_iot_option on 1
+TEST $CLI volume remove-brick $V0 replica 2 $H0:$B0/${V0}2 $H0:$B0/${V0}5 force
+check_iot_option on 1
+
+cleanup
diff --git a/xlators/mgmt/glusterd/src/glusterd-brick-ops.c b/xlators/mgmt/glusterd/src/glusterd-brick-ops.c
index fdb6014bb63..6d17ff4e32d 100644
--- a/xlators/mgmt/glusterd/src/glusterd-brick-ops.c
+++ b/xlators/mgmt/glusterd/src/glusterd-brick-ops.c
@@ -1432,6 +1432,24 @@ glusterd_op_perform_add_bricks (glusterd_volinfo_t *volinfo, int32_t count,
         /* Gets changed only if the options are given in add-brick cli */
         if (type)
                 volinfo->type = type;
+        /* performance.client-io-threads is turned on by default,
+         * however this has adverse effects on replicate volumes due to
+         * replication design issues, till that get addressed
+         * performance.client-io-threads option is turned off for all
+         * replicate volumes if not already explicitly enabled.
+         */
+        if (type && glusterd_is_volume_replicate (volinfo) &&
+            conf->op_version >= GD_OP_VERSION_3_12_2) {
+                ret = dict_set_str (volinfo->dict,
+                                    "performance.client-io-threads",
+                                    "off");
+                if (ret) {
+                        gf_msg (this->name, GF_LOG_ERROR, 0,
+                                GD_MSG_DICT_SET_FAILED, "Failed to set "
+                                "performance.client-io-threads to off");
+                        goto out;
+                }
+        }
 
         if (replica_count) {
                 volinfo->replica_count = replica_count;
@@ -2713,9 +2731,12 @@ glusterd_op_remove_brick (dict_t *dict, char **op_errstr)
         char                    *cold_shd_key  = NULL;
         char                    *hot_shd_key   = NULL;
         int                      delete_key    = 1;
+        glusterd_conf_t         *conf          = NULL;
 
         this = THIS;
         GF_ASSERT (this);
+        conf = this->private;
+        GF_VALIDATE_OR_GOTO (this->name, conf, out);
 
         ret = dict_get_str (dict, "volname", &volname);
 
@@ -3005,6 +3026,19 @@ glusterd_op_remove_brick (dict_t *dict, char **op_errstr)
         volinfo->subvol_count = (volinfo->brick_count /
                                  volinfo->dist_leaf_count);
 
+        if (!glusterd_is_volume_replicate (volinfo) &&
+            conf->op_version >= GD_OP_VERSION_3_12_2) {
+                ret = dict_set_str (volinfo->dict,
+                                    "performance.client-io-threads",
+                                    "on");
+                if (ret) {
+                        gf_msg (this->name, GF_LOG_ERROR, 0,
+                                GD_MSG_DICT_SET_FAILED, "Failed to set "
+                                "performance.client-io-threads to on");
+                        goto out;
+                }
+        }
+
         ret = glusterd_create_volfiles_and_notify_services (volinfo);
         if (ret) {
                 gf_msg (this->name, GF_LOG_WARNING, 0,
diff --git a/xlators/mgmt/glusterd/src/glusterd-handler.c b/xlators/mgmt/glusterd/src/glusterd-handler.c
index c0929fa8192..9d496e56c07 100644
--- a/xlators/mgmt/glusterd/src/glusterd-handler.c
+++ b/xlators/mgmt/glusterd/src/glusterd-handler.c
@@ -4837,7 +4837,7 @@ glusterd_get_volume_opts (rpcsvc_request_t *req, dict_t *dict)
                                                                (dict,
                                                                 _gf_false,
                                                                 key, orig_key,
-                                                                volinfo,
+                                                                volinfo->dict,
                                                                 &rsp.op_errstr);
                                         if (ret && !rsp.op_errstr) {
                                                 snprintf (err_str,
@@ -4863,7 +4863,7 @@ glusterd_get_volume_opts (rpcsvc_request_t *req, dict_t *dict)
         } else {
                 /* Handle the "all" volume option request */
                 ret = glusterd_get_default_val_for_volopt (dict, _gf_true, NULL,
-                                                           NULL, volinfo,
+                                                           NULL, volinfo->dict,
                                                            &rsp.op_errstr);
                 if (ret && !rsp.op_errstr) {
                         snprintf (err_str, sizeof(err_str),
@@ -5454,7 +5454,8 @@ glusterd_get_state (rpcsvc_request_t *req, dict_t *dict)
                         vol_all_opts = dict_new ();
 
                         ret = glusterd_get_default_val_for_volopt (vol_all_opts,
-                                        _gf_true, NULL, NULL, volinfo, &rsp.op_errstr);
+                                        _gf_true, NULL, NULL, volinfo->dict,
+                                        &rsp.op_errstr);
                         if (ret) {
                                 gf_msg (this->name, GF_LOG_ERROR, 0,
                                         GD_MSG_VOL_OPTS_IMPORT_FAIL, "Failed to "
diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c b/xlators/mgmt/glusterd/src/glusterd-utils.c
index ac59e4d75d1..e38f96313ed 100644
--- a/xlators/mgmt/glusterd/src/glusterd-utils.c
+++ b/xlators/mgmt/glusterd/src/glusterd-utils.c
@@ -12613,8 +12613,7 @@ out:
 int
 glusterd_get_default_val_for_volopt (dict_t *ctx, gf_boolean_t all_opts,
                                      char *input_key, char *orig_key,
-                                     glusterd_volinfo_t *volinfo,
-                                     char **op_errstr)
+                                     dict_t *vol_dict, char **op_errstr)
 {
         struct volopt_map_entry *vme = NULL;
         int                      ret = -1;
@@ -12625,7 +12624,6 @@ glusterd_get_default_val_for_volopt (dict_t *ctx, gf_boolean_t all_opts,
         char                     dict_key[50] = {0,};
         gf_boolean_t             key_found = _gf_false;
         glusterd_conf_t         *priv = NULL;
-        dict_t                  *vol_dict = NULL;
 
         this = THIS;
         GF_ASSERT (this);
@@ -12633,7 +12631,6 @@ glusterd_get_default_val_for_volopt (dict_t *ctx, gf_boolean_t all_opts,
         priv = this->private;
         GF_VALIDATE_OR_GOTO (this->name, priv, out);
 
-        vol_dict = volinfo->dict;
         GF_VALIDATE_OR_GOTO (this->name, vol_dict, out);
 
         /* Check whether key is passed for a single option */
@@ -12655,20 +12652,6 @@ glusterd_get_default_val_for_volopt (dict_t *ctx, gf_boolean_t all_opts,
                 if (!def_val) {
                         ret = dict_get_str (vol_dict, vme->key, &def_val);
                         if (!def_val) {
-                                /* For replicate volumes
-                                 * performance.client-io-threads will be set to
-                                 * off by default until explicitly turned on
-                                 */
-                                if (!strcmp (vme->key,
-                                            "performance.client-io-threads")) {
-                                        if (volinfo->type ==
-                                            GF_CLUSTER_TYPE_REPLICATE ||
-                                            volinfo->type ==
-                                            GF_CLUSTER_TYPE_STRIPE_REPLICATE) {
-                                                def_val = "off";
-                                                goto set_count;
-                                        }
-                                }
                                 if (vme->value) {
                                         def_val = vme->value;
                                 } else {
@@ -12681,7 +12664,6 @@ glusterd_get_default_val_for_volopt (dict_t *ctx, gf_boolean_t all_opts,
                                 }
                         }
                 }
-set_count:
                 count++;
                 sprintf (dict_key, "key%d", count);
                 ret = dict_set_str(ctx, dict_key, vme->key);
diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.h b/xlators/mgmt/glusterd/src/glusterd-utils.h
index fce56b12e9c..b802f6ca616 100644
--- a/xlators/mgmt/glusterd/src/glusterd-utils.h
+++ b/xlators/mgmt/glusterd/src/glusterd-utils.h
@@ -722,8 +722,7 @@ glusterd_get_global_options_for_all_vols (rpcsvc_request_t *req, dict_t *dict,
 int
 glusterd_get_default_val_for_volopt (dict_t *dict, gf_boolean_t all_opts,
                                      char *key, char *orig_key,
-                                     glusterd_volinfo_t  *volinfo,
-                                     char **err_str);
+                                     dict_t *vol_dict, char **err_str);
 
 int
 glusterd_check_client_op_version_support (char *volname, uint32_t op_version,
diff --git a/xlators/mgmt/glusterd/src/glusterd-volgen.c b/xlators/mgmt/glusterd/src/glusterd-volgen.c
index 9ee4c8d4c52..97049ac7c8c 100644
--- a/xlators/mgmt/glusterd/src/glusterd-volgen.c
+++ b/xlators/mgmt/glusterd/src/glusterd-volgen.c
@@ -2676,9 +2676,15 @@ perfxl_option_handler (volgen_graph_t *graph, struct volopt_map_entry *vme,
 {
         gf_boolean_t enabled = _gf_false;
         glusterd_volinfo_t *volinfo = NULL;
+        xlator_t *this = NULL;
+        glusterd_conf_t *priv = NULL;
 
-        GF_ASSERT (param);
+        GF_VALIDATE_OR_GOTO ("glusterd", param, out);
         volinfo = param;
+        this = THIS;
+        GF_VALIDATE_OR_GOTO ("glusterd", this, out);
+        priv = this->private;
+        GF_VALIDATE_OR_GOTO ("glusterd", priv, out);
 
         if (strcmp (vme->option, "!perf") != 0)
                 return 0;
@@ -2694,13 +2700,15 @@ perfxl_option_handler (volgen_graph_t *graph, struct volopt_map_entry *vme,
             (vme->op_version > volinfo->client_op_version))
                 return 0;
 
-        /* For replicate volumes do not load io-threads as it affects
-         * performance
-         */
-        if (!strcmp (vme->key, "performance.client-io-threads") &&
-            (GF_CLUSTER_TYPE_STRIPE_REPLICATE == volinfo->type ||
-             GF_CLUSTER_TYPE_REPLICATE == volinfo->type))
-                return 0;
+        if (priv->op_version < GD_OP_VERSION_3_12_2) {
+                /* For replicate volumes do not load io-threads as it affects
+                 * performance
+                 */
+                if (!strcmp (vme->key, "performance.client-io-threads") &&
+                    (GF_CLUSTER_TYPE_STRIPE_REPLICATE == volinfo->type ||
+                     GF_CLUSTER_TYPE_REPLICATE == volinfo->type))
+                        return 0;
+        }
 
         /* if VKEY_READDIR_AHEAD is enabled and parallel readdir is
          * not enabled then load readdir-ahead here else it will be
@@ -2711,8 +2719,8 @@ perfxl_option_handler (volgen_graph_t *graph, struct volopt_map_entry *vme,
 
         if (volgen_graph_add (graph, vme->voltype, volinfo->volname))
                 return 0;
-        else
-                return -1;
+out:
+        return -1;
 }
 
 static int
diff --git a/xlators/mgmt/glusterd/src/glusterd-volume-ops.c b/xlators/mgmt/glusterd/src/glusterd-volume-ops.c
index 9ce600e51c3..9d340738452 100644
--- a/xlators/mgmt/glusterd/src/glusterd-volume-ops.c
+++ b/xlators/mgmt/glusterd/src/glusterd-volume-ops.c
@@ -2231,6 +2231,23 @@ glusterd_op_create_volume (dict_t *dict, char **op_errstr)
         volinfo->stripe_count = 1;
 
         if (GF_CLUSTER_TYPE_REPLICATE == volinfo->type) {
+                /* performance.client-io-threads is turned on to default,
+                 * however this has adverse effects on replicate volumes due to
+                 * replication design issues, till that get addressed
+                 * performance.client-io-threads option is turned off for all
+                 * replicate volumes
+                 */
+                if (priv->op_version >= GD_OP_VERSION_3_12_2) {
+                        ret = dict_set_str (volinfo->dict,
+                                            "performance.client-io-threads",
+                                            "off");
+                        if (ret) {
+                                gf_msg (this->name, GF_LOG_ERROR, 0,
+                                        GD_MSG_DICT_SET_FAILED, "Failed to set "
+                                        "performance.client-io-threads to off");
+                                goto out;
+                        }
+                }
                 ret = dict_get_int32 (dict, "replica-count",
                                       &volinfo->replica_count);
                 if (ret) {
@@ -2251,6 +2268,23 @@ glusterd_op_create_volume (dict_t *dict, char **op_errstr)
                         goto out;
                 }
         } else if (GF_CLUSTER_TYPE_STRIPE_REPLICATE == volinfo->type) {
+                /* performance.client-io-threads is turned on to default,
+                 * however this has adverse effects on replicate volumes due to
+                 * replication design issues, till that get addressed
+                 * performance.client-io-threads option is turned off for all
+                 * replicate volumes
+                 */
+                if (priv->op_version >= GD_OP_VERSION_3_12_2) {
+                        ret = dict_set_str (volinfo->dict,
+                                            "performance.client-io-threads",
+                                            "off");
+                        if (ret) {
+                                gf_msg (this->name, GF_LOG_ERROR, 0,
+                                        GD_MSG_DICT_SET_FAILED, "Failed to set "
+                                        "performance.client-io-threads to off");
+                                goto out;
+                        }
+                }
                 ret = dict_get_int32 (dict, "stripe-count",
                                       &volinfo->stripe_count);
                 if (ret) {