crimson/osd: erase an element by iterator instead

we should not remove an element while iterating it in a map, as erasing
the element invalidates the iterator, which causes segmfault when we are
advancing it after erasing the dereferenced element.

in this change, an iterator is used for walking through the map, in
comparision with creating a to-be-removed list, this one is more
efficient and more idiomatic.

Signed-off-by: Kefu Chai <kchai@redhat.com>

2 files changed, 11 insertions, 7 deletions

diff --git a/src/crimson/osd/heartbeat.cc b/src/crimson/osd/heartbeat.cc
index 3a33d7f93fb..d92401bc3ab 100644
--- a/src/crimson/osd/heartbeat.cc
+++ b/src/crimson/osd/heartbeat.cc
@@ -126,16 +126,20 @@ void Heartbeat::add_peer(osd_id_t _peer, epoch_t epoch)
 
 Heartbeat::osds_t Heartbeat::remove_down_peers()
 {
-  osds_t osds;
-  for (auto& [osd, peer] : peers) {
+  osds_t old_osds; // osds not added in this epoch
+  for (auto i = peers.begin(); i != peers.end(); ) {
     auto osdmap = service.get_osdmap_service().get_map();
+    const auto& [osd, peer] = *i;
     if (!osdmap->is_up(osd)) {
-      remove_peer(osd);
-    } else if (peer.get_epoch() < osdmap->get_epoch()) {
-      osds.push_back(osd);
+      i = peers.erase(i);
+    } else {
+      if (peer.get_epoch() < osdmap->get_epoch()) {
+        old_osds.push_back(osd);
+      }
+      ++i;
     }
   }
-  return osds;
+  return old_osds;
 }
 
 void Heartbeat::add_reporter_peers(int whoami)
diff --git a/src/crimson/osd/heartbeat.h b/src/crimson/osd/heartbeat.h
index 7e8608aa23a..d71d4e0c87c 100644
--- a/src/crimson/osd/heartbeat.h
+++ b/src/crimson/osd/heartbeat.h
@@ -65,7 +65,7 @@ private:
   seastar::future<> handle_you_died();
 
   /// remove down OSDs
-  /// @return peers not needed in this epoch
+  /// @return peers not added in this epoch
   osds_t remove_down_peers();
   /// add enough reporters for fast failure detection
   void add_reporter_peers(int whoami);