rgw: Simplify log shard probing and err on the side of omap

In the multigeneration version we no longer care whether entries
exist, since we never delete and recreate empty logs. Remove logic
that marked entirely empty shards as DNE under the assumption that
they would be deleted if so.

Fixes: https://tracker.ceph.com/issues/50169
Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

diff --git a/src/cls/fifo/cls_fifo.cc b/src/cls/fifo/cls_fifo.cc
index fc89a20e6b2bfd310e65757f7dc617d4229e9e7c..14313a7351655ed02094447881926ec109ec9837 100644 (file)
--- a/src/cls/fifo/cls_fifo.cc
+++ b/src/cls/fifo/cls_fifo.cc
@@ -217,7 +217,8 @@ int create_meta(cls_method_context_t hctx,
     auto iter = in->cbegin();
     decode(op, iter);
   } catch (const ceph::buffer::error& err) {
-    CLS_ERR("ERROR: %s: failed to decode request", __PRETTY_FUNCTION__);
+    CLS_ERR("ERROR: %s: failed to decode request: %s", __PRETTY_FUNCTION__,
+           err.what());
     return -EINVAL;
   }
 
@@ -237,19 +238,24 @@ int create_meta(cls_method_context_t hctx,
 
   int r = cls_cxx_stat2(hctx, &size, nullptr);
   if (r < 0 && r != -ENOENT) {
-    CLS_ERR("ERROR: %s: cls_cxx_stat2() on obj returned %d", __PRETTY_FUNCTION__, r);
+    CLS_ERR("ERROR: %s: cls_cxx_stat2() on obj returned %d",
+           __PRETTY_FUNCTION__, r);
     return r;
   }
   if (op.exclusive && r == 0) {
-    CLS_ERR("%s: exclusive create but queue already exists", __PRETTY_FUNCTION__);
+    CLS_ERR("%s: exclusive create but queue already exists",
+           __PRETTY_FUNCTION__);
     return -EEXIST;
   }
 
   if (r == 0) {
+    CLS_LOG(5, "%s: FIFO already exists, reading from disk and comparing.",
+           __PRETTY_FUNCTION__);
     ceph::buffer::list bl;
     r = cls_cxx_read2(hctx, 0, size, &bl, CEPH_OSD_OP_FLAG_FADVISE_WILLNEED);
     if (r < 0) {
-      CLS_ERR("ERROR: %s: cls_cxx_read2() on obj returned %d", __PRETTY_FUNCTION__, r);
+      CLS_ERR("ERROR: %s: cls_cxx_read2() on obj returned %d",
+             __PRETTY_FUNCTION__, r);
       return r;
     }
 
@@ -258,7 +264,8 @@ int create_meta(cls_method_context_t hctx,
       auto iter = bl.cbegin();
       decode(header, iter);
     } catch (const ceph::buffer::error& err) {
-      CLS_ERR("ERROR: %s: failed decoding header", __PRETTY_FUNCTION__);
+      CLS_ERR("ERROR: %s: failed decoding header: %s",
+             __PRETTY_FUNCTION__, err.what());
       return -EIO;
     }
 

diff --git a/src/rgw/rgw_log_backing.cc b/src/rgw/rgw_log_backing.cc
index 1c20cc8791bb1592ba0d88202bca1da2ceb84546..415ad1363d5281a53f6533b380afa06b36e50888 100644 (file)
--- a/src/rgw/rgw_log_backing.cc
+++ b/src/rgw/rgw_log_backing.cc
@@ -30,81 +30,54 @@ inline std::ostream& operator <<(std::ostream& m, const shard_check& t) {
 
 namespace {
 /// Return the shard type, and a bool to see whether it has entries.
-std::pair<shard_check, bool>
+shard_check
 probe_shard(const DoutPrefixProvider *dpp, librados::IoCtx& ioctx, const std::string& oid,
            bool& fifo_unsupported, optional_yield y)
 {
-  bool omap = false;
-  {
-    librados::ObjectReadOperation op;
-    cls_log_header header;
-    cls_log_info(op, &header);
-    auto r = rgw_rados_operate(dpp, ioctx, oid, &op, nullptr, y);
-    if (r == -ENOENT) {
-      return { shard_check::dne, {} };
-    }
-
-    if (r < 0) {
-      ldpp_dout(dpp, -1) << __PRETTY_FUNCTION__ << ":" << __LINE__
-                << " error probing for omap: r=" << r
-                << ", oid=" << oid << dendl;
-      return { shard_check::corrupt, {} };
-    }
-    if (header != cls_log_header{})
-      omap = true;
-  }
+  ldpp_dout(dpp, 20) << __PRETTY_FUNCTION__ << ":" << __LINE__
+                    << " probing oid=" << oid
+                    << dendl;
   if (!fifo_unsupported) {
     std::unique_ptr<rgw::cls::fifo::FIFO> fifo;
     auto r = rgw::cls::fifo::FIFO::open(dpp, ioctx, oid,
                                        &fifo, y,
                                        std::nullopt, true);
-    if (r < 0 && !(r == -ENOENT || r == -ENODATA || r == -EPERM)) {
-      ldpp_dout(dpp, -1) << __PRETTY_FUNCTION__ << ":" << __LINE__
-                << " error probing for fifo: r=" << r
-                << ", oid=" << oid << dendl;
-      return { shard_check::corrupt, {} };
-    }
-    if (fifo && omap) {
-      ldpp_dout(dpp, -1) << __PRETTY_FUNCTION__ << ":" << __LINE__
-                << " fifo and omap found: oid=" << oid << dendl;
-      return { shard_check::corrupt, {} };
-    }
-    if (fifo) {
-      bool more = false;
-      std::vector<rgw::cls::fifo::list_entry> entries;
-      r = fifo->list(dpp, 1, nullopt, &entries, &more, y);
-      if (r < 0) {
-       ldpp_dout(dpp, -1) << __PRETTY_FUNCTION__ << ":" << __LINE__
-                  << ": unable to list entries: r=" << r
-                  << ", oid=" << oid << dendl;
-       return { shard_check::corrupt, {} };
-      }
-      return { shard_check::fifo, !entries.empty() };
-    }
-    if (r == -EPERM) {
-      // Returned by OSD id CLS module not loaded.
+    switch (r) {
+    case 0:
+      ldpp_dout(dpp, 20) << __PRETTY_FUNCTION__ << ":" << __LINE__
+                        << ": oid=" << oid << " is FIFO"
+                        << dendl;
+      return shard_check::fifo;
+
+    case -ENODATA:
+      ldpp_dout(dpp, 20) << __PRETTY_FUNCTION__ << ":" << __LINE__
+                        << ": oid=" << oid << " is empty and therefore OMAP"
+                        << dendl;
+      return shard_check::omap;
+
+    case -ENOENT:
+      ldpp_dout(dpp, 20) << __PRETTY_FUNCTION__ << ":" << __LINE__
+                        << ": oid=" << oid << " does not exist"
+                        << dendl;
+      return shard_check::dne;
+
+    case -EPERM:
+      ldpp_dout(dpp, 20) << __PRETTY_FUNCTION__ << ":" << __LINE__
+                        << ": FIFO is unsupported, marking."
+                        << dendl;
       fifo_unsupported = true;
-    }
-  }
-  if (omap) {
-    std::list<cls_log_entry> entries;
-    std::string out_marker;
-    bool truncated = false;
-    librados::ObjectReadOperation op;
-    cls_log_list(op, {}, {}, {}, 1, entries,
-                &out_marker, &truncated);
-    auto r = rgw_rados_operate(dpp, ioctx, oid, &op, nullptr, y);
-    if (r < 0) {
+      return shard_check::omap;
+
+    default:
       ldpp_dout(dpp, -1) << __PRETTY_FUNCTION__ << ":" << __LINE__
-                << ": failed to list: r=" << r << ", oid=" << oid << dendl;
-      return { shard_check::corrupt, {} };
+                        << ": error probing: r=" << r
+                        << ", oid=" << oid << dendl;
+      return shard_check::corrupt;
     }
-    return { shard_check::omap, !entries.empty() };
+  } else {
+    // Since FIFO is unsupported, OMAP is the only alternative
+    return shard_check::omap;
   }
-
-  // An object exists, but has never had FIFO or cls_log entries written
-  // to it. Likely just the marker Omap.
-  return { shard_check::dne, {} };
 }
 
 tl::expected<log_type, bs::error_code>
@@ -147,7 +120,7 @@ log_backing_type(const DoutPrefixProvider *dpp,
   auto check = shard_check::dne;
   bool fifo_unsupported = false;
   for (int i = 0; i < shards; ++i) {
-    auto [c, e] = probe_shard(dpp, ioctx, get_oid(i), fifo_unsupported, y);
+    auto c = probe_shard(dpp, ioctx, get_oid(i), fifo_unsupported, y);
     if (c == shard_check::corrupt)
       return tl::unexpected(bs::error_code(EIO, bs::system_category()));
     if (c == shard_check::dne) continue;
@@ -660,13 +633,13 @@ bs::error_code logback_generations::remove_empty(const DoutPrefixProvider *dpp,
       for (const auto& [gen_id, e] : es) {
        ceph_assert(e.pruned);
        auto ec = log_remove(dpp, ioctx, shards,
-                            [this, gen_id=gen_id](int shard) {
+                            [this, gen_id = gen_id](int shard) {
                               return this->get_oid(gen_id, shard);
                             }, (gen_id == 0), y);
        if (ec) {
          ldpp_dout(dpp, -1) << __PRETTY_FUNCTION__ << ":" << __LINE__
-                    << ": Error pruning: gen_id=" << gen_id
-                    << " ec=" << ec.message() << dendl;
+                            << ": Error pruning: gen_id=" << gen_id
+                            << " ec=" << ec.message() << dendl;
        }
        if (auto i = es2.find(gen_id); i != es2.end()) {
          es2.erase(i);

diff --git a/src/rgw/rgw_log_backing.h b/src/rgw/rgw_log_backing.h
index 5b9f1bfd21cd0b42cd86820ab824127d8e21f87e..29b315b21642b9ca87b2a6d386974f8a3a550e22 100644 (file)
--- a/src/rgw/rgw_log_backing.h
+++ b/src/rgw/rgw_log_backing.h
@@ -115,6 +115,10 @@ struct logback_generation {
   }
 };
 WRITE_CLASS_ENCODER(logback_generation)
+inline std::ostream& operator <<(std::ostream& m, const logback_generation& g) {
+  return m << "[" << g.gen_id << "," << g.type << ","
+          << (g.pruned ? "PRUNED" : "NOT PRUNED") << "]";
+}
 
 class logback_generations : public librados::WatchCtx2 {
 public:

diff --git a/src/test/rgw/test_log_backing.cc b/src/test/rgw/test_log_backing.cc
index f1bc30c762abbd62f41e733fcafb6944cc3a5f1d..c67debb9f59e02552ef84af9443cbfd77d87c5bc 100644 (file)
--- a/src/test/rgw/test_log_backing.cc
+++ b/src/test/rgw/test_log_backing.cc
@@ -249,7 +249,6 @@ TEST_F(LogBacking, GenerationSingle)
   auto ec = lg->empty_to(&dp, 0, null_yield);
   ASSERT_TRUE(ec);
 
-
   lg.reset();
 
   lg = *logback_generations::init<generations>(
@@ -305,18 +304,6 @@ TEST_F(LogBacking, GenerationSingle)
   ASSERT_EQ(1, lg->got_entries[1].gen_id);
   ASSERT_EQ(log_type::omap, lg->got_entries[1].type);
   ASSERT_FALSE(lg->got_entries[1].pruned);
-
-  ec = lg->remove_empty(&dp, null_yield);
-  ASSERT_FALSE(ec);
-
-  auto entries = lg->entries();
-  ASSERT_EQ(1, entries.size());
-
-  ASSERT_EQ(1, entries[1].gen_id);
-  ASSERT_EQ(log_type::omap, entries[1].type);
-  ASSERT_FALSE(entries[1].pruned);
-
-  lg.reset();
 }
 
 TEST_F(LogBacking, GenerationWN)