osd: improved get_objects_by_prefixes() ergonomics

Improved call signatures for get_next_objects_to_trim() &
get_objects_by_prefixes().

Also: as as get_next_objects_to_trim() has only a single
failure mode, we should not try to handle two distinct failures
in its callers' code.

Signed-off-by: Ronen Friedman <rfriedma@redhat.com>

diff --git a/src/crimson/osd/osd_operations/snaptrim_event.cc b/src/crimson/osd/osd_operations/snaptrim_event.cc
index 1a2292ae90fd86845a6ffdc7d4dcd714fc6b0543..aaa432e295f93535c88cb6c20c9d20f1d43fdbe2 100644 (file)
--- a/src/crimson/osd/osd_operations/snaptrim_event.cc
+++ b/src/crimson/osd/osd_operations/snaptrim_event.cc
@@ -88,28 +88,19 @@ SnapTrimEvent::start()
         client_pp().process);
     }).then_interruptible([&shard_services, this] {
       return interruptor::async([this] {
-        std::vector<hobject_t> to_trim;
         using crimson::common::local_conf;
         const auto max =
           local_conf().get_val<uint64_t>("osd_pg_max_concurrent_snap_trims");
         // we need to look for at least 1 snaptrim, otherwise we'll misinterpret
-        // the ENOENT below and erase snapid.
-        int r = snap_mapper.get_next_objects_to_trim(
+        // the nullopt below and erase snapid.
+        auto to_trim = snap_mapper.get_next_objects_to_trim(
           snapid,
-          max,
-          &to_trim);
-        if (r == -ENOENT) {
-          to_trim.clear(); // paranoia
-          return to_trim;
-        } else if (r != 0) {
-          logger().error("{}: get_next_objects_to_trim returned {}",
-                         *this, cpp_strerror(r));
-          ceph_abort_msg("get_next_objects_to_trim returned an invalid code");
-        } else {
-          assert(!to_trim.empty());
+          max);
+        if (!to_trim.has_value()) {
+          return std::vector<hobject_t>{};
         }
         logger().debug("{}: async almost done line {}", *this, __LINE__);
-        return to_trim;
+        return std::move(*to_trim);
       }).then_interruptible([&shard_services, this] (const auto& to_trim) {
         if (to_trim.empty()) {
           // the legit ENOENT -> done

diff --git a/src/osd/PrimaryLogPG.cc b/src/osd/PrimaryLogPG.cc
index f3b89440425dafec533e250deafb221238406b48..62b8aad27d5b9de6fda22e61ae1c9f9a9b891bec 100644 (file)
--- a/src/osd/PrimaryLogPG.cc
+++ b/src/osd/PrimaryLogPG.cc
@@ -15707,23 +15707,16 @@ boost::statechart::result PrimaryLogPG::AwaitAsyncWork::react(const DoSnapWork&)
 
   ldout(pg->cct, 10) << "AwaitAsyncWork: trimming snap " << snap_to_trim << dendl;
 
-  vector<hobject_t> to_trim;
   unsigned max = pg->cct->_conf->osd_pg_max_concurrent_snap_trims;
   // we need to look for at least 1 snaptrim, otherwise we'll misinterpret
   // the ENOENT below and erase snap_to_trim.
   ceph_assert(max > 0);
-  to_trim.reserve(max);
-  int r = pg->snap_mapper.get_next_objects_to_trim(
-    snap_to_trim,
-    max,
-    &to_trim);
-  if (r != 0 && r != -ENOENT) {
-    lderr(pg->cct) << "get_next_objects_to_trim returned "
-                  << cpp_strerror(r) << dendl;
-    ceph_abort_msg("get_next_objects_to_trim returned an invalid code");
-  } else if (r == -ENOENT) {
+
+  auto to_trim =
+      pg->snap_mapper.get_next_objects_to_trim(snap_to_trim, max);
+  if (!to_trim.has_value()) {
     // Done!
-    ldout(pg->cct, 10) << "got ENOENT" << dendl;
+    ldout(pg->cct, 10) << "no more entries to trim" << dendl;
 
     pg->snap_trimq.erase(snap_to_trim);
 
@@ -15754,9 +15747,8 @@ boost::statechart::result PrimaryLogPG::AwaitAsyncWork::react(const DoSnapWork&)
     pg->set_snaptrim_duration();
     return transit< NotTrimming >();
   }
-  ceph_assert(!to_trim.empty());
 
-  for (auto &&object: to_trim) {
+  for (auto &&object: *to_trim) {
     // Get next
     ldout(pg->cct, 10) << "AwaitAsyncWork react trimming " << object << dendl;
     OpContextUPtr ctx;

diff --git a/src/osd/SnapMapper.cc b/src/osd/SnapMapper.cc
index c168482722094e5f539329d41c4664e51541a0ed..e6a2b4d7d5575db0c85bf28219adc83e35f0f5f0 100644 (file)
--- a/src/osd/SnapMapper.cc
+++ b/src/osd/SnapMapper.cc
@@ -576,27 +576,27 @@ void SnapMapper::reset_prefix_itr(snapid_t snap, const char *s)
   prefix_itr      = prefixes.begin();
 }
 
-void SnapMapper::get_objects_by_prefixes(
+vector<hobject_t> SnapMapper::get_objects_by_prefixes(
   snapid_t snap,
-  unsigned max,
-  vector<hobject_t> *out)
+  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);
+    const string prefix(get_prefix(pool, snap) + *prefix_itr);
     string pos = prefix;
-    while (out->size() < max) {
+    while (out.size() < max) {
       pair<string, ceph::buffer::list> next;
       // access RocksDB (an expensive operation!)
       int r = backend.get_next(pos, &next);
       dout(20) << __func__ << " get_next(" << pos << ") returns " << r
               << " " << next << dendl;
       if (r != 0) {
-       return; // Done
+       return out; // Done
       }
 
-      if (next.first.substr(0, prefix.size()) !=
-         prefix) {
+      if (!next.first.starts_with(prefix)) {
        // TBD: we access the DB twice for the first object of each iterator...
        break; // Done with this prefix
       }
@@ -608,24 +608,22 @@ void SnapMapper::get_objects_by_prefixes(
       ceph_assert(next_decoded.first == snap);
       ceph_assert(check(next_decoded.second));
 
-      out->push_back(next_decoded.second);
+      out.push_back(next_decoded.second);
       pos = next.first;
     }
 
-    if (out->size() >= max) {
-      return;
+    if (out.size() >= max) {
+      return out;
     }
   }
+  return out;
 }
 
-int SnapMapper::get_next_objects_to_trim(
+std::optional<vector<hobject_t>> SnapMapper::get_next_objects_to_trim(
   snapid_t snap,
-  unsigned max,
-  vector<hobject_t> *out)
+  unsigned max)
 {
   dout(20) << __func__ << "::snapid=" << snap << dendl;
-  ceph_assert(out);
-  ceph_assert(out->empty());
 
   // if max would be 0, we return ENOENT and the caller would mistakenly
   // trim the snaptrim queue
@@ -649,25 +647,26 @@ int SnapMapper::get_next_objects_to_trim(
   // For more info see PG::filter_snapc()
   //
   // We still like to be extra careful and run one extra loop over all prefixes
-  get_objects_by_prefixes(snap, max, out);
-  if (unlikely(out->size() == 0)) {
+  auto objs = get_objects_by_prefixes(snap, max);
+  if (unlikely(objs.size() == 0)) {
     reset_prefix_itr(snap, "Second pass trim");
-    get_objects_by_prefixes(snap, max, out);
+    objs = get_objects_by_prefixes(snap, max);
 
-    if (unlikely(out->size() > 0)) {
+    if (unlikely(objs.size() > 0)) {
       derr << __func__ << "::New Clone-Objects were added to Snap " << snap
           << " after trimming was started" << dendl;
     }
     reset_prefix_itr(CEPH_NOSNAP, "Trim was completed successfully");
   }
 
-  if (out->size() == 0) {
-    return -ENOENT;
+  if (objs.size() == 0) {
+    return std::nullopt;
   } else {
-    return 0;
+    return objs;
   }
 }
 
+
 int SnapMapper::remove_oid(
   const hobject_t &oid,
   MapCacher::Transaction<std::string, ceph::buffer::list> *t)

diff --git a/src/osd/SnapMapper.h b/src/osd/SnapMapper.h
index 70d1b6c39add78a977f3a3695ed80be37bcdf4b0..a90faa8e84fc332b211d55ecdb7fe29fc131e89d 100644 (file)
--- a/src/osd/SnapMapper.h
+++ b/src/osd/SnapMapper.h
@@ -291,11 +291,10 @@ private:
   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(
+  /// \returns vector with the first objects with @snap as a snap
+  std::vector<hobject_t> get_objects_by_prefixes(
     snapid_t snap,
-    unsigned max,
-    std::vector<hobject_t> *out);
+    unsigned max);
 
   std::set<std::string>           prefixes;
   // maintain a current active prefix
@@ -373,11 +372,10 @@ private:
     );
 
   /// Returns first object with snap as a snap
-  int get_next_objects_to_trim(
+  std::optional<std::vector<hobject_t>> get_next_objects_to_trim(
     snapid_t snap,              ///< [in] snap to check
-    unsigned max,               ///< [in] max to get
-    std::vector<hobject_t> *out      ///< [out] next objects to trim (must be empty)
-    );  ///< @return error, -ENOENT if no more objects
+    unsigned max                ///< [in] max to get
+    );  ///< @return nullopt if no more objects
 
   /// Remove mapping for oid
   int remove_oid(

diff --git a/src/test/test_snap_mapper.cc b/src/test/test_snap_mapper.cc
index 9fe726afb1f9b1f7c12ff1d255c7bbb09928a702..a47d2538c3a65097454cadebc38e5fac7d04c8f5 100644 (file)
--- a/src/test/test_snap_mapper.cc
+++ b/src/test/test_snap_mapper.cc
@@ -558,18 +558,18 @@ public:
   // must be called with lock held to protect access to
   // snap_to_hobject and hobject_to_snap
   int trim_snap(snapid_t snapid, unsigned max_count, vector<hobject_t> & out) {
-    set<hobject_t>&   hobjects = snap_to_hobject[snapid];
-    vector<hobject_t> hoids;
-    int ret = mapper->get_next_objects_to_trim(snapid, max_count, &hoids);
-    if (ret == 0) {
-      out.insert(out.end(), hoids.begin(), hoids.end());
-      for (auto &&hoid: hoids) {
+
+    set<hobject_t>& hobjects = snap_to_hobject[snapid];
+    auto hoids = mapper->get_next_objects_to_trim(snapid, max_count);
+    if (hoids.has_value()) {
+      out.insert(out.end(), hoids->begin(), hoids->end());
+      for (auto &&hoid: *hoids) {
        ceph_assert(!hoid.is_max());
        ceph_assert(hobjects.count(hoid));
        hobjects.erase(hoid);
 
        map<hobject_t, set<snapid_t>>::iterator j = hobject_to_snap.find(hoid);
-       ceph_assert(j->second.count(snapid));
+       ceph_assert(j->second.contains(snapid));
        set<snapid_t> old_snaps(j->second);
        j->second.erase(snapid);
 
@@ -587,9 +587,9 @@ public:
        }
        hoid = hobject_t::get_max();
       }
-      hoids.clear();
+      return 0;
     }
-    return ret;
+    return -1;
   }
 
   // must be called with lock held to protect access to