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>

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 c2cb5b62796bd7783bf89010198bcdf91ceb8b8b..89ba1cc7102f01352ad07b1d97358ff8ee02d893 100644 (file)
--- 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<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);
   }
@@ -190,7 +190,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);
   }
@@ -198,7 +198,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);
@@ -207,7 +207,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);
   }
@@ -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 a6e04f10f3b8aedb10481364e47f15adaaa17a3f..9946cfa413dfd0b90138c79c2de624ddd9a64d56 100644 (file)
--- 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 {};
 }