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 6306def1397a1d6c3edff40d0638ab2a9980e2a9..5b8371f0ab834979ed90d8b8659c3fa7c93e0b0a 100644 (file)
--- 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 1977a0fe7647c7e98b897856504cd41defaf4393..9561bd1516b884bc3448139a72bac12e049e6673 100644 (file)
--- 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 8c0b5f0d3823470ef2c4d1e3948b497c3ce9d72e..ca1a45af9a77431ae68831c2d138406413c31414 100644 (file)
--- 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 d1c1c1e6e8791797c21903a4d35c059a4f98baca..7b52fef4f484fad98ae3ebc11d9e90de44f8e4b5 100644 (file)
--- 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>();
   }