From: Xiaoxi Chen Date: Wed, 20 Aug 2014 07:35:44 +0000 (+0800) Subject: CrushWrapper: pick a ruleset same as rule_id X-Git-Tag: v0.86~228^2 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=78e84f34da83abf5a62ae97bb84ab70774b164a6;p=ceph.git CrushWrapper: pick a ruleset same as rule_id Originally in the add_simple_ruleset funtion, the ruleset_id is not reused but rule_id is reused. So after some add/remove against rules, the newly created rule likely to have ruleset!=rule_id. We dont want this happen because we are trying to hold the constraint that ruleset == rule_id. Signed-off-by: Xiaoxi Chen --- diff --git a/src/crush/CrushWrapper.cc b/src/crush/CrushWrapper.cc index 8965f8bf69a07..9618d982e3fef 100644 --- a/src/crush/CrushWrapper.cc +++ b/src/crush/CrushWrapper.cc @@ -805,20 +805,18 @@ int CrushWrapper::add_simple_ruleset(string name, string root_name, return -EINVAL; } - int ruleset = 0; - for (int i = 0; i < get_max_rules(); i++) { - if (rule_exists(i) && - get_rule_mask_ruleset(i) >= ruleset) { - ruleset = get_rule_mask_ruleset(i) + 1; - } + int rno = -1; + for (rno = 0; rno < get_max_rules(); rno++) { + if (!rule_exists(rno) && !ruleset_exists(rno)) + break; } - int steps = 3; if (mode == "indep") steps = 4; int min_rep = mode == "firstn" ? 1 : 3; int max_rep = mode == "firstn" ? 10 : 20; - crush_rule *rule = crush_make_rule(steps, ruleset, rule_type, min_rep, max_rep); + //set the ruleset the same as rule_id(rno) + crush_rule *rule = crush_make_rule(steps, rno, rule_type, min_rep, max_rep); assert(rule); int step = 0; if (mode == "indep") @@ -837,7 +835,12 @@ int CrushWrapper::add_simple_ruleset(string name, string root_name, CRUSH_CHOOSE_N, 0); crush_rule_set_step(rule, step++, CRUSH_RULE_EMIT, 0, 0); - int rno = crush_add_rule(crush, rule, -1); + + int ret = crush_add_rule(crush, rule, rno); + if(ret < 0) { + *err << "failed to add rule " << rno << " because " << cpp_strerror(ret); + return ret; + } set_rule_name(rno, name); have_rmaps = false; return rno; diff --git a/src/crush/CrushWrapper.h b/src/crush/CrushWrapper.h index d6e4fa43c6db9..56e3d16a0539f 100644 --- a/src/crush/CrushWrapper.h +++ b/src/crush/CrushWrapper.h @@ -885,9 +885,9 @@ public: bool ruleset_exists(int const ruleset) const { for (size_t i = 0; i < crush->max_rules; ++i) { - if (crush->rules[i]->mask.ruleset == ruleset) { - return true; - } + if (rule_exists(i) && crush->rules[i]->mask.ruleset == ruleset) { + return true; + } } return false; diff --git a/src/crush/builder.c b/src/crush/builder.c index c319620860205..41b90aabf4452 100644 --- a/src/crush/builder.c +++ b/src/crush/builder.c @@ -63,7 +63,7 @@ int crush_add_rule(struct crush_map *map, struct crush_rule *rule, int ruleno) for (r=0; r < map->max_rules; r++) if (map->rules[r] == 0) break; - assert(r <= INT_MAX); + assert(r < CRUSH_MAX_RULES); } else r = ruleno; @@ -72,6 +72,8 @@ int crush_add_rule(struct crush_map *map, struct crush_rule *rule, int ruleno) /* expand array */ int oldsize; void *_realloc = NULL; + if (map->max_rules +1 > CRUSH_MAX_RULES) + return -ENOSPC; oldsize = map->max_rules; map->max_rules = r+1; if ((_realloc = realloc(map->rules, map->max_rules * sizeof(map->rules[0]))) == NULL) { diff --git a/src/crush/crush.h b/src/crush/crush.h index 8bac92aaa9ed5..322d16c39b7b9 100644 --- a/src/crush/crush.h +++ b/src/crush/crush.h @@ -26,6 +26,8 @@ #define CRUSH_MAGIC 0x00010000ul /* for detecting algorithm revisions */ #define CRUSH_MAX_DEPTH 10 /* max crush hierarchy depth */ +#define CRUSH_MAX_RULESET (1<<8) /*max crush ruleset number*/ +#define CRUSH_MAX_RULES CRUSH_MAX_RULESET /*max crush rules, shold be the same as max rulesets*/ #define CRUSH_MAX_DEVICE_WEIGHT (100u * 0x10000u) #define CRUSH_MAX_BUCKET_WEIGHT (65535u * 0x10000u) diff --git a/src/test/erasure-code/TestErasureCodeIsa.cc b/src/test/erasure-code/TestErasureCodeIsa.cc index 93f342a20ea4a..516d26b944d3f 100644 --- a/src/test/erasure-code/TestErasureCodeIsa.cc +++ b/src/test/erasure-code/TestErasureCodeIsa.cc @@ -734,36 +734,6 @@ TEST(IsaErasureCodeTest, create_ruleset) } } - // - // The ruleid may be different from the ruleset when a crush rule is - // removed because the removed ruleid will be reused but the removed - // ruleset will not be reused. - // - // This also asserts that the create_ruleset() method returns a - // ruleset and not a ruleid http://tracker.ceph.com/issues/9044 - // - { - stringstream ss; - ErasureCodeIsaDefault isa; - map parameters; - parameters["k"] = "2"; - parameters["m"] = "2"; - parameters["w"] = "8"; - isa.init(parameters); - int FIRST = isa.create_ruleset("FIRST", *c, &ss); - int SECOND = isa.create_ruleset("SECOND", *c, &ss); - int FIRST_ruleid = c->get_rule_id("FIRST"); - EXPECT_EQ(0, c->remove_rule(FIRST_ruleid)); - int ruleset = isa.create_ruleset("myrule", *c, &ss); - EXPECT_NE(FIRST, ruleset); - EXPECT_NE(SECOND, ruleset); - EXPECT_NE(ruleset, c->get_rule_id("myrule")); - int SECOND_ruleid = c->get_rule_id("SECOND"); - EXPECT_EQ(0, c->remove_rule(SECOND_ruleid)); - int myrule_ruleid = c->get_rule_id("myrule"); - EXPECT_EQ(0, c->remove_rule(myrule_ruleid)); - } - { stringstream ss; ErasureCodeIsaDefault isa; diff --git a/src/test/erasure-code/TestErasureCodeJerasure.cc b/src/test/erasure-code/TestErasureCodeJerasure.cc index 2cf81a8ef9a4a..b7f35ac6496d4 100644 --- a/src/test/erasure-code/TestErasureCodeJerasure.cc +++ b/src/test/erasure-code/TestErasureCodeJerasure.cc @@ -293,36 +293,6 @@ TEST(ErasureCodeTest, create_ruleset) } } - // - // The ruleid may be different from the ruleset when a crush rule is - // removed because the removed ruleid will be reused but the removed - // ruleset will not be reused. - // - // This also asserts that the create_ruleset() method returns a - // ruleset and not a ruleid http://tracker.ceph.com/issues/9044 - // - { - stringstream ss; - ErasureCodeJerasureReedSolomonVandermonde jerasure; - map parameters; - parameters["k"] = "2"; - parameters["m"] = "2"; - parameters["w"] = "8"; - jerasure.init(parameters); - int FIRST = jerasure.create_ruleset("FIRST", *c, &ss); - int SECOND = jerasure.create_ruleset("SECOND", *c, &ss); - int FIRST_ruleid = c->get_rule_id("FIRST"); - EXPECT_EQ(0, c->remove_rule(FIRST_ruleid)); - int ruleset = jerasure.create_ruleset("myrule", *c, &ss); - EXPECT_NE(FIRST, ruleset); - EXPECT_NE(SECOND, ruleset); - EXPECT_NE(ruleset, c->get_rule_id("myrule")); - int SECOND_ruleid = c->get_rule_id("SECOND"); - EXPECT_EQ(0, c->remove_rule(SECOND_ruleid)); - int myrule_ruleid = c->get_rule_id("myrule"); - EXPECT_EQ(0, c->remove_rule(myrule_ruleid)); - } - { stringstream ss; ErasureCodeJerasureReedSolomonVandermonde jerasure; diff --git a/src/test/mon/osd-crush.sh b/src/test/mon/osd-crush.sh index ceb1efb1b8222..8f39a1a923a8f 100755 --- a/src/test/mon/osd-crush.sh +++ b/src/test/mon/osd-crush.sh @@ -23,16 +23,16 @@ function run() { CEPH_ARGS+="--fsid=$(uuidgen) --auth-supported=none " CEPH_ARGS+="--mon-host=127.0.0.1 " - setup $dir || return 1 - run_mon $dir a --public-addr 127.0.0.1 FUNCTIONS=${FUNCTIONS:-$(set | sed -n -e 's/^\(TEST_[0-9a-z_]*\) .*/\1/p')} for TEST_function in $FUNCTIONS ; do - if ! $TEST_function $dir ; then - cat $dir/a/log - return 1 - fi + setup $dir || return 1 + run_mon $dir a --public-addr 127.0.0.1 + if ! $TEST_function $dir ; then + cat $dir/a/log + return 1 + fi + teardown $dir || return 1 done - teardown $dir || return 1 } function TEST_crush_rule_create_simple() { @@ -147,6 +147,61 @@ function TEST_crush_rule_create_erasure_profile_default_exists() { ./ceph osd erasure-code-profile ls | grep default || return 1 } +function Check_ruleset_id_match_rule_id() { + local rule_name=$1 + rule_id=`./ceph osd crush rule dump $rule_name | grep "\"rule_id\":" | awk -F ":|," '{print int($2)}'` + ruleset_id=`./ceph osd crush rule dump $rule_name | grep "\"ruleset\":"| awk -F ":|," '{print int($2)}'` + test $ruleset_id = $rule_id || return 1 +} + +function generate_manipulated_rules() { + local dir=$1 + ./ceph osd crush add-bucket $root host + ./ceph osd crush rule create-simple test_rule1 $root osd firstn || return 1 + ./ceph osd crush rule create-simple test_rule2 $root osd firstn || return 1 + ./ceph osd getcrushmap -o $dir/original_map + ./crushtool -d $dir/original_map -o $dir/decoded_original_map + #manipulate the rulesets , to make the rule_id != ruleset_id + sed -i 's/ruleset 0/ruleset 3/' $dir/decoded_original_map + sed -i 's/ruleset 2/ruleset 0/' $dir/decoded_original_map + sed -i 's/ruleset 1/ruleset 2/' $dir/decoded_original_map + + ./crushtool -c $dir/decoded_original_map -o $dir/new_map + ./ceph osd setcrushmap -i $dir/new_map + + ./ceph osd crush rule dump +} + +function TEST_crush_ruleset_match_rule_when_creating() { + local dir=$1 + local root=host1 + + generate_manipulated_rules $dir + + ./ceph osd crush rule create-simple special_rule_simple $root osd firstn || return 1 + + ./ceph osd crush rule dump + #show special_rule_simple has same rule_id and ruleset_id + Check_ruleset_id_match_rule_id special_rule_simple || return 1 +} + +function TEST_add_ruleset_failed() { + local dir=$1 + local root=host1 + + ./ceph osd crush add-bucket $root host + ./ceph osd crush rule create-simple test_rule1 $root osd firstn || return 1 + ./ceph osd crush rule create-simple test_rule2 $root osd firstn || return 1 + #./ceph osd crush rule rm test_rule1 + for i in `seq 3 255` + do + ./ceph osd crush rule create-simple test_rule$i $root osd firstn || return 1 + Check_ruleset_id_match_rule_id test_rule$i || return 1 + done + + ./ceph osd crush rule create-simple test_rule_nospace $root osd firstn 2>&1 | grep "Error ENOSPC" || return 1 + +} main osd-crush # Local Variables: