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>

diff --git a/src/crimson/osd/heartbeat.cc b/src/crimson/osd/heartbeat.cc
index 3a33d7f93fbc759c74213531315ad224e8ea74a5..d92401bc3ab58ccefbc04ada46f7ccfc1aab1dad 100644 (file)
--- 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 7e8608aa23af58ff7f86b79703a7b94018790b92..d71d4e0c87c2c70b6afa976f9ac3e6c905b79358 100644 (file)
--- 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);