From: xie xingguo Date: Sat, 1 Jun 2019 07:50:32 +0000 (+0800) Subject: osd: maybe_remove_pg_upmaps -> clean_pg_upmaps X-Git-Tag: v14.2.3~119^2~4 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=4974d4268719c4d357c207d71b958dbf63eea428;p=ceph.git 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.. Signed-off-by: xie xingguo (cherry picked from commit a37106f0fae4ae2f0696d05b981564ed5170f192) --- diff --git a/src/mon/OSDMonitor.cc b/src/mon/OSDMonitor.cc index b9eed198a4d1..0b4c81fe86cb 100644 --- a/src/mon/OSDMonitor.cc +++ b/src/mon/OSDMonitor.cc @@ -1095,7 +1095,7 @@ void OSDMonitor::encode_pending(MonitorDBStore::TransactionRef t) OSDMap::clean_temps(cct, osdmap, tmp, &pending_inc); // clean inappropriate pg_upmap/pg_upmap_items (if any) - osdmap.maybe_remove_pg_upmaps(cct, osdmap, tmp, &pending_inc); + tmp.clean_pg_upmaps(cct, &pending_inc); // update creating pgs first so that we can remove the created pgid and // process the pool flag removal below in the same osdmap epoch. diff --git a/src/osd/OSDMap.cc b/src/osd/OSDMap.cc index e5c19a605fb4..11ae66e940b1 100644 --- a/src/osd/OSDMap.cc +++ b/src/osd/OSDMap.cc @@ -1776,30 +1776,24 @@ void OSDMap::clean_temps(CephContext *cct, } } -void OSDMap::maybe_remove_pg_upmaps(CephContext *cct, - const OSDMap& oldmap, - const OSDMap& nextmap, - Incremental *pending_inc) +bool OSDMap::clean_pg_upmaps( + CephContext *cct, + Incremental *pending_inc) const { ldout(cct, 10) << __func__ << dendl; set to_check; set to_cancel; map> rule_weight_map; + bool any_change = false; - for (auto& p : nextmap.pg_upmap) { - to_check.insert(p.first); - } - for (auto& p : nextmap.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) { - const pg_pool_t *pi = nextmap.get_pg_pool(pg.pool()); + const pg_pool_t *pi = get_pg_pool(pg.pool()); if (!pi || pg.ps() >= pi->get_pg_num_pending()) { ldout(cct, 0) << __func__ << " pg " << pg << " is gone or merge source" << dendl; @@ -1813,17 +1807,17 @@ void OSDMap::maybe_remove_pg_upmaps(CephContext *cct, continue; } vector raw, up; - nextmap.pg_to_raw_upmap(pg, &raw, &up); - auto i = nextmap.pg_upmap.find(pg); - if (i != nextmap.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 = nextmap.pg_upmap_items.find(pg); - if (j != nextmap.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()) { @@ -1849,14 +1843,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 = nextmap.get_pg_pool_crush_rule(pg); - auto r = nextmap.crush->verify_upmap(cct, - crush_rule, - nextmap.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 @@ -1868,7 +1863,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 = nextmap.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; @@ -1888,7 +1883,7 @@ void OSDMap::maybe_remove_pg_upmaps(CephContext *cct, to_cancel.insert(pg); break; } - auto adjusted_weight = nextmap.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); @@ -1897,42 +1892,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 (oldmap.pg_upmap.count(pg)) { - ldout(cct, 10) << __func__ << " cancel invalid pg_upmap entry " - << oldmap.pg_upmap.find(pg)->first << "->" - << oldmap.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 (oldmap.pg_upmap_items.count(pg)) { - ldout(cct, 10) << __func__ << " cancel invalid " - << "pg_upmap_items entry " - << oldmap.pg_upmap_items.find(pg)->first << "->" - << oldmap.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) @@ -4377,56 +4370,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 ac7348be424b..30534bcb7160 100644 --- a/src/osd/OSDMap.h +++ b/src/osd/OSDMap.h @@ -1034,10 +1034,7 @@ public: */ uint64_t get_up_osd_features() const; - void maybe_remove_pg_upmaps(CephContext *cct, - const OSDMap& oldmap, - const OSDMap& nextmap, - Incremental *pending_inc); + bool clean_pg_upmaps(CephContext *cct, Incremental *pending_inc) const; int apply_incremental(const Incremental &inc); @@ -1386,10 +1383,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 140aed053684..5dd3f6d8641a 100644 --- a/src/test/osd/TestOSDMap.cc +++ b/src/test/osd/TestOSDMap.cc @@ -714,12 +714,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); - OSDMap nextmap; - nextmap.deepish_copy_from(tmpmap); - nextmap.maybe_remove_pg_upmaps(g_ceph_context, tmpmap, - 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)); } @@ -767,12 +764,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, tmpmap, - 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)); } @@ -894,12 +888,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, tmp, - 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)); } @@ -964,10 +955,7 @@ TEST_F(OSDMapTest, CleanPGUpmaps) { { // STEP-2: apply cure OSDMap::Incremental pending_inc(osdmap.get_epoch() + 1); - OSDMap tmpmap; - tmpmap.deepish_copy_from(osdmap); - tmpmap.apply_incremental(pending_inc); - osdmap.maybe_remove_pg_upmaps(g_ceph_context, osdmap, tmpmap, &pending_inc); + osdmap.clean_pg_upmaps(g_ceph_context, &pending_inc); osdmap.apply_incremental(pending_inc); { // validate pg_upmap is gone (reverted) @@ -1101,11 +1089,7 @@ TEST_F(OSDMapTest, CleanPGUpmaps) { { // STEP-4: apply cure OSDMap::Incremental pending_inc(osdmap.get_epoch() + 1); - OSDMap tmpmap; - tmpmap.deepish_copy_from(osdmap); - tmpmap.apply_incremental(pending_inc); - osdmap.maybe_remove_pg_upmaps(g_ceph_context, osdmap, tmpmap, - &pending_inc); + osdmap.clean_pg_upmaps(g_ceph_context, &pending_inc); osdmap.apply_incremental(pending_inc); { // validate pg_upmap_items is gone (reverted) @@ -1388,14 +1372,10 @@ 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); + osdmap.clean_pg_upmaps(g_ceph_context, &pending_inc); auto latency = mono_clock::now() - start; - std::cout << "maybe_remove_pg_upmaps (~" << big_pg_num + std::cout << "clean_pg_upmaps (~" << big_pg_num << " pg_upmap_items) latency:" << timespan_str(latency) << std::endl; }