From 10d9f100ee284087ac78d712023a29e4265e1090 Mon Sep 17 00:00:00 2001 From: xie xingguo Date: Mon, 17 Jun 2019 18:44:09 +0800 Subject: [PATCH] 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 --- src/mon/OSDMonitor.cc | 6 +- src/osd/OSDMap.cc | 160 ++++++++++++------------------------- src/osd/OSDMap.h | 8 +- src/test/osd/TestOSDMap.cc | 32 +++----- 4 files changed, 66 insertions(+), 140 deletions(-) diff --git a/src/mon/OSDMonitor.cc b/src/mon/OSDMonitor.cc index f3dd6b5665e7f..fb665f00c4606 100644 --- 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 515d3f6bba3b4..0007a36c34996 100644 --- 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 to_check; set to_cancel; map> 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 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> 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 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 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 raw; - int primary; - pg_to_raw_osds(p.first, &raw, &primary); - mempool::osdmap::vector> 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 8d6bd64b5f374..71999968147df 100644 --- 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 7fb65921a6a64..dc85502d2c588 100644 --- 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; } } -- 2.39.5