From 91e1b6fad4c51bf5f8757e8d4b8f3cee5fe959ab Mon Sep 17 00:00:00 2001
From: Sage Weil <sage@redhat.com>
Date: Fri, 18 May 2018 13:11:57 -0500
Subject: [PATCH] crush: update choose_args on bucket removal

The specific bug I see is that a bucket no longer exists but its
choose_args still does.  However, I'm also taking the opportunity to
verify that the choose_args agrees with the bucket sizes and position
counts everywhere else, too.  Check for

- ids or weight_sets for buckets that don't exist or aren't straw2
- weight_set_positions that don't match the choose_args
  - don't fix this, just warn.  i'm not sure how it would happen.  :/
- weight_set sizes that don't match the bucket size

Fixes: http://tracker.ceph.com/issues/24167
Signed-off-by: Sage Weil <sage@redhat.com>
(cherry picked from commit a75ffcd17cc171a383315ec9738865de6c455086)
---
 src/crush/CrushWrapper.cc | 47 +++++++++++++--------------------------
 1 file changed, 15 insertions(+), 32 deletions(-)

diff --git a/src/crush/CrushWrapper.cc b/src/crush/CrushWrapper.cc
index 29b81cdbeb859..ace0bd3e49a64 100644
--- a/src/crush/CrushWrapper.cc
+++ b/src/crush/CrushWrapper.cc
@@ -434,46 +434,29 @@ void CrushWrapper::update_choose_args(CephContext *cct)
 	  carg.weight_set_positions = 0;
 	}
 	continue;
-      } else if (positions != carg.weight_set_positions) {
-	// missing/mismatched positions?
+      }
+      if (carg.weight_set_positions == 0) {
+	continue;	// skip it
+      }
+      if (carg.weight_set_positions != positions) {
 	if (cct)
-	  ldout(cct,0) << __func__ << " fixing " << i.first << " bucket "
-		       << (-1-j) << " positions " << carg.weight_set_positions
-		       << " -> " << positions << dendl;
-	auto old_pos = carg.weight_set_positions;
-	auto old_ws = carg.weight_set;
-	carg.weight_set_positions = positions;
-	carg.weight_set = static_cast<crush_weight_set*>(
-	  calloc(sizeof(crush_weight_set), positions));
-	for (unsigned p = 0; p < positions; ++p) {
-	  carg.weight_set[p].size = b->size;
-	  carg.weight_set[p].weights = (__u32*)calloc(b->size, sizeof(__u32));
-	  if (old_ws && old_pos < p) {
-	    auto max = std::max<unsigned>(old_ws[p].size, b->size);
-	    for (unsigned k = 0; k < max; ++k) {
-	      carg.weight_set[p].weights[k] = old_ws[p].weights[k];
-	    }
-	  }
-	}
-	if (old_ws) {
-	  for (unsigned p = 0; p < old_pos; ++p) {
-	    free(old_ws[p].weights);
-	  }
-	  free(old_ws);
-	}
+	  lderr(cct) << __func__ << " " << i.first << " bucket "
+		     << (-1-j) << " positions " << carg.weight_set_positions
+		     << " -> " << positions << dendl;
+	continue;	// wth... skip!
       }
-      // mis-sized weight_sets?
+      // mis-sized weight_sets?  this shouldn't ever happen.
       for (unsigned p = 0; p < positions; ++p) {
 	if (carg.weight_set[p].size != b->size) {
 	  if (cct)
-	    ldout(cct,0) << __func__ << " fixing " << i.first << " bucket "
-			 << (-1-j) << " position " << p
-			 << " size " << carg.weight_set[p].size << " -> "
-			 << b->size << dendl;
+	    lderr(cct) << __func__ << " fixing " << i.first << " bucket "
+		       << (-1-j) << " position " << p
+		       << " size " << carg.weight_set[p].size << " -> "
+		       << b->size << dendl;
 	  auto old_ws = carg.weight_set[p];
 	  carg.weight_set[p].size = b->size;
 	  carg.weight_set[p].weights = (__u32*)calloc(b->size, sizeof(__u32));
-	  auto max = std::max<unsigned>(old_ws.size, b->size);
+	  auto max = std::min<unsigned>(old_ws.size, b->size);
 	  for (unsigned k = 0; k < max; ++k) {
 	    carg.weight_set[p].weights[k] = old_ws.weights[k];
 	  }
-- 
2.39.5