crimson/os/seastore/segment_cleaner: fix an issue that caused forever-GC

Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>

diff --git a/src/crimson/os/seastore/segment_cleaner.cc b/src/crimson/os/seastore/segment_cleaner.cc
index ad55494c1ce7efa3c1bba0f95c5351a7231eaa40..cf087c69a05348550b963d62ddece48037a1297b 100644 (file)
--- a/src/crimson/os/seastore/segment_cleaner.cc
+++ b/src/crimson/os/seastore/segment_cleaner.cc
@@ -80,6 +80,7 @@ void segments_info_t::reset()
   segment_size = 0;
 
   journal_segment_id = NULL_SEG_ID;
+  // the number of in-journal closed segments
   num_in_journal = 0;
 
   num_open = 0;
@@ -167,7 +168,6 @@ void segments_info_t::mark_open(
       ceph_assert(last_journal_segment.seq + 1 == seq);
     }
     journal_segment_id = segment;
-    ++num_in_journal;
   }
   ceph_assert(segment_info.written_to == 0);
   avail_bytes_in_open += get_segment_size();
@@ -209,6 +209,9 @@ void segments_info_t::mark_closed(
   ceph_assert(num_open > 0);
   --num_open;
   ++num_closed;
+  if (segment_info.type == segment_type_t::JOURNAL) {
+    ++num_in_journal;
+  }
   ceph_assert(get_segment_size() >= segment_info.written_to);
   auto seg_avail_bytes = get_segment_size() - segment_info.written_to;
   ceph_assert(avail_bytes_in_open >= seg_avail_bytes);

diff --git a/src/crimson/os/seastore/segment_cleaner.h b/src/crimson/os/seastore/segment_cleaner.h
index 12ca08954c93e6c915eb00f372f49c316de8d0b9..09b8aebfd2b9a826f89ce72c62116136b0c0ebce 100644 (file)
--- a/src/crimson/os/seastore/segment_cleaner.h
+++ b/src/crimson/os/seastore/segment_cleaner.h
@@ -115,21 +115,11 @@ public:
     return segment_size;
   }
   std::size_t get_num_in_journal() const {
+    assert(num_in_journal <= num_closed);
     return num_in_journal;
   }
-  std::size_t get_num_unreclaimable() const {
-    auto ret = num_in_journal + num_open;
-    if (ret > 0) {
-      // assume there is always 1 segment opened for journal (1 overlap)
-      return ret - 1;
-    } else {
-      return ret;
-    }
-  }
   std::size_t get_num_reclaimable() const {
-    auto ret = num_open + num_closed;
-    assert(ret >= get_num_unreclaimable());
-    return ret - get_num_unreclaimable();
+    return num_closed - get_num_in_journal();
   }
   std::size_t get_num_open() const {
     return num_open;
@@ -168,7 +158,7 @@ public:
     return total_bytes - get_available_bytes();
   }
   std::size_t get_unavailable_unreclaimable_bytes() const {
-    auto ret = get_num_unreclaimable() * get_segment_size();
+    auto ret = (num_open + get_num_in_journal()) * get_segment_size();
     assert(ret >= avail_bytes_in_open);
     return ret - avail_bytes_in_open;
   }
@@ -907,6 +897,9 @@ private:
        max_benefit_cost);
       return journal_seq_t{seq, paddr_t::make_seg_paddr(id, 0)};
     } else {
+      ceph_assert(segments.get_num_reclaimable() == 0);
+      // see gc_should_reclaim_space()
+      ceph_abort("impossible!");
       return JOURNAL_SEQ_NULL;
     }
   }
@@ -1106,13 +1099,18 @@ private:
    * Encapsulates whether block pending gc.
    */
   bool should_block_on_gc() const {
+    if (get_dirty_tail_limit() > journal_tail_target) {
+      return true;
+    }
+    if (segments.get_num_reclaimable() == 0) {
+      return false;
+    }
     auto aratio = get_projected_available_ratio();
     auto rratio = get_reclaim_ratio();
     return (
       (aratio < config.available_ratio_hard_limit) ||
       ((aratio < config.available_ratio_gc_max) &&
-       (rratio > config.reclaim_ratio_hard_limit)) ||
-      (get_dirty_tail_limit() > journal_tail_target)
+       (rratio > config.reclaim_ratio_hard_limit))
     );
   }
 
@@ -1121,11 +1119,15 @@ private:
     if (logger.is_enabled(seastar::log_level::debug)) {
       logger.debug(
        "SegmentCleaner::log_gc_state({}): "
-       "total {}, "
-       "available {}, "
-       "unavailable {}, "
-       "unavailable_used {}, "
-       "unavailable_unused {}, "
+       "empty {}, "
+       "open {}, "
+       "closed {}, "
+       "in_journal {}, "
+       "total {}B, "
+       "available {}B, "
+       "unavailable {}B, "
+       "unavailable_used {}B, "
+       "unavailable_unused {}B; "
        "reclaim_ratio {}, "
        "available_ratio {}, "
        "should_block_on_gc {}, "
@@ -1137,6 +1139,10 @@ private:
        "dirty_tail_limit {}, "
        "gc_should_trim_journal {}, ",
        caller,
+       segments.get_num_empty(),
+       segments.get_num_open(),
+       segments.get_num_closed(),
+       segments.get_num_in_journal(),
        segments.get_total_bytes(),
        segments.get_available_bytes(),
        segments.get_unavailable_bytes(),
@@ -1213,6 +1219,9 @@ private:
    * Encapsulates logic for whether gc should be reclaiming segment space.
    */
   bool gc_should_reclaim_space() const {
+    if (segments.get_num_reclaimable() == 0) {
+      return false;
+    }
     auto aratio = segments.get_available_ratio();
     auto rratio = get_reclaim_ratio();
     return (