From b9450b532ab7ad23ec6e2c22ed7cf55e6e1cc4c0 Mon Sep 17 00:00:00 2001 From: Xiaoxi Chen Date: Wed, 20 Aug 2014 15:35:44 +0800 Subject: [PATCH] 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 (cherry picked from commit 78e84f34da83abf5a62ae97bb84ab70774b164a6) Conflicts: src/test/erasure-code/TestErasureCodeIsa.cc Fixes: #9675 --- src/crush/CrushWrapper.cc | 21 +++--- src/crush/CrushWrapper.h | 6 +- src/crush/builder.c | 4 +- src/crush/crush.h | 2 + .../erasure-code/TestErasureCodeJerasure.cc | 30 -------- src/test/mon/osd-crush.sh | 69 +++++++++++++++++-- 6 files changed, 82 insertions(+), 50 deletions(-) diff --git a/src/crush/CrushWrapper.cc b/src/crush/CrushWrapper.cc index 31da4f5217b24..083cba88dabec 100644 --- a/src/crush/CrushWrapper.cc +++ b/src/crush/CrushWrapper.cc @@ -778,20 +778,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") @@ -810,7 +808,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 d5d4f4f875671..1024f83e4ec34 100644 --- a/src/crush/CrushWrapper.h +++ b/src/crush/CrushWrapper.h @@ -880,9 +880,9 @@ public: bool ruleset_exists(int 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 eff0bf63a52da..ab0f08626efa4 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/TestErasureCodeJerasure.cc b/src/test/erasure-code/TestErasureCodeJerasure.cc index 5c637dae831be..4b768a80ae2eb 100644 --- a/src/test/erasure-code/TestErasureCodeJerasure.cc +++ b/src/test/erasure-code/TestErasureCodeJerasure.cc @@ -288,36 +288,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: -- 2.39.5