PG: switch the start_recovery_ops interface to specify work to do as a param

We previously inferred whether there was useful work to be done
by looking at the number of ops started, but with the upcoming
introduction of the rw_manager read locking on backfill, we could
start no ops while still having work to do. Switch around the
interfaces to specify these as separate pieces of information.

Signed-off-by: Greg Farnum <greg@inktank.com>

4 files changed, 40 insertions, 18 deletions

diff --git a/src/osd/OSD.cc b/src/osd/OSD.cc
index fabe6da30b8..1753243b260 100644
--- a/src/osd/OSD.cc
+++ b/src/osd/OSD.cc
@@ -6762,7 +6762,9 @@ void OSD::do_recovery(PG *pg, ThreadPool::TPHandle &handle)
 #endif
     
     PG::RecoveryCtx rctx = create_context();
-    int started = pg->start_recovery_ops(max, &rctx, handle);
+
+    int started;
+    bool more = pg->start_recovery_ops(max, &rctx, handle, started);
     dout(10) << "do_recovery started " << started << "/" << max << " on " << *pg << dendl;
 
     /*
@@ -6771,7 +6773,7 @@ void OSD::do_recovery(PG *pg, ThreadPool::TPHandle &handle)
      * It may be that our initial locations were bad and we errored
      * out while trying to pull.
      */
-    if (!started && pg->have_unfound()) {
+    if (!more && pg->have_unfound()) {
       pg->discover_all_missing(*rctx.query_map);
       if (rctx.query_map->empty()) {
 	dout(10) << "do_recovery  no luck, giving up on this pg for now" << dendl;
diff --git a/src/osd/PG.h b/src/osd/PG.h
index dc11638fd4b..023c9c985a5 100644
--- a/src/osd/PG.h
+++ b/src/osd/PG.h
@@ -645,9 +645,14 @@ public:
 
   virtual void check_local() = 0;
 
-  virtual int start_recovery_ops(
+  /**
+   * @param ops_begun returns how many recovery ops the function started
+   * @returns true if any useful work was accomplished; false otherwise
+   */
+  virtual bool start_recovery_ops(
     int max, RecoveryCtx *prctx,
-    ThreadPool::TPHandle &handle) = 0;
+    ThreadPool::TPHandle &handle,
+    int& ops_begun) = 0;
 
   void purge_strays();
 
diff --git a/src/osd/ReplicatedPG.cc b/src/osd/ReplicatedPG.cc
index f6c84843529..85a6d472906 100644
--- a/src/osd/ReplicatedPG.cc
+++ b/src/osd/ReplicatedPG.cc
@@ -7476,17 +7476,22 @@ void ReplicatedPG::check_recovery_sources(const OSDMapRef osdmap)
 }
   
 
-int ReplicatedPG::start_recovery_ops(
+bool ReplicatedPG::start_recovery_ops(
   int max, RecoveryCtx *prctx,
-  ThreadPool::TPHandle &handle)
+  ThreadPool::TPHandle &handle,
+  int& ops_begun)
 {
-  int started = 0;
+  int& started = ops_begun;
+  started = 0;
+  bool work_in_progress = false;
   assert(is_primary());
 
   if (!state_test(PG_STATE_RECOVERING) &&
       !state_test(PG_STATE_BACKFILL)) {
+    /* TODO: I think this case is broken and will make do_recovery()
+     * unhappy since we're returning false */
     dout(10) << "recovery raced and were queued twice, ignoring!" << dendl;
-    return 0;
+    return false;
   }
 
   const pg_missing_t &missing = pg_log.get_missing();
@@ -7512,6 +7517,9 @@ int ReplicatedPG::start_recovery_ops(
     started = recover_replicas(max, handle);
   }
 
+  if (started)
+    work_in_progress = true;
+
   bool deferred_backfill = false;
   if (recovering.empty() &&
       state_test(PG_STATE_BACKFILL) &&
@@ -7535,7 +7543,7 @@ int ReplicatedPG::start_recovery_ops(
       }
       deferred_backfill = true;
     } else {
-      started += recover_backfill(max - started, handle);
+      started += recover_backfill(max - started, handle, work_in_progress);
     }
   }
 
@@ -7543,8 +7551,8 @@ int ReplicatedPG::start_recovery_ops(
   osd->logger->inc(l_osd_rop, started);
 
   if (!recovering.empty() ||
-      started || recovery_ops_active > 0 || deferred_backfill)
-    return started;
+      work_in_progress || recovery_ops_active > 0 || deferred_backfill)
+    return work_in_progress;
 
   assert(recovering.empty());
   assert(recovery_ops_active == 0);
@@ -7552,21 +7560,21 @@ int ReplicatedPG::start_recovery_ops(
   int unfound = get_num_unfound();
   if (unfound) {
     dout(10) << " still have " << unfound << " unfound" << dendl;
-    return started;
+    return work_in_progress;
   }
 
   if (missing.num_missing() > 0) {
     // this shouldn't happen!
     osd->clog.error() << info.pgid << " recovery ending with " << missing.num_missing()
 		      << ": " << missing.missing << "\n";
-    return started;
+    return work_in_progress;
   }
 
   if (needs_recovery()) {
     // this shouldn't happen!
     // We already checked num_missing() so we must have missing replicas
     osd->clog.error() << info.pgid << " recovery ending with missing replicas\n";
-    return started;
+    return work_in_progress;
   }
 
   if (state_test(PG_STATE_RECOVERING)) {
@@ -7917,7 +7925,7 @@ int ReplicatedPG::recover_replicas(int max, ThreadPool::TPHandle &handle)
  */
 int ReplicatedPG::recover_backfill(
   int max,
-  ThreadPool::TPHandle &handle)
+  ThreadPool::TPHandle &handle, bool& work_started)
 {
   dout(10) << "recover_backfill (" << max << ")" << dendl;
   assert(backfill_target >= 0);
@@ -8097,6 +8105,8 @@ int ReplicatedPG::recover_backfill(
 
   dout(10) << " peer num_objects now " << pinfo.stats.stats.sum.num_objects
 	   << " / " << info.stats.stats.sum.num_objects << dendl;
+  if (ops)
+    work_started = true;
   return ops;
 }
 
diff --git a/src/osd/ReplicatedPG.h b/src/osd/ReplicatedPG.h
index 356ad4e31d7..c7734efa3f8 100644
--- a/src/osd/ReplicatedPG.h
+++ b/src/osd/ReplicatedPG.h
@@ -881,13 +881,18 @@ protected:
   void _clear_recovery_state();
 
   void queue_for_recovery();
-  int start_recovery_ops(
+  bool start_recovery_ops(
     int max, RecoveryCtx *prctx,
-    ThreadPool::TPHandle &handle);
+    ThreadPool::TPHandle &handle, int& started);
 
   int recover_primary(int max, ThreadPool::TPHandle &handle);
   int recover_replicas(int max, ThreadPool::TPHandle &handle);
-  int recover_backfill(int max, ThreadPool::TPHandle &handle);
+  /**
+   * @param work_started will be set to true if recover_backfill got anywhere
+   * @returns the number of operations started
+   */
+  int recover_backfill(int max, ThreadPool::TPHandle &handle,
+                       bool& work_started);
 
   /**
    * scan a (hash) range of objects in the current pg