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 <aemerson@redhat.com>
(cherry picked from commit 32b514c52b37374202f20f588ef444144ba0e2c4)
Fixes: https://tracker.ceph.com/issues/58402
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 5fe3404b995908925834b393fd7fa84e1b2c974c..3cbb4636c9d9f28e027c1eac98948bf5f85ab060 100644 (file)
--- 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<std::string>& 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 b8f6ceec87be716f315be870bf7ee2596262aa68..027c22d372784e92729f25dcb79f909489944d29 100644 (file)
--- a/src/cls/fifo/cls_fifo_types.h
+++ b/src/cls/fifo/cls_fifo_types.h
@@ -178,16 +178,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<std::uint64_t> tail_part_num_;
-  std::optional<std::uint64_t> head_part_num_;
-  std::optional<std::uint64_t> min_push_part_num_;
-  std::optional<std::uint64_t> max_push_part_num_;
+  std::optional<std::int64_t> tail_part_num_;
+  std::optional<std::int64_t> head_part_num_;
+  std::optional<std::int64_t> min_push_part_num_;
+  std::optional<std::int64_t> max_push_part_num_;
   std::vector<fifo::journal_entry> journal_entries_add_;
   std::vector<fifo::journal_entry> journal_entries_rm_;
 
 public:
 
-  update&& tail_part_num(std::optional<std::uint64_t> num) noexcept {
+  update&& tail_part_num(std::optional<std::int64_t> num) noexcept {
     tail_part_num_ = num;
     return std::move(*this);
   }
@@ -195,7 +195,7 @@ public:
     return tail_part_num_;
   }
 
-  update&& head_part_num(std::optional<std::uint64_t> num) noexcept {
+  update&& head_part_num(std::optional<std::int64_t> num) noexcept {
     head_part_num_ = num;
     return std::move(*this);
   }
@@ -203,7 +203,7 @@ public:
     return head_part_num_;
   }
 
-  update&& min_push_part_num(std::optional<std::uint64_t> num)
+  update&& min_push_part_num(std::optional<std::int64_t> num)
     noexcept {
     min_push_part_num_ = num;
     return std::move(*this);
@@ -212,7 +212,7 @@ public:
     return min_push_part_num_;
   }
 
-  update&& max_push_part_num(std::optional<std::uint64_t> num) noexcept {
+  update&& max_push_part_num(std::optional<std::int64_t> num) noexcept {
     max_push_part_num_ = num;
     return std::move(*this);
   }
@@ -364,17 +364,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()) {
@@ -384,16 +390,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/cls_fifo_legacy.cc b/src/rgw/cls_fifo_legacy.cc
index 42e1af391e5d78b375bfad8afbfdd10fecf2494d..a29b8813bd6a8ae7bf34af582a02f0cd17a2e6ce 100644 (file)
--- a/src/rgw/cls_fifo_legacy.cc
+++ b/src/rgw/cls_fifo_legacy.cc
@@ -423,9 +423,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 {};
 }