From 4c4405ee272bfccda1e6746d1093aefb6ea6e324 Mon Sep 17 00:00:00 2001 From: Yingxin Cheng Date: Mon, 28 Jun 2021 22:12:46 +0800 Subject: [PATCH] crimson/os/seastore/cache: cleanup try_construct_record() optional return Signed-off-by: Yingxin Cheng --- src/crimson/os/seastore/cache.cc | 4 ++-- src/crimson/os/seastore/cache.h | 7 ++---- .../os/seastore/transaction_manager.cc | 3 +-- .../seastore/test_btree_lba_manager.cc | 6 +---- .../crimson/seastore/test_seastore_cache.cc | 23 ++++++------------- 5 files changed, 13 insertions(+), 30 deletions(-) diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc index 7c9d883c574..ce4df936045 100644 --- a/src/crimson/os/seastore/cache.cc +++ b/src/crimson/os/seastore/cache.cc @@ -235,7 +235,7 @@ CachedExtentRef Cache::duplicate_for_write( return ret; } -std::optional Cache::try_construct_record(Transaction &t) +record_t Cache::try_construct_record(Transaction &t) { LOG_PREFIX(Cache::try_construct_record); DEBUGT("enter", t); @@ -343,7 +343,7 @@ std::optional Cache::try_construct_record(Transaction &t) }); } - return std::make_optional(std::move(record)); + return record; } void Cache::complete_commit( diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h index 1a9998cc7f2..551f6e7d0a7 100644 --- a/src/crimson/os/seastore/cache.h +++ b/src/crimson/os/seastore/cache.h @@ -372,12 +372,9 @@ public: /** * try_construct_record * - * First checks for conflicts. If a racing write has mutated/retired - * an extent mutated by this transaction, nullopt will be returned. - * - * Otherwise, a record will be returned valid for use with Journal. + * Construct the record for Journal from transaction. */ - std::optional try_construct_record( + record_t try_construct_record( Transaction &t ///< [in, out] current transaction ); diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc index 8f34581eb44..688e3e80397 100644 --- a/src/crimson/os/seastore/transaction_manager.cc +++ b/src/crimson/os/seastore/transaction_manager.cc @@ -240,13 +240,12 @@ TransactionManager::submit_transaction_direct( ).then_interruptible([this, FNAME, &tref]() mutable -> submit_transaction_iertr::future<> { auto record = cache->try_construct_record(tref); - assert(record); // interruptible future would have already failed tref.get_handle().maybe_release_collection_lock(); DEBUGT("about to submit to journal", tref); - return journal->submit_record(std::move(*record), tref.get_handle() + return journal->submit_record(std::move(record), tref.get_handle() ).safe_then([this, FNAME, &tref](auto p) mutable { auto [addr, journal_seq] = p; DEBUGT("journal commit to {} seq {}", tref, addr, journal_seq); diff --git a/src/test/crimson/seastore/test_btree_lba_manager.cc b/src/test/crimson/seastore/test_btree_lba_manager.cc index a4b6d1db339..175ca86eaff 100644 --- a/src/test/crimson/seastore/test_btree_lba_manager.cc +++ b/src/test/crimson/seastore/test_btree_lba_manager.cc @@ -59,11 +59,7 @@ struct btree_lba_manager_test : seastar::future<> submit_transaction(TransactionRef t) { auto record = cache.try_construct_record(*t); - if (!record) { - ceph_assert(0 == "cannot fail"); - } - - return journal.submit_record(std::move(*record), t->get_handle()).safe_then( + return journal.submit_record(std::move(record), t->get_handle()).safe_then( [this, t=std::move(t)](auto p) mutable { auto [addr, seq] = p; cache.complete_commit(*t, addr, seq); diff --git a/src/test/crimson/seastore/test_seastore_cache.cc b/src/test/crimson/seastore/test_seastore_cache.cc index 31b77209090..09cf6dc0562 100644 --- a/src/test/crimson/seastore/test_seastore_cache.cc +++ b/src/test/crimson/seastore/test_seastore_cache.cc @@ -29,16 +29,12 @@ struct cache_test_t : public seastar_test_suite_t { : segment_manager(segment_manager::create_test_ephemeral()), cache(*segment_manager) {} - seastar::future> submit_transaction( + seastar::future submit_transaction( TransactionRef t) { auto record = cache.try_construct_record(*t); - if (!record) { - return seastar::make_ready_future>( - std::nullopt); - } bufferlist bl; - for (auto &&block : record->extents) { + for (auto &&block : record.extents) { bl.append(block.bl); } @@ -57,7 +53,7 @@ struct cache_test_t : public seastar_test_suite_t { ).safe_then( [this, prev, t=std::move(t)]() mutable { cache.complete_commit(*t, prev, seq /* TODO */); - return seastar::make_ready_future>(prev); + return prev; }, crimson::ct_error::all_same_way([](auto e) { ASSERT_FALSE("failed to submit"); @@ -90,9 +86,7 @@ struct cache_test_t : public seastar_test_suite_t { return cache.mkfs(*transaction).safe_then( [this, &transaction] { return submit_transaction(std::move(transaction)).then( - [](auto p) { - ASSERT_TRUE(p); - }); + [](auto p) {}); }); }); }).handle_error( @@ -120,8 +114,7 @@ TEST_F(cache_test_t, test_addr_fixup) TestBlockPhysical::SIZE); extent->set_contents('c'); csum = extent->get_crc32c(); - auto ret = submit_transaction(std::move(t)).get0(); - ASSERT_TRUE(ret); + submit_transaction(std::move(t)).get0(); addr = extent->get_paddr(); } { @@ -165,8 +158,7 @@ TEST_F(cache_test_t, test_dirty_extent) ASSERT_EQ(extent->get_version(), 0); ASSERT_EQ(csum, extent->get_crc32c()); } - auto ret = submit_transaction(std::move(t)).get0(); - ASSERT_TRUE(ret); + submit_transaction(std::move(t)).get0(); addr = extent->get_paddr(); } { @@ -222,8 +214,7 @@ TEST_F(cache_test_t, test_dirty_extent) ASSERT_EQ(csum2, extent->get_crc32c()); } // submit transaction - auto ret = submit_transaction(std::move(t)).get0(); - ASSERT_TRUE(ret); + submit_transaction(std::move(t)).get0(); ASSERT_TRUE(extent->is_dirty()); ASSERT_EQ(addr, extent->get_paddr()); ASSERT_EQ(extent->get_version(), 1); -- 2.39.5