From 71ae2dd27ae07e6a5d2fda12834158fd2720bd7b Mon Sep 17 00:00:00 2001 From: "Adam C. Emerson" Date: Thu, 20 May 2021 19:19:55 -0400 Subject: [PATCH] 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 --- src/cls/fifo/cls_fifo.cc | 17 +++-- src/rgw/rgw_log_backing.cc | 107 ++++++++++++------------------- src/rgw/rgw_log_backing.h | 4 ++ src/test/rgw/test_log_backing.cc | 13 ---- 4 files changed, 56 insertions(+), 85 deletions(-) diff --git a/src/cls/fifo/cls_fifo.cc b/src/cls/fifo/cls_fifo.cc index fc89a20e6b2b..14313a735165 100644 --- 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 1c20cc8791bb..415ad1363d52 100644 --- 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 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 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 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 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 @@ -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 5b9f1bfd21cd..29b315b21642 100644 --- 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 f1bc30c762ab..c67debb9f59e 100644 --- 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( @@ -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) -- 2.47.3