From 32b514c52b37374202f20f588ef444144ba0e2c4 Mon Sep 17 00:00:00 2001 From: "Adam C. Emerson" Date: Wed, 14 Dec 2022 13:12:48 -0500 Subject: [PATCH] cls/fifo: Move version inc into `apply_update()` Only increment the version if we make an actual change. Return whether we have changed something or not so the OSD side can skip writing if there's no change. Fixes: https://tracker.ceph.com/issues/57562 Signed-off-by: Adam C. Emerson --- src/cls/fifo/cls_fifo.cc | 23 ++++++++------- src/cls/fifo/cls_fifo_types.h | 39 ++++++++++++++++--------- src/rgw/driver/rados/cls_fifo_legacy.cc | 3 +- 3 files changed, 39 insertions(+), 26 deletions(-) diff --git a/src/cls/fifo/cls_fifo.cc b/src/cls/fifo/cls_fifo.cc index 5fe3404b995..3cbb4636c9d 100644 --- a/src/cls/fifo/cls_fifo.cc +++ b/src/cls/fifo/cls_fifo.cc @@ -76,8 +76,7 @@ std::string new_oid_prefix(std::string id, std::optional& val) } int write_header(cls_method_context_t hctx, - info& header, - bool inc_ver = true) + info& header) { static constexpr auto HEADER_INSTANCE_SIZE = 16; if (header.version.instance.empty()) { @@ -86,9 +85,6 @@ int write_header(cls_method_context_t hctx, cls_gen_rand_base64(buf, sizeof(buf) - 1); header.version.instance = buf; } - if (inc_ver) { - ++header.version.ver; - } ceph::buffer::list bl; encode(header, bl); return cls_cxx_write_full(hctx, &bl); @@ -298,7 +294,7 @@ int create_meta(cls_method_context_t hctx, header.params.max_entry_size = op.max_entry_size; header.params.full_size_threshold = op.max_part_size - op.max_entry_size - part_entry_overhead; - r = write_header(hctx, header, false); + r = write_header(hctx, header); if (r < 0) { CLS_ERR("%s: failed to write header: r=%d", __PRETTY_FUNCTION__, r); return r; @@ -342,11 +338,16 @@ int update_meta(cls_method_context_t hctx, ceph::buffer::list* in, .journal_entries_rm( std::move(op.journal_entries_rm)); - header.apply_update(u); - r = write_header(hctx, header); - if (r < 0) { - CLS_ERR("%s: failed to write header: r=%d", __PRETTY_FUNCTION__, r); - return r; + auto changed = header.apply_update(u); + if (changed) { + r = write_header(hctx, header); + if (r < 0) { + CLS_ERR("%s: failed to write header: r=%d", __PRETTY_FUNCTION__, r); + return r; + } + } else { + CLS_LOG(10, "%s: No change, nothing to write.", + __PRETTY_FUNCTION__); } return 0; diff --git a/src/cls/fifo/cls_fifo_types.h b/src/cls/fifo/cls_fifo_types.h index c2cb5b62796..89ba1cc7102 100644 --- a/src/cls/fifo/cls_fifo_types.h +++ b/src/cls/fifo/cls_fifo_types.h @@ -173,16 +173,16 @@ inline std::ostream& operator <<(std::ostream& m, const journal_entry& j) { // This is actually a useful builder, since otherwise we end up with // four uint64_ts in a row and only care about a subset at a time. class update { - std::optional tail_part_num_; - std::optional head_part_num_; - std::optional min_push_part_num_; - std::optional max_push_part_num_; + std::optional tail_part_num_; + std::optional head_part_num_; + std::optional min_push_part_num_; + std::optional max_push_part_num_; std::vector journal_entries_add_; std::vector journal_entries_rm_; public: - update&& tail_part_num(std::optional num) noexcept { + update&& tail_part_num(std::optional num) noexcept { tail_part_num_ = num; return std::move(*this); } @@ -190,7 +190,7 @@ public: return tail_part_num_; } - update&& head_part_num(std::optional num) noexcept { + update&& head_part_num(std::optional num) noexcept { head_part_num_ = num; return std::move(*this); } @@ -198,7 +198,7 @@ public: return head_part_num_; } - update&& min_push_part_num(std::optional num) + update&& min_push_part_num(std::optional num) noexcept { min_push_part_num_ = num; return std::move(*this); @@ -207,7 +207,7 @@ public: return min_push_part_num_; } - update&& max_push_part_num(std::optional num) noexcept { + update&& max_push_part_num(std::optional num) noexcept { max_push_part_num_ = num; return std::move(*this); } @@ -359,17 +359,23 @@ struct info { return fmt::format("{}.{}", oid_prefix, part_num); } - void apply_update(const update& update) { - if (update.tail_part_num()) { + bool apply_update(const update& update) { + bool changed = false; + if (update.tail_part_num() && (tail_part_num != *update.tail_part_num())) { tail_part_num = *update.tail_part_num(); + changed = true; } - if (update.min_push_part_num()) { + if (update.min_push_part_num() && + (min_push_part_num != *update.min_push_part_num())) { min_push_part_num = *update.min_push_part_num(); + changed = true; } - if (update.max_push_part_num()) { + if (update.max_push_part_num() && + (max_push_part_num != *update.max_push_part_num())) { max_push_part_num = *update.max_push_part_num(); + changed = true; } for (const auto& entry : update.journal_entries_add()) { @@ -379,16 +385,23 @@ struct info { continue; } else { journal.emplace(entry.part_num, entry); + changed = true; } } for (const auto& entry : update.journal_entries_rm()) { journal.erase(entry.part_num); + changed = true; } - if (update.head_part_num()) { + if (update.head_part_num() && (head_part_num != *update.head_part_num())) { head_part_num = *update.head_part_num(); + changed = true; } + if (changed) { + ++version.ver; + } + return changed; } }; WRITE_CLASS_ENCODER(info) diff --git a/src/rgw/driver/rados/cls_fifo_legacy.cc b/src/rgw/driver/rados/cls_fifo_legacy.cc index a6e04f10f3b..9946cfa413d 100644 --- a/src/rgw/driver/rados/cls_fifo_legacy.cc +++ b/src/rgw/driver/rados/cls_fifo_legacy.cc @@ -421,9 +421,8 @@ int FIFO::apply_update(const DoutPrefixProvider *dpp, << " version mismatch, canceling: tid=" << tid << dendl; return -ECANCELED; } - info->apply_update(update); - ++info->version.ver; + info->apply_update(update); return {}; } -- 2.39.5