Merge pull request ceph#53104 from ceph/revert-49428-maintain_prefix_itr

Revert "osd/SnapMapper: Maintain the prefix_itr between calls to avoid search…"

Reviewed-by: Gabriel BenHanokh <[email protected]>
Reviewed-by: Neha Ojha <[email protected]>

Author: rzarzynski

qa/suites/rados/singleton-nomsgr/all/ceph-snapmapper.yaml

@@ -55,9 +55,8 @@
 	osd debug shutdown = true
         osd debug op order = true
         osd debug verify stray on activate = true
-        osd debug trim objects = true
 
-        osd open classes on start = true
+	osd open classes on start = true
         osd debug pg log writeout = true
 
 	osd deep scrub update digest min age = 30

qa/tasks/ceph.conf.template

@@ -194,11 +194,6 @@ options:
     load on busy clusters.
   default: false
   with_legacy: true
-- name: osd_debug_trim_objects
-  type: bool
-  level: advanced
-  desc: Asserts that no clone-objects were added to a snap after we start trimming it
-  default: false
 - name: osd_repair_during_recovery
   type: bool
   level: advanced

src/common/options/osd.yaml.in

@@ -540,28 +540,36 @@ void SnapMapper::add_oid(
   backend.set_keys(to_add, t);
 }
 
-void SnapMapper::get_objects_by_prefixes(
+int SnapMapper::get_next_objects_to_trim(
   snapid_t snap,
   unsigned max,
   vector<hobject_t> *out)
 {
-  /// maintain the prefix_itr between calls to avoid searching depleted prefixes
-  for ( ; prefix_itr != prefixes.end(); prefix_itr++) {
-    string prefix(get_prefix(pool, snap) + *prefix_itr);
+  ceph_assert(out);
+  ceph_assert(out->empty());
+
+  // if max would be 0, we return ENOENT and the caller would mistakenly
+  // trim the snaptrim queue
+  ceph_assert(max > 0);
+  int r = 0;
+
+  /// \todo cache the prefixes-set in update_bits()
+  for (set<string>::iterator i = prefixes.begin();
+       i != prefixes.end() && out->size() < max && r == 0;
+       ++i) {
+    string prefix(get_prefix(pool, snap) + *i);
     string pos = prefix;
     while (out->size() < max) {
       pair<string, ceph::buffer::list> next;
-      // access RocksDB (an expensive operation!)
-      int r = backend.get_next(pos, &next);
+      r = backend.get_next(pos, &next);
       dout(20) << __func__ << " get_next(" << pos << ") returns " << r
 	       << " " << next << dendl;
       if (r != 0) {
-	return; // Done
+	break; // Done
       }
 
       if (next.first.substr(0, prefix.size()) !=
 	  prefix) {
-	// TBD: we access the DB twice for the first object of each iterator...
 	break; // Done with this prefix
       }
 
@@ -575,59 +583,15 @@ void SnapMapper::get_objects_by_prefixes(
       out->push_back(next_decoded.second);
       pos = next.first;
     }
-
-    if (out->size() >= max) {
-      return;
-    }
   }
-}
-
-int SnapMapper::get_next_objects_to_trim(
-  snapid_t snap,
-  unsigned max,
-  vector<hobject_t> *out)
-{
-  ceph_assert(out);
-  ceph_assert(out->empty());
-
-  // if max would be 0, we return ENOENT and the caller would mistakenly
-  // trim the snaptrim queue
-  ceph_assert(max > 0);
-
-  // The prefix_itr is bound to a prefix_itr_snap so if we trim another snap
-  // we must reset the prefix_itr (should not happen normally)
-  if (prefix_itr_snap != snap) {
-    dout(10) << __func__ << "::Reset prefix_itr(unexpcted) snap was changed from <"
-	     << prefix_itr_snap << "> to <" << snap << ">" << dendl;
-    reset_prefix_itr(snap);
-  }
-
-  // when reaching the end of the DB reset the prefix_ptr and verify
-  // we didn't miss objects which were added after we started trimming
-  // This should never happen in reality because the snap was logically deleted
-  // before trimming starts (and so no new clone-objects could be added)
-  // For more info see PG::filter_snapc()
-  //
-  // We still like to be extra careful and run one extra loop over all prefeixes
-  get_objects_by_prefixes(snap, max, out);
-  if (out->size() == 0) {
-    dout(10) << __func__ << "::Reset prefix_itr after a complete trim!" << dendl;
-    reset_prefix_itr(snap);
-    get_objects_by_prefixes(snap, max, out);
-
-    bool osd_debug_trim_objects = cct->_conf.get_val<bool>("osd_debug_trim_objects");
-    if (osd_debug_trim_objects) {
-      ceph_assert(out->size() == 0);
-    }
-  }
-
   if (out->size() == 0) {
     return -ENOENT;
   } else {
     return 0;
   }
 }
 
+
 int SnapMapper::remove_oid(
   const hobject_t &oid,
   MapCacher::Transaction<std::string, ceph::buffer::list> *t)

src/osd/SnapMapper.cc

@@ -281,24 +281,6 @@ class SnapMapper : public Scrub::SnapMapReaderI {
   tl::expected<object_snaps, SnapMapReaderI::result_t> get_snaps_common(
     const hobject_t &hoid) const;
 
-  /// file @out vector with the first objects with @snap as a snap
-  void get_objects_by_prefixes(
-    snapid_t snap,
-    unsigned max,
-    std::vector<hobject_t> *out);
-
-  std::set<std::string>           prefixes;
-  // maintain a current active prefix
-  std::set<std::string>::iterator prefix_itr;
-  // associate the active prefix with a snap
-  snapid_t                        prefix_itr_snap;
-
-  // reset the prefix iterator to the first prefix hash
-  void reset_prefix_itr(snapid_t snap) {
-    prefix_itr_snap = snap;
-    prefix_itr      = prefixes.begin();
-  }
-
  public:
   static std::string make_shard_prefix(shard_id_t shard) {
     if (shard == shard_id_t::NO_SHARD)
@@ -327,6 +309,7 @@ class SnapMapper : public Scrub::SnapMapReaderI {
     update_bits(mask_bits);
   }
 
+  std::set<std::string> prefixes;
   /// Update bits in case of pg split or merge
   void update_bits(
     uint32_t new_bits  ///< [in] new split bits
@@ -340,12 +323,6 @@ class SnapMapper : public Scrub::SnapMapReaderI {
     for (auto i = _prefixes.begin(); i != _prefixes.end(); ++i) {
       prefixes.insert(shard_prefix + *i);
     }
-
-    reset_prefix_itr(CEPH_NOSNAP);
-  }
-
-  const std::set<std::string>::iterator get_prefix_itr() {
-    return prefix_itr;
   }
 
   /// Update snaps for oid, empty new_snaps removes the mapping

src/osd/SnapMapper.h

@@ -466,10 +466,6 @@ add_executable(ceph_test_snap_mapper
   $<TARGET_OBJECTS:unit-main>
   )
 target_link_libraries(ceph_test_snap_mapper osd global ${BLKID_LIBRARIES} ${UNITTEST_LIBS})
-
-install(TARGETS
-  ceph_test_snap_mapper
-  DESTINATION ${CMAKE_INSTALL_BINDIR})
 endif(NOT WIN32)
 
 add_executable(ceph_test_stress_watch