From 7018c25d47edf7e12b581f7f28c2549fe73bde15 Mon Sep 17 00:00:00 2001 From: "Adam C. Emerson" Date: Wed, 10 Feb 2021 16:18:09 -0500 Subject: [PATCH] rgw: Wait until a generation has been empty for an hour to delete This fixes a problem where, while the backing handle remains allocated while a call completes, the objects it depends on may be deleted behind it. Signed-off-by: Adam C. Emerson --- src/rgw/rgw_datalog.cc | 2 +- src/rgw/rgw_log_backing.cc | 37 +++++++++++++++++++++----------- src/rgw/rgw_log_backing.h | 8 +++---- src/test/rgw/test_log_backing.cc | 24 ++++++++++----------- 4 files changed, 42 insertions(+), 29 deletions(-) diff --git a/src/rgw/rgw_datalog.cc b/src/rgw/rgw_datalog.cc index 404f7e9a0e573..2716a9c2bee3a 100644 --- a/src/rgw/rgw_datalog.cc +++ b/src/rgw/rgw_datalog.cc @@ -390,7 +390,7 @@ bs::error_code DataLogBackends::handle_init(entries_t e) noexcept { std::unique_lock l(m); for (const auto& [gen_id, gen] : e) { - if (gen.empty) { + if (gen.pruned) { lderr(datalog.cct) << __PRETTY_FUNCTION__ << ":" << __LINE__ << ": ERROR: given empty generation: gen_id=" << gen_id << dendl; diff --git a/src/rgw/rgw_log_backing.cc b/src/rgw/rgw_log_backing.cc index 67fc925586919..8ce88aa21414f 100644 --- a/src/rgw/rgw_log_backing.cc +++ b/src/rgw/rgw_log_backing.cc @@ -583,7 +583,7 @@ bs::error_code logback_generations::empty_to(uint64_t gen_id, } for (auto i = es.begin(); i < ei; ++i) { newtail = i->first; - i->second.empty = true; + i->second.pruned = ceph::real_clock::now(); } ec = write(std::move(es), std::move(l), y); ++tries; @@ -626,31 +626,44 @@ bs::error_code logback_generations::remove_empty(optional_yield y) noexcept { entries_t new_entries; std::unique_lock l(m); ceph_assert(!entries_.empty()); - auto i = lowest_nomempty(entries_); - if (i == entries_.begin()) { - return {}; + { + auto i = lowest_nomempty(entries_); + if (i == entries_.begin()) { + return {}; + } } - auto ln = i->first; entries_t es; - std::copy(entries_.cbegin(), i, - std::inserter(es, es.end())); + auto now = ceph::real_clock::now(); l.unlock(); do { + std::copy_if(entries_.cbegin(), entries_.cend(), + std::inserter(es, es.end()), + [now](const auto& e) { + if (!e.second.pruned) + return false; + + auto pruned = *e.second.pruned; + return (now - pruned) >= 1h; + }); + auto es2 = entries_; for (const auto& [gen_id, e] : es) { - ceph_assert(e.empty); + ceph_assert(e.pruned); auto ec = log_remove(ioctx, shards, [this, gen_id](int shard) { return this->get_oid(gen_id, shard); }, (gen_id == 0), y); if (ec) { - return ec; + lderr(cct) << __PRETTY_FUNCTION__ << ":" << __LINE__ + << ": Error pruning: gen_id=" << gen_id + << " ec=" << ec.message() << dendl; + } + if (auto i = es2.find(gen_id); i != es2.end()) { + es2.erase(i); } } l.lock(); - i = entries_.find(ln); es.clear(); - std::copy(i, entries_.cend(), std::inserter(es, es.end())); - ec = write(std::move(es), std::move(l), y); + ec = write(std::move(es2), std::move(l), y); ++tries; } while (ec == bs::errc::operation_canceled && tries < max_tries); diff --git a/src/rgw/rgw_log_backing.h b/src/rgw/rgw_log_backing.h index e592bc29b2bcf..d5996049e5873 100644 --- a/src/rgw/rgw_log_backing.h +++ b/src/rgw/rgw_log_backing.h @@ -95,13 +95,13 @@ bs::error_code log_remove(librados::IoCtx& ioctx, struct logback_generation { uint64_t gen_id = 0; log_type type; - bool empty = false; + std::optional pruned; void encode(ceph::buffer::list& bl) const { ENCODE_START(1, 1, bl); encode(gen_id, bl); encode(type, bl); - encode(empty, bl); + encode(pruned, bl); ENCODE_FINISH(bl); } @@ -109,7 +109,7 @@ struct logback_generation { DECODE_START(1, bl); decode(gen_id, bl); decode(type, bl); - decode(empty, bl); + decode(pruned, bl); DECODE_FINISH(bl); } }; @@ -157,7 +157,7 @@ private: auto lowest_nomempty(const entries_t& es) { return std::find_if(es.begin(), es.end(), [](const auto& e) { - return !e.second.empty; + return !e.second.pruned; }); } diff --git a/src/test/rgw/test_log_backing.cc b/src/test/rgw/test_log_backing.cc index 166de2dd8242c..95f1e613936b0 100644 --- a/src/test/rgw/test_log_backing.cc +++ b/src/test/rgw/test_log_backing.cc @@ -241,7 +241,7 @@ TEST_F(LogBacking, GenerationSingle) ASSERT_EQ(0, lg->got_entries[0].gen_id); ASSERT_EQ(log_type::fifo, lg->got_entries[0].type); - ASSERT_FALSE(lg->got_entries[0].empty); + ASSERT_FALSE(lg->got_entries[0].pruned); auto ec = lg->empty_to(0, null_yield); ASSERT_TRUE(ec); @@ -258,7 +258,7 @@ TEST_F(LogBacking, GenerationSingle) ASSERT_EQ(0, lg->got_entries[0].gen_id); ASSERT_EQ(log_type::fifo, lg->got_entries[0].type); - ASSERT_FALSE(lg->got_entries[0].empty); + ASSERT_FALSE(lg->got_entries[0].pruned); lg->got_entries.clear(); @@ -268,7 +268,7 @@ TEST_F(LogBacking, GenerationSingle) ASSERT_EQ(1, lg->got_entries.size()); 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].empty); + ASSERT_FALSE(lg->got_entries[1].pruned); lg.reset(); @@ -280,11 +280,11 @@ TEST_F(LogBacking, GenerationSingle) ASSERT_EQ(2, lg->got_entries.size()); ASSERT_EQ(0, lg->got_entries[0].gen_id); ASSERT_EQ(log_type::fifo, lg->got_entries[0].type); - ASSERT_FALSE(lg->got_entries[0].empty); + ASSERT_FALSE(lg->got_entries[0].pruned); 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].empty); + ASSERT_FALSE(lg->got_entries[1].pruned); ec = lg->empty_to(0, null_yield); ASSERT_FALSE(ec); @@ -301,7 +301,7 @@ TEST_F(LogBacking, GenerationSingle) ASSERT_EQ(1, lg->got_entries.size()); 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].empty); + ASSERT_FALSE(lg->got_entries[1].pruned); ec = lg->remove_empty(null_yield); ASSERT_FALSE(ec); @@ -311,7 +311,7 @@ TEST_F(LogBacking, GenerationSingle) ASSERT_EQ(1, entries[1].gen_id); ASSERT_EQ(log_type::omap, entries[1].type); - ASSERT_FALSE(entries[1].empty); + ASSERT_FALSE(entries[1].pruned); lg.reset(); } @@ -329,7 +329,7 @@ TEST_F(LogBacking, GenerationWN) ASSERT_EQ(1, lg1->got_entries.size()); ASSERT_EQ(1, lg1->got_entries[1].gen_id); ASSERT_EQ(log_type::omap, lg1->got_entries[1].type); - ASSERT_FALSE(lg1->got_entries[1].empty); + ASSERT_FALSE(lg1->got_entries[1].pruned); lg1->got_entries.clear(); @@ -342,11 +342,11 @@ TEST_F(LogBacking, GenerationWN) ASSERT_EQ(0, lg2->got_entries[0].gen_id); ASSERT_EQ(log_type::fifo, lg2->got_entries[0].type); - ASSERT_FALSE(lg2->got_entries[0].empty); + ASSERT_FALSE(lg2->got_entries[0].pruned); ASSERT_EQ(1, lg2->got_entries[1].gen_id); ASSERT_EQ(log_type::omap, lg2->got_entries[1].type); - ASSERT_FALSE(lg2->got_entries[1].empty); + ASSERT_FALSE(lg2->got_entries[1].pruned); lg2->got_entries.clear(); @@ -356,12 +356,12 @@ TEST_F(LogBacking, GenerationWN) ASSERT_EQ(1, lg1->got_entries.size()); ASSERT_EQ(2, lg1->got_entries[2].gen_id); ASSERT_EQ(log_type::fifo, lg1->got_entries[2].type); - ASSERT_FALSE(lg1->got_entries[2].empty); + ASSERT_FALSE(lg1->got_entries[2].pruned); ASSERT_EQ(1, lg2->got_entries.size()); ASSERT_EQ(2, lg2->got_entries[2].gen_id); ASSERT_EQ(log_type::fifo, lg2->got_entries[2].type); - ASSERT_FALSE(lg2->got_entries[2].empty); + ASSERT_FALSE(lg2->got_entries[2].pruned); lg1->got_entries.clear(); lg2->got_entries.clear(); -- 2.39.5