From ad63d4ba2c3f343ef7f5b1df4b9c549084ea2750 Mon Sep 17 00:00:00 2001 From: Carudy Date: Wed, 30 Aug 2017 10:06:54 +0800 Subject: [PATCH] crush: safe check for 'ceph osd crush swap-bucket' The swap method is just swap the two buckets'weight,son and name.So there is a problem when we swap parent and children. When swap two buckets,one should not be ancestor of the other,or there will be a loop,which would destory the structure. Signed-off-by: Carudy --- src/crush/CrushWrapper.cc | 15 +++++++++++++++ src/crush/CrushWrapper.h | 1 + src/test/crush/CrushWrapper.cc | 3 +++ 3 files changed, 19 insertions(+) diff --git a/src/crush/CrushWrapper.cc b/src/crush/CrushWrapper.cc index 8f2d63e77c8cc..edf2567537b9b 100644 --- a/src/crush/CrushWrapper.cc +++ b/src/crush/CrushWrapper.cc @@ -1035,6 +1035,18 @@ int CrushWrapper::detach_bucket(CephContext *cct, int item) return bucket_weight; } +bool CrushWrapper::is_parent_of(int child, int p) const +{ + int parent = 0; + while (!get_immediate_parent_id(child, &parent)) { + if (parent == p) { + return true; + } + child = parent; + } + return false; +} + int CrushWrapper::swap_bucket(CephContext *cct, int src, int dst) { if (src >= 0 || dst >= 0) @@ -1043,6 +1055,9 @@ int CrushWrapper::swap_bucket(CephContext *cct, int src, int dst) return -EINVAL; crush_bucket *a = get_bucket(src); crush_bucket *b = get_bucket(dst); + if (is_parent_of(a->id, b->id) || is_parent_of(b->id, a->id)) { + return -EINVAL; + } unsigned aw = a->weight; unsigned bw = b->weight; diff --git a/src/crush/CrushWrapper.h b/src/crush/CrushWrapper.h index 737ba583ecece..27d5c20bc916f 100644 --- a/src/crush/CrushWrapper.h +++ b/src/crush/CrushWrapper.h @@ -627,6 +627,7 @@ private: * @return true if present */ bool _search_item_exists(int i) const; + bool is_parent_of(int child, int p) const; public: /** diff --git a/src/test/crush/CrushWrapper.cc b/src/test/crush/CrushWrapper.cc index 4502d0d0e69b3..efd70889b6fc4 100644 --- a/src/test/crush/CrushWrapper.cc +++ b/src/test/crush/CrushWrapper.cc @@ -174,6 +174,9 @@ TEST(CrushWrapper, swap_bucket) { ASSERT_EQ(1, c->get_bucket_item(a, 1)); ASSERT_EQ(2, c->get_bucket_item(a, 2)); ASSERT_EQ(3, c->get_bucket_item(b, 0)); + + // check if it can swap parent with child + ASSERT_EQ(-EINVAL, c->swap_bucket(g_ceph_context, root, a)); c->swap_bucket(g_ceph_context, a, b); ASSERT_EQ(0x30000, c->get_item_weight(b)); -- 2.39.5