From cb807b00060e998fc2051503fcab65f7d1595bb2 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Thu, 3 Aug 2017 18:05:08 -0400 Subject: [PATCH] crush: fix preservation of shadow bucket ids 1- a decompiled and recompiled was parsing the class bucket ids but it wasn't actually using them. 2- rebuild_roots_with_classes() was throwing out the old ids and assigning new ids when the tree was rebuilt. Fix by passing in a (potentially partial) class_bucket map into populate_classes(). Take care to allocate new bucket ids that don't collide with previously used ids. Signed-off-by: Sage Weil --- src/crush/CrushCompiler.cc | 4 +-- src/crush/CrushCompiler.h | 1 + src/crush/CrushWrapper.cc | 51 ++++++++++++++++++++++++++-------- src/crush/CrushWrapper.h | 10 +++++-- src/test/crush/CrushWrapper.cc | 32 ++++++++++++++++----- 5 files changed, 75 insertions(+), 23 deletions(-) diff --git a/src/crush/CrushCompiler.cc b/src/crush/CrushCompiler.cc index d59fdaaff5cde..89253fb20dbb9 100644 --- a/src/crush/CrushCompiler.cc +++ b/src/crush/CrushCompiler.cc @@ -734,7 +734,7 @@ int CrushCompiler::parse_bucket(iter_t const& i) } for (auto &i : class_id) - crush.class_bucket[id][i.first] = i.second; + class_bucket[id][i.first] = i.second; if (verbose) err << "bucket " << name << " (" << id << ") " << size << " items and weight " << (float)bucketweight / (float)0x10000 << std::endl; @@ -1084,7 +1084,7 @@ int CrushCompiler::parse_crush(iter_t const& i) case crush_grammar::_crushrule: if (!saw_rule) { saw_rule = true; - crush.populate_classes(); + crush.populate_classes(class_bucket); } r = parse_rule(p); break; diff --git a/src/crush/CrushCompiler.h b/src/crush/CrushCompiler.h index 7bfd259953239..f035085e70ecb 100644 --- a/src/crush/CrushCompiler.h +++ b/src/crush/CrushCompiler.h @@ -53,6 +53,7 @@ class CrushCompiler { map item_weight; map type_id; map rule_id; + std::map > class_bucket; // bucket id -> class id -> shadow bucket id string string_node(node_t &node); int int_node(node_t &node); diff --git a/src/crush/CrushWrapper.cc b/src/crush/CrushWrapper.cc index 37d6f8a18a9a8..26e4857e87ae1 100644 --- a/src/crush/CrushWrapper.cc +++ b/src/crush/CrushWrapper.cc @@ -1398,8 +1398,16 @@ bool CrushWrapper::class_is_in_use(int class_id, ostream *ss) return true; } -int CrushWrapper::populate_classes() +int CrushWrapper::populate_classes( + const std::map>& old_class_bucket) { + // build set of previous used shadow ids + set used_ids; + for (auto& p : old_class_bucket) { + for (auto& q : p.second) { + used_ids.insert(q.second); + } + } set roots; find_nonshadow_roots(roots); for (auto &r : roots) { @@ -1407,7 +1415,8 @@ int CrushWrapper::populate_classes() continue; for (auto &c : class_name) { int clone; - int res = device_class_clone(r, c.first, &clone); + int res = device_class_clone(r, c.first, old_class_bucket, used_ids, + &clone); if (res < 0) return res; } @@ -1856,7 +1865,11 @@ int CrushWrapper::remove_device_class(CephContext *cct, int id, ostream *ss) return 0; } -int CrushWrapper::device_class_clone(int original_id, int device_class, int *clone) +int CrushWrapper::device_class_clone( + int original_id, int device_class, + const std::map>& old_class_bucket, + const std::set& used_ids, + int *clone) { const char *item_name = get_item_name(original_id); if (item_name == NULL) @@ -1870,15 +1883,13 @@ int CrushWrapper::device_class_clone(int original_id, int device_class, int *clo return 0; } crush_bucket *original = get_bucket(original_id); - if (IS_ERR(original)) - return -ENOENT; + assert(!IS_ERR(original)); crush_bucket *copy = crush_make_bucket(crush, original->alg, original->hash, original->type, 0, NULL, NULL); - if(copy == NULL) - return -ENOMEM; + assert(copy); for (unsigned i = 0; i < original->size; i++) { int item = original->items[i]; int weight = crush_get_bucket_item_weight(original, i); @@ -1890,20 +1901,34 @@ int CrushWrapper::device_class_clone(int original_id, int device_class, int *clo } } else { int child_copy_id; - int res = device_class_clone(item, device_class, &child_copy_id); + int res = device_class_clone(item, device_class, old_class_bucket, + used_ids, &child_copy_id); if (res < 0) return res; crush_bucket *child_copy = get_bucket(child_copy_id); - if (IS_ERR(child_copy)) - return -ENOENT; + assert(!IS_ERR(child_copy)); res = bucket_add_item(copy, child_copy_id, child_copy->weight); if (res) return res; } } - int res = crush_add_bucket(crush, 0, copy, clone); + int bno = 0; + if (old_class_bucket.count(original_id) && + old_class_bucket.at(original_id).count(device_class)) { + bno = old_class_bucket.at(original_id).at(device_class); + } else { + // pick a new shadow bucket id that is not used by the current map + // *or* any previous shadow buckets. + bno = -1; + while (crush->buckets[-1-bno] || + used_ids.count(bno)) { + --bno; + } + } + int res = crush_add_bucket(crush, bno, copy, clone); if (res) return res; + assert(!bno || bno == *clone); res = set_item_class(*clone, device_class); if (res < 0) return res; @@ -1917,10 +1942,12 @@ int CrushWrapper::device_class_clone(int original_id, int device_class, int *clo int CrushWrapper::rebuild_roots_with_classes() { + std::map > old_class_bucket = class_bucket; int r = trim_roots_with_class(false); if (r < 0) return r; - r = populate_classes(); + class_bucket.clear(); + r = populate_classes(old_class_bucket); if (r < 0) return r; return trim_roots_with_class(true); diff --git a/src/crush/CrushWrapper.h b/src/crush/CrushWrapper.h index 94730d53d1963..5cb0e795cf351 100644 --- a/src/crush/CrushWrapper.h +++ b/src/crush/CrushWrapper.h @@ -65,6 +65,7 @@ public: std::map type_map; /* bucket/device type names */ std::map name_map; /* bucket/device names */ std::map rule_name_map; + std::map class_map; /* item id -> class id */ std::map class_name; /* class id -> class name */ std::map class_rname; /* class name -> class id */ @@ -1208,9 +1209,14 @@ public: int update_device_class(int id, const string& class_name, const string& name, ostream *ss); int remove_device_class(CephContext *cct, int id, ostream *ss); - int device_class_clone(int original, int device_class, int *clone); + int device_class_clone( + int original, int device_class, + const std::map>& old_class_bucket, + const std::set& used_ids, + int *clone); bool class_is_in_use(int class_id, ostream *ss = nullptr); - int populate_classes(); + int populate_classes( + const std::map>& old_class_bucket); int rebuild_roots_with_classes(); /* remove unused roots generated for class devices */ int trim_roots_with_class(bool unused); diff --git a/src/test/crush/CrushWrapper.cc b/src/test/crush/CrushWrapper.cc index fd85d55bd8a69..7929bffc4291e 100644 --- a/src/test/crush/CrushWrapper.cc +++ b/src/test/crush/CrushWrapper.cc @@ -1110,7 +1110,11 @@ TEST(CrushWrapper, trim_roots_with_class) { int root_id = c.get_item_id("default"); int clone_id; - ASSERT_EQ(c.device_class_clone(root_id, cl, &clone_id), 0); + map> old_class_bucket; + set used_ids; + + ASSERT_EQ(c.device_class_clone(root_id, cl, old_class_bucket, used_ids, + &clone_id), 0); ASSERT_TRUE(c.name_exists("default")); ASSERT_TRUE(c.name_exists("default~ssd")); @@ -1144,9 +1148,12 @@ TEST(CrushWrapper, device_class_clone) { c.reweight(g_ceph_context); + map> old_class_bucket; + set used_ids; int root_id = c.get_item_id("default"); int clone_id; - ASSERT_EQ(c.device_class_clone(root_id, cl, &clone_id), 0); + ASSERT_EQ(c.device_class_clone(root_id, cl, old_class_bucket, used_ids, + &clone_id), 0); ASSERT_TRUE(c.name_exists("default~ssd")); ASSERT_EQ(clone_id, c.get_item_id("default~ssd")); ASSERT_TRUE(c.subtree_contains(clone_id, item)); @@ -1156,11 +1163,14 @@ TEST(CrushWrapper, device_class_clone) { ASSERT_EQ(c.get_item_weightf(clone_id), 1); // cloning again does nothing and returns the existing one int other_clone_id; - ASSERT_EQ(c.device_class_clone(root_id, cl, &other_clone_id), 0); + ASSERT_EQ(c.device_class_clone(root_id, cl, old_class_bucket, used_ids, + &other_clone_id), 0); ASSERT_EQ(clone_id, other_clone_id); // invalid arguments - ASSERT_EQ(c.device_class_clone(12345, cl, &other_clone_id), -ECHILD); - ASSERT_EQ(c.device_class_clone(root_id, 12345, &other_clone_id), -EBADF); + ASSERT_EQ(c.device_class_clone(12345, cl, old_class_bucket, used_ids, + &other_clone_id), -ECHILD); + ASSERT_EQ(c.device_class_clone(root_id, 12345, old_class_bucket, used_ids, + &other_clone_id), -EBADF); } TEST(CrushWrapper, split_id_class) { @@ -1177,9 +1187,12 @@ TEST(CrushWrapper, split_id_class) { int class_id = c.get_or_create_class_id("ssd"); c.class_map[item] = class_id; + map> old_class_bucket; + set used_ids; int item_id = c.get_item_id("default"); int clone_id; - ASSERT_EQ(c.device_class_clone(item_id, class_id, &clone_id), 0); + ASSERT_EQ(c.device_class_clone(item_id, class_id, old_class_bucket, used_ids, + &clone_id), 0); int retrieved_item_id; int retrieved_class_id; ASSERT_EQ(c.split_id_class(clone_id, &retrieved_item_id, &retrieved_class_id), 0); @@ -1205,10 +1218,15 @@ TEST(CrushWrapper, populate_and_cleanup_classes) { int class_id = c.get_or_create_class_id("ssd"); c.class_map[item] = class_id; - ASSERT_EQ(c.populate_classes(), 0); + map> old_class_bucket; + ASSERT_EQ(c.populate_classes(old_class_bucket), 0); ASSERT_TRUE(c.name_exists("default~ssd")); + old_class_bucket = c.class_bucket; + ASSERT_EQ(c.populate_classes(old_class_bucket), 0); + ASSERT_EQ(old_class_bucket, c.class_bucket); + c.class_bucket.clear(); ASSERT_EQ(c.cleanup_classes(), 0); ASSERT_FALSE(c.name_exists("default~ssd")); -- 2.39.5