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 <[email protected]>
mitsu-ko · Jan 9, 2023 · 32b514c · 32b514c
1 parent ade8909
commit 32b514c
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 26 deletions.
diff --git 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
@@ -173,32 +173,32 @@ 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);
   }
   auto tail_part_num() const noexcept {
     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);
   }
   auto head_part_num() const noexcept {
     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 @@ class update {
     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
@@ -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 {};
 }