crushtool: do not dump core with non-unique bucket IDs

Return -EEXIST on duplicate ID
BUG FIX: crush_add_bucket() mixes error returns and IDs
  Add optional argument to return generated ID

Fixes: #6246
Signed-off-by: David Zafman <david.zafman@inktank.com>
Reviewed-by: Sage Weil <sage@inktank.com>

diff --git a/src/crush/CrushCompiler.cc b/src/crush/CrushCompiler.cc
index aee621d8e32d0e42af2a94c1b0bb18ddf74942fe..5f92bf7e4ecd0f65258e98af67ebd93548c02a91 100644 (file)
--- a/src/crush/CrushCompiler.cc
+++ b/src/crush/CrushCompiler.cc
@@ -527,9 +527,17 @@ int CrushCompiler::parse_bucket(iter_t const& i)
   item_id[name] = id;
   item_weight[id] = bucketweight;
   
-  crush.add_bucket(id, alg, hash, type, size, &items[0], &weights[0]);
-  crush.set_item_name(id, name.c_str());
-  return 0;
+  assert(id != 0);
+  int r = crush.add_bucket(id, alg, hash, type, size, &items[0], &weights[0], NULL);
+  if (r < 0) {
+    if (r == -EEXIST)
+      err << "Duplicate bucket id " << id << std::endl;
+    else
+      err << "add_bucket failed " << strerror(-r) << std::endl;
+    return r;
+  }
+  r = crush.set_item_name(id, name.c_str());
+  return r;
 }
 
 int CrushCompiler::parse_rule(iter_t const& i)

diff --git a/src/crush/CrushWrapper.cc b/src/crush/CrushWrapper.cc
index e96e6123aaba17c070547d72b550fd45331fcde7..bab9f9a817e38a655cf7f7c5078327871f570885 100644 (file)
--- a/src/crush/CrushWrapper.cc
+++ b/src/crush/CrushWrapper.cc
@@ -363,9 +363,15 @@ int CrushWrapper::insert_item(CephContext *cct, int item, float weight, string n
 
     if (!name_exists(q->second)) {
       ldout(cct, 5) << "insert_item creating bucket " << q->second << dendl;
-      int empty = 0;
-      cur = add_bucket(0, CRUSH_BUCKET_STRAW, CRUSH_HASH_DEFAULT, p->first, 1, &cur, &empty);
-      set_item_name(cur, q->second);
+      int empty = 0, newid;
+      int r = add_bucket(0, CRUSH_BUCKET_STRAW, CRUSH_HASH_DEFAULT, p->first, 1, &cur, &empty, &newid);
+      if (r < 0) {
+        char buf[128]; 
+        ldout(cct, 1) << "add_bucket failure error: " << strerror_r(-r, buf, sizeof(buf)) << dendl;
+        return r;
+      }
+      set_item_name(newid, q->second);
+      cur = newid;
       continue;
     }
 

diff --git a/src/crush/CrushWrapper.h b/src/crush/CrushWrapper.h
index 3d07a281956ed7830931e3ad5a962cf6f83f1cd8..80906e4fe180345856661c1238cc8154ab822505 100644 (file)
--- a/src/crush/CrushWrapper.h
+++ b/src/crush/CrushWrapper.h
@@ -720,10 +720,10 @@ public:
 
   /* modifiers */
   int add_bucket(int bucketno, int alg, int hash, int type, int size,
-                int *items, int *weights) {
+                int *items, int *weights, int *idout) {
     crush_bucket *b = crush_make_bucket(alg, hash, type, size, items, weights);
     assert(b);
-    return crush_add_bucket(crush, bucketno, b);
+    return crush_add_bucket(crush, bucketno, b, idout);
   }
   
   void finalize() {

diff --git a/src/crush/builder.c b/src/crush/builder.c
index 2eb6ff5fc1e5531eed41d7177bbe527b1b537b32..9bfde0bd8e2597e446d6a8fe5cfb04d8aef68459 100644 (file)
--- a/src/crush/builder.c
+++ b/src/crush/builder.c
@@ -123,7 +123,8 @@ int crush_get_next_bucket_id(struct crush_map *map)
 
 int crush_add_bucket(struct crush_map *map,
                     int id,
-                    struct crush_bucket *bucket)
+                    struct crush_bucket *bucket,
+                    int *idout)
 {
        int pos;
 
@@ -148,13 +149,16 @@ int crush_add_bucket(struct crush_map *map,
                memset(map->buckets + oldsize, 0, (map->max_buckets-oldsize) * sizeof(map->buckets[0]));
        }
 
-       assert(map->buckets[pos] == 0);
+       if (map->buckets[pos] != 0) {
+               return -EEXIST;
+       }
 
         /* add it */
        bucket->id = id;
        map->buckets[pos] = bucket;
 
-       return id;
+       if (idout) *idout = id;
+       return 0;
 }
 
 int crush_remove_bucket(struct crush_map *map, struct crush_bucket *bucket)

diff --git a/src/crush/builder.h b/src/crush/builder.h
index 7d30c8823433371d96af082c54c8d17a2165c73d..1003c353e6075496b94b913224d6c3e8ec7039b2 100644 (file)
--- a/src/crush/builder.h
+++ b/src/crush/builder.h
@@ -15,7 +15,7 @@ extern int crush_add_rule(struct crush_map *map, struct crush_rule *rule, int ru
 extern int crush_get_next_bucket_id(struct crush_map *map);
 extern int crush_add_bucket(struct crush_map *map,
                            int bucketno,
-                           struct crush_bucket *bucket);
+                           struct crush_bucket *bucket, int *idout);
 struct crush_bucket *crush_make_bucket(int alg, int hash, int type, int size, int *items, int *weights);
 extern int crush_bucket_add_item(struct crush_bucket *bucket, int item, int weight);
 extern int crush_bucket_adjust_item_weight(struct crush_bucket *bucket, int item, int weight);

diff --git a/src/mon/OSDMonitor.cc b/src/mon/OSDMonitor.cc
index 03f70a05a3588625a150528d21f5b9771404f0be..5450532e46d1f8eb3b6f001dbca84005a4d38d0c 100644 (file)
--- a/src/mon/OSDMonitor.cc
+++ b/src/mon/OSDMonitor.cc
@@ -2699,9 +2699,15 @@ bool OSDMonitor::prepare_command(MMonCommand *m)
       err = -EINVAL;
       goto reply;
     }
-    int bucketno = newcrush.add_bucket(0, CRUSH_BUCKET_STRAW,
+    int bucketno;
+    err = newcrush.add_bucket(0, CRUSH_BUCKET_STRAW,
                                       CRUSH_HASH_DEFAULT, type, 0, NULL,
-                                      NULL);
+                                      NULL, &bucketno);
+    if (err < 0) {
+      char buf[128];
+      ss << "add_bucket error: '" << strerror_r(-err, buf, sizeof(buf)) << "'";
+      goto reply;
+    }
     err = newcrush.set_item_name(bucketno, name);
     if (err < 0) {
       ss << "error setting bucket name to '" << name << "'";

diff --git a/src/osd/OSDMap.cc b/src/osd/OSDMap.cc
index 4b35b0c48ea24759c8ea4672dfa7baeae08fed97..8007d613b8c97c71d2e5950366914459068af822 100644 (file)
--- a/src/osd/OSDMap.cc
+++ b/src/osd/OSDMap.cc
@@ -1845,7 +1845,9 @@ void OSDMap::build_simple_crush_map(CephContext *cct, CrushWrapper& crush,
   crush.set_type_name(6, "root");
 
   // root
-  int rootid = crush.add_bucket(0, CRUSH_BUCKET_STRAW, CRUSH_HASH_DEFAULT, 6 /* pool */, 0, NULL, NULL);
+  int rootid;
+  int r = crush.add_bucket(0, CRUSH_BUCKET_STRAW, CRUSH_HASH_DEFAULT, 6 /* pool */, 0, NULL, NULL, &rootid);
+  assert(r == 0);
   crush.set_item_name(rootid, "default");
 
   for (int o=0; o<nosd; o++) {
@@ -1975,7 +1977,9 @@ void OSDMap::build_simple_crush_map_from_conf(CephContext *cct, CrushWrapper& cr
   set<string> hosts, racks;
 
   // root
-  int rootid = crush.add_bucket(0, CRUSH_BUCKET_STRAW, CRUSH_HASH_DEFAULT, 6 /* pool */, 0, NULL, NULL);
+  int rootid;
+  int r = crush.add_bucket(0, CRUSH_BUCKET_STRAW, CRUSH_HASH_DEFAULT, 6 /* pool */, 0, NULL, NULL, &rootid);
+  assert(r == 0);
   crush.set_item_name(rootid, "default");
 
   // add osds

diff --git a/src/tools/crushtool.cc b/src/tools/crushtool.cc
index 75c26c098b66768a697a7dac8a50253383a1935c..03c83f2415673853e5f4740092898d5428fdbe25 100644 (file)
--- a/src/tools/crushtool.cc
+++ b/src/tools/crushtool.cc
@@ -569,7 +569,11 @@ int main(int argc, const char **argv)
 
        crush_bucket *b = crush_make_bucket(buckettype, CRUSH_HASH_DEFAULT, type, j, items, weights);
        assert(b);
-       int id = crush_add_bucket(crush.crush, 0, b);
+       int id;
+       int r = crush_add_bucket(crush.crush, 0, b, &id);
+       if (r < 0) {
+               dout(0) << "Couldn't add root bucket: " << strerror(-r) << dendl;
+       }
        rootid = id;
 
        char format[20];