From e34557b686ae1c51ba90be80c7d47b14122bbd38 Mon Sep 17 00:00:00 2001 From: Csaba Henk Date: Tue, 9 May 2017 15:52:43 +0200 Subject: [PATCH 425/426] libglusterfs: stop special casing "cache-size" in size_t validation The original situation was as follows: The function that validates xlator options indicating a size, xlator_option_validate_sizet(), handles the case when the name of the option is "cache-size" in a special way. - Xlator options (things of type volume_option_t) has a min and max attribute of type double. - An xlator option is endowed with a gluster specific type (not C type). An instance of an xlator option goes through a validation process by a type specific validator function (which are collected in option.c). - Validators of numeric types - size being one of them - make use the min and max attributes to perform a range check, except in one case: if an option is defined with min = max = 0, then this option will be exempt of range checking. (Note: the volume_option_t definition features the following comments along the min, max fields: double min; /* 0 means no range */ double max; /* 0 means no range */ which is slightly misleading as it lets one to conclude that zeroing min or max buys exemption from low or high boundary check, which is not true -- only *both* being zero buys exemption.) - Besides this, the validator for options of size type, xlator_option_validate_sizet() special cases options named "cache-size" so that only min is enforced. (The only consequence of a value exceeding max is that glusterd logs a warning about it, but the cli user who makes such a setting gets no feedback on it.) - This was introduced because a hard coded limit is not useful for io-cache and quick-read. They rather use a runtime calculated upper limit. (See changes I7dd4d8c53051b89a293696abf1ee8dc237e39a20 I9c744b5ace10604d5a814e6218ca0d83c796db80 about the last two points.) - As an unintended consequence, the upper limit check of cache-size of write-behind, for which a conventional hard coded limit is specified, is defeated. What we do about it: - Remove the special casing clause for cache-size in xlator_option_validate_sizet. Thus the general range check policy (as described above) will apply to cache-size too. - To implement a lower bound only check by the validator for cache-size of io-cache and quick-read, change the max attribute of these options to INFINITY. The only behavioral difference is the omission of the warnings about cache-size of io-cache and quick-read exceeding the former max values. (They were rather heuristic anyway.) > BUG: 1445609 > Change-Id: I0bd8bd391fa7d926f76e214a2178833fe4673b4a > Signed-off-by: Csaba Henk > Reviewed-on: https://review.gluster.org/17125 > Smoke: Gluster Build System > Reviewed-by: Amar Tumballi > Tested-by: Raghavendra G > NetBSD-regression: NetBSD Build System > CentOS-regression: Gluster Build System BUG: 1297743 Change-Id: Idd758c3ed47f0b50706d3becce281593005841ce Signed-off-by: Csaba Henk Reviewed-on: https://code.engineering.redhat.com/gerrit/105675 Reviewed-by: Atin Mukherjee --- libglusterfs/src/options.c | 23 +++++++---------------- xlators/performance/io-cache/src/io-cache.c | 3 ++- xlators/performance/quick-read/src/quick-read.c | 3 ++- 3 files changed, 11 insertions(+), 18 deletions(-) diff --git a/libglusterfs/src/options.c b/libglusterfs/src/options.c index 53bd779..02928a9 100644 --- a/libglusterfs/src/options.c +++ b/libglusterfs/src/options.c @@ -152,22 +152,13 @@ xlator_option_validate_sizet (xlator_t *xl, const char *key, const char *value, } if ((size < opt->min) || (size > opt->max)) { - if ((strncmp (key, "cache-size", 10) == 0) && - (size > opt->max)) { - snprintf (errstr, 256, "Cache size %" GF_PRI_SIZET " is out of " - "range [%.0f - %.0f]", - size, opt->min, opt->max); - gf_msg (xl->name, GF_LOG_WARNING, 0, - LG_MSG_OUT_OF_RANGE, "%s", errstr); - } else { - snprintf (errstr, 256, - "'%" GF_PRI_SIZET "' in 'option %s %s' " - "is out of range [%.0f - %.0f]", - size, key, value, opt->min, opt->max); - gf_msg (xl->name, GF_LOG_ERROR, 0, - LG_MSG_OUT_OF_RANGE, "%s", errstr); - ret = -1; - } + snprintf (errstr, 256, + "'%" GF_PRI_SIZET "' in 'option %s %s' " + "is out of range [%.0f - %.0f]", + size, key, value, opt->min, opt->max); + gf_msg (xl->name, GF_LOG_ERROR, 0, + LG_MSG_OUT_OF_RANGE, "%s", errstr); + ret = -1; } out: diff --git a/xlators/performance/io-cache/src/io-cache.c b/xlators/performance/io-cache/src/io-cache.c index 98c3774..9de6ee1 100644 --- a/xlators/performance/io-cache/src/io-cache.c +++ b/xlators/performance/io-cache/src/io-cache.c @@ -8,6 +8,7 @@ cases as published by the Free Software Foundation. */ +#include #include "glusterfs.h" #include "logging.h" #include "dict.h" @@ -2155,7 +2156,7 @@ struct volume_options options[] = { { .key = {"cache-size"}, .type = GF_OPTION_TYPE_SIZET, .min = 4 * GF_UNIT_MB, - .max = 32 * GF_UNIT_GB, + .max = INFINITY, .default_value = "32MB", .description = "Size of the read cache." }, diff --git a/xlators/performance/quick-read/src/quick-read.c b/xlators/performance/quick-read/src/quick-read.c index 4e9c6dc..8edf495 100644 --- a/xlators/performance/quick-read/src/quick-read.c +++ b/xlators/performance/quick-read/src/quick-read.c @@ -8,6 +8,7 @@ cases as published by the Free Software Foundation. */ +#include #include "quick-read.h" #include "statedump.h" #include "quick-read-messages.h" @@ -1144,7 +1145,7 @@ struct volume_options options[] = { { .key = {"cache-size"}, .type = GF_OPTION_TYPE_SIZET, .min = 0, - .max = 32 * GF_UNIT_GB, + .max = INFINITY, .default_value = "128MB", .description = "Size of the read cache." }, -- 1.8.3.1