From: Greg Farnum <gfarnum@redhat.com>
Date: Tue, 1 Aug 2017 02:52:42 +0000 (-0700)
Subject: osd: pg: be more careful with locking around forced pg recovery
X-Git-Tag: ses5-milestone10~6^2~10^2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=refs%2Fpull%2F16712%2Fhead;p=ceph.git

osd: pg: be more careful with locking around forced pg recovery

This does several little things that add up to big concurrency and safety
improvements:
* Switch to passing around PGRefs instead of raw pointers, which is
  generally a good idea
* drop the pg_map_lock once we're done looking up the PGRefs, since
  we don't need it and holding the PG pointer alive was the only previous
  thing that might have made it necessary
* don't hold the recovery_lock since we don't need any OSD-level
  synchronization
* make sure the PG is not being deleted before we do a force-change of its
  state

Fixes: http://tracker.ceph.com/issues/20808

Signed-off-by: Greg Farnum <gfarnum@redhat.com>
---

diff --git a/src/osd/OSD.cc b/src/osd/OSD.cc
index f8227ebe0ec..b225d590dd6 100644
--- a/src/osd/OSD.cc
+++ b/src/osd/OSD.cc
@@ -8922,17 +8922,19 @@ void OSD::handle_force_recovery(Message *m)
 {
   MOSDForceRecovery *msg = static_cast<MOSDForceRecovery*>(m);
   assert(msg->get_type() == MSG_OSD_FORCE_RECOVERY);
-  RWLock::RLocker l(pg_map_lock);
 
-  vector<PG*> local_pgs;
+  vector<PGRef> local_pgs;
   local_pgs.reserve(msg->forced_pgs.size());
 
-  for (auto& i : msg->forced_pgs) {
-    spg_t locpg;
-    if (osdmap->get_primary_shard(i, &locpg)) {
-      auto pg_map_entry = pg_map.find(locpg);
-      if (pg_map_entry != pg_map.end()) {
-	local_pgs.push_back(pg_map_entry->second);
+  {
+    RWLock::RLocker l(pg_map_lock);
+    for (auto& i : msg->forced_pgs) {
+      spg_t locpg;
+      if (osdmap->get_primary_shard(i, &locpg)) {
+	auto pg_map_entry = pg_map.find(locpg);
+	if (pg_map_entry != pg_map.end()) {
+	  local_pgs.push_back(pg_map_entry->second);
+	}
       }
     }
   }
@@ -9198,14 +9200,12 @@ bool OSDService::_recover_now(uint64_t *available_pushes)
 }
 
 
-void OSDService::adjust_pg_priorities(vector<PG*> pgs, int newflags)
+void OSDService::adjust_pg_priorities(const vector<PGRef>& pgs, int newflags)
 {
   if (!pgs.size() || !(newflags & (OFR_BACKFILL | OFR_RECOVERY)))
     return;
   int newstate = 0;
 
-  Mutex::Locker l(recovery_lock);
-
   if (newflags & OFR_BACKFILL) {
     newstate = PG_STATE_FORCED_BACKFILL;
   } else if (newflags & OFR_RECOVERY) {
@@ -9226,17 +9226,21 @@ void OSDService::adjust_pg_priorities(vector<PG*> pgs, int newflags)
 
   if (newflags & OFR_CANCEL) {
     for (auto& i : pgs) {
-      i->change_recovery_force_mode(newstate, true);
+      i->lock();
+      i->_change_recovery_force_mode(newstate, true);
+      i->unlock();
     }
   } else {
     for (auto& i : pgs) {
       // make sure the PG is in correct state before forcing backfill or recovery, or
       // else we'll make PG keeping FORCE_* flag forever, requiring osds restart
       // or forcing somehow recovery/backfill.
+      i->lock();
       int pgstate = i->get_state();
       if ( ((newstate == PG_STATE_FORCED_RECOVERY) && (pgstate & (PG_STATE_DEGRADED | PG_STATE_RECOVERY_WAIT | PG_STATE_RECOVERING))) ||
 	    ((newstate == PG_STATE_FORCED_BACKFILL) && (pgstate & (PG_STATE_DEGRADED | PG_STATE_BACKFILL_WAIT | PG_STATE_BACKFILL))) )
-        i->change_recovery_force_mode(newstate, false);
+        i->_change_recovery_force_mode(newstate, false);
+      i->unlock();
     }
   }
 }
diff --git a/src/osd/OSD.h b/src/osd/OSD.h
index 573235d8ca4..33e5e3b27ac 100644
--- a/src/osd/OSD.h
+++ b/src/osd/OSD.h
@@ -962,7 +962,7 @@ public:
     _queue_for_recovery(make_pair(queued, pg), reserved_pushes);
   }
 
-  void adjust_pg_priorities(vector<PG*> pgs, int newflags);
+  void adjust_pg_priorities(const vector<PGRef>& pgs, int newflags);
 
   // osd map cache (past osd maps)
   Mutex map_cache_lock;
diff --git a/src/osd/PG.cc b/src/osd/PG.cc
index 5153d5d8f32..5bce295fb55 100644
--- a/src/osd/PG.cc
+++ b/src/osd/PG.cc
@@ -2039,17 +2039,17 @@ void PG::mark_clean()
   kick_snap_trim();
 }
 
-void PG::change_recovery_force_mode(int new_mode, bool clear)
+void PG::_change_recovery_force_mode(int new_mode, bool clear)
 {
-  lock(true);
-  if (clear) {
-    state_clear(new_mode);
-  } else {
-    state_set(new_mode);
+  if (!deleting) {
+    // we can't and shouldn't do anything if the PG is being deleted locally
+    if (clear) {
+      state_clear(new_mode);
+    } else {
+      state_set(new_mode);
+    }
+    publish_stats_to_osd();
   }
-  publish_stats_to_osd();
-
-  unlock();
 }
 
 inline int PG::clamp_recovery_priority(int priority)
diff --git a/src/osd/PG.h b/src/osd/PG.h
index 924c930dd83..e6246ba6973 100644
--- a/src/osd/PG.h
+++ b/src/osd/PG.h
@@ -982,7 +982,7 @@ public:
   unsigned get_backfill_priority();
 
   void mark_clean();  ///< mark an active pg clean
-  void change_recovery_force_mode(int new_mode, bool clear);
+  void _change_recovery_force_mode(int new_mode, bool clear);
 
   /// return [start,end) bounds for required past_intervals
   static pair<epoch_t, epoch_t> get_required_past_interval_bounds(