From: Matan Breizman <mbreizma@redhat.com>
Date: Sun, 7 Dec 2025 10:36:28 +0000 (+0000)
Subject: crimson/os/seastore/cache: Verify crc prior to complete_io
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=95d0c2cc0c5db52e80015094645b4b0d43b360be;p=ceph.git

crimson/os/seastore/cache: Verify crc prior to complete_io

Calling check_full_extent_integrity in pin_to_extent is wrong.
By the time we partially read the extent, another transaction
might fully load the entire extent. Either by a full_extent read
or by filling the missing unloaded extent gap.
This would result in veryfing the extent crc since it's fully
loaded. However, since the extent was only partially read our
data (and crc) might be outdated.
Instead move the crc checks to read_extent prior to complete_io.
That way we would only check the crc when the extent was intended to
be fully loaded. Other users of read_extent which do not use pin_crc
(CRC_NULL), would skip this check.

Fixes: https://tracker.ceph.com/issues/73790
Fixes: https://tracker.ceph.com/issues/72864

Signed-off-by: Zhang Song <zhangsong02@qianxin.com>
Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
---

diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc
index 6306def1397a..5b8371f0ab83 100644
--- a/src/crimson/os/seastore/cache.cc
+++ b/src/crimson/os/seastore/cache.cc
@@ -1081,6 +1081,21 @@ void Cache::on_transaction_destruct(Transaction& t)
   }
 }
 
+void Cache::check_full_extent_integrity(
+  uint32_t ref_crc, uint32_t pin_crc)
+{
+  LOG_PREFIX(Cache::check_full_extent_integrity);;
+  DEBUG("checksum in the lba tree: 0x{:x}, actual checksum: 0x{:x}",
+    pin_crc,
+    ref_crc);
+  if (unlikely(pin_crc != ref_crc)) {
+    ERROR("extent checksum inconsistent, recorded: 0x{:x}, actual: 0x{:x}",
+      pin_crc,
+      ref_crc);
+      ceph_abort_msg("extent checksum inconsistent");
+  }
+}
+
 CachedExtentRef Cache::alloc_new_non_data_extent_by_type(
   Transaction &t,        ///< [in, out] current transaction
   extent_types_t type,   ///< [in] type tag
diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h
index 1977a0fe7647..9561bd1516b8 100644
--- a/src/crimson/os/seastore/cache.h
+++ b/src/crimson/os/seastore/cache.h
@@ -1890,6 +1890,9 @@ private:
   /// Introspect transaction when it is being destructed
   void on_transaction_destruct(Transaction& t);
 
+  static void check_full_extent_integrity(
+    uint32_t ref_crc, uint32_t pin_crc);
+
   /// Read the extent in range offset~length,
   /// must be called exclusively for an extent,
   /// also see do_read_extent_maybe_partial().
@@ -1945,6 +1948,17 @@ private:
             extent->on_clean_read();
             SUBDEBUG(seastore_cache, "read extent 0x{:x}~0x{:x} done -- {}",
               offset, length, *extent);
+
+            if (pin_crc != CRC_NULL) {
+              SUBDEBUG(seastore_cache, "read extent 0x{:x}~0x{:x} veryfing integrity -- {}",
+                offset, length, *extent);
+              // We must check the integrity here prior to complete_io.
+              // Previously, concurrent transaction could have checked
+              // crc of non matching extent data.
+              // See: https://tracker.ceph.com/issues/73790
+              assert(extent->is_fully_loaded());
+              check_full_extent_integrity(extent->last_committed_crc, pin_crc);
+            }
           } else {
             extent->last_committed_crc = CRC_NULL;
             SUBDEBUG(seastore_cache, "read extent 0x{:x}~0x{:x} done (partial) -- {}",
diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc
index 8c0b5f0d3823..ca1a45af9a77 100644
--- a/src/crimson/os/seastore/transaction_manager.cc
+++ b/src/crimson/os/seastore/transaction_manager.cc
@@ -625,25 +625,6 @@ TransactionManager::do_submit_transaction(
   );
 }
 
-void TransactionManager::check_full_extent_integrity(
-  Transaction &t, uint32_t ref_crc, uint32_t pin_crc)
-{
-  LOG_PREFIX(TransactionManager::check_full_extent_integrity);
-  SUBDEBUGT(seastore_tm,
-    "checksum in the lba tree: 0x{:x}, actual checksum: 0x{:x}",
-    t,
-    pin_crc,
-    ref_crc);
-  if (unlikely(pin_crc != ref_crc)) {
-    SUBERRORT(seastore_tm,
-      "extent checksum inconsistent, recorded: 0x{:x}, actual: 0x{:x}",
-      t,
-      pin_crc,
-      ref_crc);
-      ceph_abort_msg("extent checksum inconsistent");
-  }
-}
-
 seastar::future<> TransactionManager::flush(OrderingHandle &handle)
 {
   LOG_PREFIX(TransactionManager::flush);
diff --git a/src/crimson/os/seastore/transaction_manager.h b/src/crimson/os/seastore/transaction_manager.h
index d1c1c1e6e879..7b52fef4f484 100644
--- a/src/crimson/os/seastore/transaction_manager.h
+++ b/src/crimson/os/seastore/transaction_manager.h
@@ -1469,11 +1469,6 @@ private:
       }),
       pin.get_checksum()
     );
-    if (ref->is_fully_loaded()) {
-      check_full_extent_integrity(t, ref->calc_crc32c(), pin.get_checksum());
-    } else {
-      assert(!full_extent_integrity_check);
-    }
 
     SUBDEBUGT(seastore_tm, "got extent -- {} fully_loaded: {}",
               t, *ref, ref->is_fully_loaded());
@@ -1481,9 +1476,6 @@ private:
     co_return std::move(ref);
   }
 
-  static void check_full_extent_integrity(
-    Transaction &t, uint32_t ref_crc, uint32_t pin_crc);
-
   /**
    * pin_to_extent_by_type
    *
@@ -1530,7 +1522,6 @@ private:
     SUBDEBUGT(seastore_tm, "got extent -- {} fully_loaded: {}",
               t, *ref, ref->is_fully_loaded());
     assert(ref->is_fully_loaded());
-    check_full_extent_integrity(t, ref->calc_crc32c(), pin.get_checksum());
     co_return ref->template cast<LogicalChildNode>();
   }