osd: maybe_remove_pg_upmaps -> clean_pg_upmaps

It should always be the preferred option to kill the unnecessary
or duplicated code, which is good for maintenance.
Also I've noticed there is already a clean_temps helper, so re-naming
maybe_remove_pg_upmaps to clean_pg_upmaps to at least keep pace with
that sounds to be a natural choice for me..

Master PR: https://github.com/ceph/ceph/pull/28373

This does not follow the normal backport process
since this piece of code has been changed a lot for the past 18 months..

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>

diff --git a/src/mon/OSDMonitor.cc b/src/mon/OSDMonitor.cc
index f3dd6b5665e7ff643cae94187146f60b31cee244..fb665f00c4606e53c44b5694632b6e071548e2d2 100644 (file)
--- a/src/mon/OSDMonitor.cc
+++ b/src/mon/OSDMonitor.cc
@@ -1061,6 +1061,9 @@ void OSDMonitor::encode_pending(MonitorDBStore::TransactionRef t)
     tmp.deepish_copy_from(osdmap);
     tmp.apply_incremental(pending_inc);
 
+    // clean inappropriate pg_upmap/pg_upmap_items (if any)
+    tmp.clean_pg_upmaps(cct, &pending_inc);
+
     // remove any legacy osdmap nearfull/full flags
     {
       if (tmp.test_flag(CEPH_OSDMAP_FULL | CEPH_OSDMAP_NEARFULL)) {
@@ -1350,9 +1353,6 @@ void OSDMonitor::encode_pending(MonitorDBStore::TransactionRef t)
     }
   }
 
-  // clean inappropriate pg_upmap/pg_upmap_items (if any)
-  osdmap.maybe_remove_pg_upmaps(cct, osdmap, &pending_inc);
-
   // features for osdmap and its incremental
   uint64_t features;
 

diff --git a/src/osd/OSDMap.cc b/src/osd/OSDMap.cc
index 515d3f6bba3b45262df778bb8979e9530454de08..0007a36c349964847718b86c551e89e4b1dae43e 100644 (file)
--- a/src/osd/OSDMap.cc
+++ b/src/osd/OSDMap.cc
@@ -1638,48 +1638,39 @@ void OSDMap::clean_temps(CephContext *cct,
   }
 }
 
-void OSDMap::maybe_remove_pg_upmaps(CephContext *cct,
-                                    const OSDMap& osdmap,
-                                    Incremental *pending_inc)
+bool OSDMap::clean_pg_upmaps(CephContext *cct,
+                             Incremental *pending_inc) const
 {
   ldout(cct, 10) << __func__ << dendl;
-  OSDMap tmpmap;
-  tmpmap.deepish_copy_from(osdmap);
-  tmpmap.apply_incremental(*pending_inc);
   set<pg_t> to_check;
   set<pg_t> to_cancel;
   map<int, map<int, float>> rule_weight_map;
+  bool any_change = false;
 
-  for (auto& p : tmpmap.pg_upmap) {
-    to_check.insert(p.first);
-  }
-  for (auto& p : tmpmap.pg_upmap_items) {
-    to_check.insert(p.first);
-  }
-  for (auto& p : pending_inc->new_pg_upmap) {
+  for (auto& p : pg_upmap) {
     to_check.insert(p.first);
   }
-  for (auto& p : pending_inc->new_pg_upmap_items) {
+  for (auto& p : pg_upmap_items) {
     to_check.insert(p.first);
   }
   for (auto& pg : to_check) {
-    if (!tmpmap.pg_exists(pg)) {
+    if (!pg_exists(pg)) {
       ldout(cct, 0) << __func__ << " pg " << pg << " is gone" << dendl;
       to_cancel.insert(pg);
       continue;
     }
     vector<int> raw, up;
-    tmpmap.pg_to_raw_upmap(pg, &raw, &up);
-    auto i = tmpmap.pg_upmap.find(pg);
-    if (i != tmpmap.pg_upmap.end() && raw == i->second) {
+    pg_to_raw_upmap(pg, &raw, &up);
+    auto i = pg_upmap.find(pg);
+    if (i != pg_upmap.end() && raw == i->second) {
       ldout(cct, 10) << " removing redundant pg_upmap "
                      << i->first << " " << i->second
                      << dendl;
       to_cancel.insert(pg);
       continue;
     }
-    auto j = tmpmap.pg_upmap_items.find(pg);
-    if (j != tmpmap.pg_upmap_items.end()) {
+    auto j = pg_upmap_items.find(pg);
+    if (j != pg_upmap_items.end()) {
       mempool::osdmap::vector<pair<int,int>> newmap;
       for (auto& p : j->second) {
         if (std::find(raw.begin(), raw.end(), p.first) == raw.end()) {
@@ -1705,14 +1696,15 @@ void OSDMap::maybe_remove_pg_upmaps(CephContext *cct,
                        << " -> " << newmap
                        << dendl;
         pending_inc->new_pg_upmap_items[pg] = newmap;
+        any_change = true;
         continue;
       }
     }
-    auto crush_rule = tmpmap.get_pg_pool_crush_rule(pg);
-    auto r = tmpmap.crush->verify_upmap(cct,
-                                        crush_rule,
-                                        tmpmap.get_pg_pool_size(pg),
-                                        up);
+    auto crush_rule = get_pg_pool_crush_rule(pg);
+    auto r = crush->verify_upmap(cct,
+                                 crush_rule,
+                                 get_pg_pool_size(pg),
+                                 up);
     if (r < 0) {
       ldout(cct, 0) << __func__ << " verify_upmap of pg " << pg
                     << " returning " << r
@@ -1724,7 +1716,7 @@ void OSDMap::maybe_remove_pg_upmaps(CephContext *cct,
     map<int, float> weight_map;
     auto it = rule_weight_map.find(crush_rule);
     if (it == rule_weight_map.end()) {
-      auto r = tmpmap.crush->get_rule_weight_osd_map(crush_rule, &weight_map);
+      auto r = crush->get_rule_weight_osd_map(crush_rule, &weight_map);
       if (r < 0) {
         lderr(cct) << __func__ << " unable to get crush weight_map for "
                    << "crush_rule " << crush_rule << dendl;
@@ -1744,7 +1736,7 @@ void OSDMap::maybe_remove_pg_upmaps(CephContext *cct,
         to_cancel.insert(pg);
         break;
       }
-      auto adjusted_weight = tmpmap.get_weightf(it->first) * it->second;
+      auto adjusted_weight = get_weightf(it->first) * it->second;
       if (adjusted_weight == 0) {
         // osd is out/crush-out
         to_cancel.insert(pg);
@@ -1753,42 +1745,40 @@ void OSDMap::maybe_remove_pg_upmaps(CephContext *cct,
     }
   }
   for (auto &pg: to_cancel) {
-    { // pg_upmap
-      auto it = pending_inc->new_pg_upmap.find(pg);
-      if (it != pending_inc->new_pg_upmap.end()) {
-        ldout(cct, 10) << __func__ << " cancel invalid pending "
-                       << "pg_upmap entry "
-                       << it->first << "->" << it->second
-                       << dendl;
-        pending_inc->new_pg_upmap.erase(it);
-      }
-      if (osdmap.pg_upmap.count(pg)) {
-        ldout(cct, 10) << __func__ << " cancel invalid pg_upmap entry "
-                       << osdmap.pg_upmap.find(pg)->first << "->"
-                       << osdmap.pg_upmap.find(pg)->second
-                       << dendl;
-        pending_inc->old_pg_upmap.insert(pg);
-      }
+    auto i = pending_inc->new_pg_upmap.find(pg);
+    if (i != pending_inc->new_pg_upmap.end()) {
+      ldout(cct, 10) << __func__ << " cancel invalid pending "
+                     << "pg_upmap entry "
+                     << i->first << "->" << i->second
+                     << dendl;
+      pending_inc->new_pg_upmap.erase(i);
     }
-    { // pg_upmap_items
-      auto it = pending_inc->new_pg_upmap_items.find(pg);
-      if (it != pending_inc->new_pg_upmap_items.end()) {
-        ldout(cct, 10) << __func__ << " cancel invalid pending "
-                       << "pg_upmap_items entry "
-                       << it->first << "->" << it->second
-                       << dendl;
-        pending_inc->new_pg_upmap_items.erase(it);
-      }
-      if (osdmap.pg_upmap_items.count(pg)) {
-        ldout(cct, 10) << __func__ << " cancel invalid "
-                       << "pg_upmap_items entry "
-                       << osdmap.pg_upmap_items.find(pg)->first << "->"
-                       << osdmap.pg_upmap_items.find(pg)->second
-                       << dendl;
-        pending_inc->old_pg_upmap_items.insert(pg);
-      }
+    auto j = pg_upmap.find(pg);
+    if (j != pg_upmap.end()) {
+      ldout(cct, 10) << __func__ << " cancel invalid pg_upmap entry "
+                     << j->first << "->" << j->second
+                     << dendl;
+      pending_inc->old_pg_upmap.insert(pg);
+    }
+    auto p = pending_inc->new_pg_upmap_items.find(pg);
+    if (p != pending_inc->new_pg_upmap_items.end()) {
+      ldout(cct, 10) << __func__ << " cancel invalid pending "
+                     << "pg_upmap_items entry "
+                     << p->first << "->" << p->second
+                     << dendl;
+      pending_inc->new_pg_upmap_items.erase(p);
+    }
+    auto q = pg_upmap_items.find(pg);
+    if (q != pg_upmap_items.end()) {
+      ldout(cct, 10) << __func__ << " cancel invalid "
+                     << "pg_upmap_items entry "
+                     << q->first << "->" << q->second
+                     << dendl;
+      pending_inc->old_pg_upmap_items.insert(pg);
     }
   }
+  any_change = any_change || !to_cancel.empty();
+  return any_change;
 }
 
 int OSDMap::apply_incremental(const Incremental &inc)
@@ -4092,56 +4082,6 @@ int OSDMap::summarize_mapping_stats(
   return 0;
 }
 
-
-int OSDMap::clean_pg_upmaps(
-  CephContext *cct,
-  Incremental *pending_inc) const
-{
-  ldout(cct, 10) << __func__ << dendl;
-  int changed = 0;
-  for (auto& p : pg_upmap) {
-    vector<int> raw;
-    int primary;
-    pg_to_raw_osds(p.first, &raw, &primary);
-    if (raw == p.second) {
-      ldout(cct, 10) << " removing redundant pg_upmap " << p.first << " "
-                    << p.second << dendl;
-      pending_inc->old_pg_upmap.insert(p.first);
-      ++changed;
-    }
-  }
-  for (auto& p : pg_upmap_items) {
-    vector<int> raw;
-    int primary;
-    pg_to_raw_osds(p.first, &raw, &primary);
-    mempool::osdmap::vector<pair<int,int>> newmap;
-    for (auto& q : p.second) {
-      if (std::find(raw.begin(), raw.end(), q.first) == raw.end()) {
-        // cancel mapping if source osd does not exist anymore
-        continue;
-      }
-      if (q.second != CRUSH_ITEM_NONE && q.second < max_osd &&
-          q.second >= 0 && osd_weight[q.second] == 0) {
-        // cancel mapping if target osd is out
-        continue;
-      }
-      newmap.push_back(q);
-    }
-    if (newmap.empty()) {
-      ldout(cct, 10) << " removing no-op pg_upmap_items " << p.first << " "
-                    << p.second << dendl;
-      pending_inc->old_pg_upmap_items.insert(p.first);
-      ++changed;
-    } else if (newmap != p.second) {
-      ldout(cct, 10) << " simplifying partially no-op pg_upmap_items "
-                    << p.first << " " << p.second << " -> " << newmap << dendl;
-      pending_inc->new_pg_upmap_items[p.first] = newmap;
-      ++changed;
-    }
-  }
-  return changed;
-}
-
 bool OSDMap::try_pg_upmap(
   CephContext *cct,
   pg_t pg,                       ///< pg to potentially remap

diff --git a/src/osd/OSDMap.h b/src/osd/OSDMap.h
index 8d6bd64b5f37497fce51f3f26f0c7902375cb02c..71999968147df4f8e446ac004ca2daa722f54e6e 100644 (file)
--- a/src/osd/OSDMap.h
+++ b/src/osd/OSDMap.h
@@ -1009,9 +1009,7 @@ public:
    */
   uint64_t get_up_osd_features() const;
 
-  void maybe_remove_pg_upmaps(CephContext *cct,
-                              const OSDMap& osdmap,
-                              Incremental *pending_inc);
+  bool clean_pg_upmaps(CephContext *cct, Incremental *pending_inc) const;
 
   int apply_incremental(const Incremental &inc);
 
@@ -1358,10 +1356,6 @@ public:
     return calc_pg_role(osd, group, group.size()) >= 0;
   }
 
-  int clean_pg_upmaps(
-    CephContext *cct,
-    Incremental *pending_inc) const;
-
   bool try_pg_upmap(
     CephContext *cct,
     pg_t pg,                       ///< pg to potentially remap

diff --git a/src/test/osd/TestOSDMap.cc b/src/test/osd/TestOSDMap.cc
index 7fb65921a6a64b9eb87b9f3b6f8acbdecca1e43f..dc85502d2c58862f62322c42a040898d045dc4c8 100644 (file)
--- a/src/test/osd/TestOSDMap.cc
+++ b/src/test/osd/TestOSDMap.cc
@@ -674,9 +674,9 @@ TEST_F(OSDMapTest, CleanPGUpmaps) {
       ASSERT_TRUE(tmpmap.have_pg_upmaps(ec_pgid));
     }
     {
-      // confirm *maybe_remove_pg_upmaps* won't do anything bad
+      // confirm *clean_pg_upmaps* won't do anything bad
       OSDMap::Incremental pending_inc(tmpmap.get_epoch() + 1);
-      tmpmap.maybe_remove_pg_upmaps(g_ceph_context, tmpmap, &pending_inc);
+      tmpmap.clean_pg_upmaps(g_ceph_context, &pending_inc);
       tmpmap.apply_incremental(pending_inc);
       ASSERT_TRUE(tmpmap.have_pg_upmaps(ec_pgid));
     }
@@ -724,11 +724,9 @@ TEST_F(OSDMapTest, CleanPGUpmaps) {
       ASSERT_TRUE(tmpmap.have_pg_upmaps(ec_pgid));
     }
     {
-      // *maybe_remove_pg_upmaps* should be able to remove the above *bad* mapping
+      // *clean_pg_upmaps* should be able to remove the above *bad* mapping
       OSDMap::Incremental pending_inc(tmpmap.get_epoch() + 1);
-      OSDMap nextmap;
-      nextmap.deepish_copy_from(tmpmap);
-      nextmap.maybe_remove_pg_upmaps(g_ceph_context, nextmap, &pending_inc);
+      tmpmap.clean_pg_upmaps(g_ceph_context, &pending_inc);
       tmpmap.apply_incremental(pending_inc);
       ASSERT_TRUE(!tmpmap.have_pg_upmaps(ec_pgid));
     }
@@ -850,11 +848,9 @@ TEST_F(OSDMapTest, CleanPGUpmaps) {
       ASSERT_TRUE(tmp.have_pg_upmaps(ec_pgid));
     }
     {
-      // *maybe_remove_pg_upmaps* should not remove the above upmap_item
+      // *clean_pg_upmaps* should not remove the above upmap_item
       OSDMap::Incremental pending_inc(tmp.get_epoch() + 1);
-      OSDMap nextmap;
-      nextmap.deepish_copy_from(tmp);
-      nextmap.maybe_remove_pg_upmaps(g_ceph_context, nextmap, &pending_inc);
+      tmp.clean_pg_upmaps(g_ceph_context, &pending_inc);
       tmp.apply_incremental(pending_inc);
       ASSERT_TRUE(tmp.have_pg_upmaps(ec_pgid));
     }
@@ -919,7 +915,7 @@ TEST_F(OSDMapTest, CleanPGUpmaps) {
     {
       // STEP-2: apply cure
       OSDMap::Incremental pending_inc(osdmap.get_epoch() + 1);
-      osdmap.maybe_remove_pg_upmaps(g_ceph_context, osdmap, &pending_inc);
+      osdmap.clean_pg_upmaps(g_ceph_context, &pending_inc);
       osdmap.apply_incremental(pending_inc);
       {
         // validate pg_upmap is gone (reverted)
@@ -1053,7 +1049,7 @@ TEST_F(OSDMapTest, CleanPGUpmaps) {
     {
       // STEP-4: apply cure
       OSDMap::Incremental pending_inc(osdmap.get_epoch() + 1);
-      osdmap.maybe_remove_pg_upmaps(g_ceph_context, osdmap, &pending_inc);
+      osdmap.clean_pg_upmaps(g_ceph_context, &pending_inc);
       osdmap.apply_incremental(pending_inc);
       {
         // validate pg_upmap_items is gone (reverted)
@@ -1336,15 +1332,11 @@ TEST_F(OSDMapTest, BUG_40104) {
   }
   {
     OSDMap::Incremental pending_inc(osdmap.get_epoch() + 1);
-    OSDMap tmpmap;
-    tmpmap.deepish_copy_from(osdmap);
-    tmpmap.apply_incremental(pending_inc);
-    auto start = mono_clock::now();
-    osdmap.maybe_remove_pg_upmaps(g_ceph_context, osdmap, tmpmap,
-                                  &pending_inc);
-    auto latency = mono_clock::now() - start;
+    auto start = ceph_clock_now();
+    osdmap.clean_pg_upmaps(g_ceph_context, &pending_inc);
+    auto latency = ceph_clock_now() - start;
     std::cout << "maybe_remove_pg_upmaps (~" << big_pg_num
-              << " pg_upmap_items) latency:" << timespan_str(latency)
+              << " pg_upmap_items) latency:" << latency
               << std::endl;
   }
 }