crimson/os/seastore: fix bugs in test_map_existing_extent_concurrent

Signed-off-by: Zhang Song <zhangsong325@gmail.com>

diff --git a/src/crimson/os/seastore/transaction_manager.h b/src/crimson/os/seastore/transaction_manager.h
index d5c153df43cf951a4c7922bc1a0a6f8a77cc969a..6a5b82f047f489c8efd8b0009946c88a606800e4 100644 (file)
--- a/src/crimson/os/seastore/transaction_manager.h
+++ b/src/crimson/os/seastore/transaction_manager.h
@@ -388,6 +388,8 @@ public:
     ceph_assert(existing_paddr.is_absolute());
     assert(t.is_retired(existing_paddr, length));
 
+    SUBDEBUGT(seastore_tm, " laddr_hint: {} existing_paddr: {} length: {}",
+             t, laddr_hint, existing_paddr, length);
     auto bp = ceph::bufferptr(buffer::create_page_aligned(length));
     bp.zero();
 
@@ -407,9 +409,7 @@ public:
       laddr_hint,
       length,
       existing_paddr
-    ).si_then([ext=std::move(ext), laddr_hint, &t, this, FNAME](auto &&ref) {
-      SUBDEBUGT(seastore_tm, "map existing extent: {}, laddr_hint: {} pin: {}",
-               t, *ext, laddr_hint, *ref);
+    ).si_then([ext=std::move(ext), laddr_hint, this](auto &&ref) {
       ceph_assert(laddr_hint == ref->get_key());
       ext->set_pin(std::move(ref));
       return epm->read(

diff --git a/src/test/crimson/seastore/test_transaction_manager.cc b/src/test/crimson/seastore/test_transaction_manager.cc
index 34f379511289d8d55a142e6ad66070c331eb0ea5..e3b0f486619b06e18c888615a3fd45caca5f0785 100644 (file)
--- a/src/test/crimson/seastore/test_transaction_manager.cc
+++ b/src/test/crimson/seastore/test_transaction_manager.cc
@@ -754,18 +754,27 @@ struct transaction_manager_test_t :
     });
   }
 
-  TestBlockRef map_existing_extent(
+  std::optional<TestBlockRef> map_existing_extent(
     test_transaction_t &t,
     laddr_t hint,
     paddr_t existing_paddr,
     extent_len_t length) {
+    if (t.t->is_conflicted()) {
+      return std::nullopt;
+    }
     auto extent = with_trans_intr(*(t.t), [&](auto& trans) {
       return tm->map_existing_extent<TestBlock>(trans, hint, existing_paddr, length);
-    }).unsafe_get0();
+    }).handle_error(crimson::ct_error::eagain::handle([] {
+      return TCachedExtentRef<TestBlock>(new TestBlock(0));
+    }), crimson::ct_error::pass_further_all{}).unsafe_get0();
+    if (t.t->is_conflicted()) {
+      return std::nullopt;
+    }
+    EXPECT_TRUE(extent->get_length() != 0);
     EXPECT_FALSE(test_mappings.contains(extent->get_laddr(), t.mapping_delta));
     EXPECT_EQ(length, extent->get_length());
     test_mappings.alloced(hint, *extent, t.mapping_delta);
-    return extent;
+    return std::make_optional(std::move(extent));
   }
 
   void test_map_existing_extent() {
@@ -783,14 +792,17 @@ struct transaction_manager_test_t :
        auto base_paddr = extent->get_paddr();
        dec_ref(t, offset);
        auto extent1 = map_existing_extent(t, offset, base_paddr, 4 << 10);
+       ASSERT_TRUE(extent1.has_value());
        auto extent2 = map_existing_extent(t, offset + (4 << 10), base_paddr.add_offset(4 << 10), 4 << 10);
+       ASSERT_TRUE(extent2.has_value());
        auto extent3 = map_existing_extent(t, offset + (8 << 10), base_paddr.add_offset(8 << 10), 8 << 10);
-       ASSERT_TRUE(extent1->is_exist_clean());
-       ASSERT_TRUE(extent2->is_exist_clean());
-       ASSERT_TRUE(extent3->is_exist_clean());
-       auto extent4 = mutate_extent(t, extent3);
+       ASSERT_TRUE(extent3.has_value());
+       ASSERT_TRUE((*extent1)->is_exist_clean());
+       ASSERT_TRUE((*extent2)->is_exist_clean());
+       ASSERT_TRUE((*extent3)->is_exist_clean());
+       auto extent4 = mutate_extent(t, (*extent3));
        ASSERT_TRUE(extent4->is_exist_mutation_pending());
-       ASSERT_TRUE(extent3.get() == extent4.get());
+       ASSERT_TRUE((*extent3).get() == extent4.get());
        submit_transaction(std::move(t));
        check();
       }
@@ -812,6 +824,7 @@ struct transaction_manager_test_t :
       }
       int success = 0;
       int early_exit = 0;
+      int conflicted = 0;
 
       seastar::parallel_for_each(
         boost::make_counting_iterator(0u),
@@ -822,7 +835,7 @@ struct transaction_manager_test_t :
            std::set<uint32_t> split_points;
            for (uint32_t i = 0; i < pieces; i++) {
              auto p = std::uniform_int_distribution<>(1, 256)(gen);
-             split_points.insert((p - p % 4)%length);
+             split_points.insert(p - p % 4);
            }
 
            auto t = create_transaction();
@@ -835,12 +848,18 @@ struct transaction_manager_test_t :
            dec_ref(t, offset);
 
            auto base = 0;
+           ASSERT_TRUE(!split_points.empty());
            for (auto off : split_points) {
              if (off == 0) {
                continue;
              }
 
-             auto ext = map_existing_extent(t, base << 10, paddr.add_offset(base << 10), (off - base) << 10);
+             auto ext_ = map_existing_extent(t, base << 10, paddr.add_offset(base << 10), (off - base) << 10);
+             if (!ext_) {
+               conflicted++;
+               return;
+             }
+             auto ext = *ext_;
              ASSERT_TRUE(ext->is_exist_clean());
              if (get_random_contents() % 2 == 0) {
                auto ext1 = mutate_extent(t, ext);
@@ -848,20 +867,34 @@ struct transaction_manager_test_t :
              }
              base = off;
            }
-           auto ext = map_existing_extent(t, base << 10, paddr.add_offset(base << 10), length - (base << 10));
-           ASSERT_TRUE(ext->is_exist_clean());
-           if (get_random_contents() % 2 == 0) {
-             auto ext1 = mutate_extent(t, ext);
-             ASSERT_TRUE(ext1->is_exist_mutation_pending());
-           }
 
-           success += try_submit_transaction(std::move(t));
+           base <<= 10;
+           if (base != length) {
+             auto ext_ = map_existing_extent(t, base, paddr.add_offset(base), length - base);
+             if (!ext_) {
+               conflicted++;
+               return;
+             }
+             auto ext = *ext_;
+             ASSERT_TRUE(ext->is_exist_clean());
+             if (get_random_contents() % 2 == 0) {
+               auto ext1 = mutate_extent(t, ext);
+               ASSERT_TRUE(ext1->is_exist_mutation_pending());
+             }
+           }
+           if (try_submit_transaction(std::move(t))) {
+             success++;
+             logger().info("transaction {} submit the transction", static_cast<void*>(t.t.get()));
+           } else {
+             conflicted++;
+           }
          });
        }).handle_exception([](std::exception_ptr e) {
          logger().info("{}", e);
        }).get0();
+      logger().info("test_map_existing_extent_concurrent: early_exit {} conflicted {} success {}", early_exit, conflicted, success);
       ASSERT_TRUE(success == 1);
-      logger().info("test_map_existing_extent_concurrent: early_exit {}", early_exit);
+      ASSERT_EQ(success + conflicted + early_exit, REMAP_NUM);
       replay();
       check();
     });