crimson/os/seastore/transaction: new conflict/no_conflict trans

synchronization

Use seastar::shared_mutex to synchronize transactions when committing
no_conflict ones.

Specifically, this commit achieves conflict/no_conflict trans
synchronization by:
1. The no_conflict trans acquires locks on all the stable extents which
   it modifies.
2. The conflicting trans acquires shared locks on all the stable extents
   which it modifies.
3. The no_conflict trans releases the locks after its committing.
4. The conflicting trans releases the shared locks once it enter the
   prepare pipeline phase.

This commits makes sure that:
1. only transactions accessing the extents that the no_conflict trans
   is committing will be blocked.
2. avoid persisting extents that a committing no_conflict trans is
   modified

Compared to the old approach, this commit's strategy is much more
straightforward and understandable.

This commit is more performant as the no_conflict trans only blocks
the trans the modified extent set of which intersects with it, while,
in the old approach, all other transactions will be blocked once a
conflicting trans is blocked because it's blocked after it enters the
prepare pipeline phase

Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>

diff --git a/src/crimson/os/seastore/cached_extent.h b/src/crimson/os/seastore/cached_extent.h
index e0325c6bf6083fe9e4bbac3f9c21c4a7360ec36b..91416f870604391844777cb3ebb1dd1884060980 100644 (file)
--- a/src/crimson/os/seastore/cached_extent.h
+++ b/src/crimson/os/seastore/cached_extent.h
@@ -1012,6 +1012,8 @@ private:
 
   void new_committer(Transaction &t);
 
+  seastar::shared_mutex commit_lock;
+
 protected:
   trans_view_set_t mutation_pending_extents;
   trans_view_set_t retired_transactions;

diff --git a/src/crimson/os/seastore/transaction.h b/src/crimson/os/seastore/transaction.h
index 99caa62dd80fb25610df64d44e53655d6012995a..c7ff9071af94aaba70c88e9aa07ee104abcdb593 100644 (file)
--- a/src/crimson/os/seastore/transaction.h
+++ b/src/crimson/os/seastore/transaction.h
@@ -444,6 +444,26 @@ public:
     std::for_each(existing_block_list.begin(), existing_block_list.end(), f);
   }
 
+  template <typename F>
+  void for_each_mutated_extent(F &&f) {
+    std::for_each(
+      retired_set.begin(),
+      retired_set.end(),
+      [&f](auto &link) {
+        std::invoke(f, *link.extent);
+      });
+    std::for_each(
+      mutated_block_list.begin(),
+      mutated_block_list.end(),
+      [&f](auto &e) {
+        if (!e->is_valid() ||
+            e->is_exist_mutation_pending()) {
+          return;
+        }
+        std::invoke(f, *e->get_prior_instance());
+      });
+  }
+
   const io_stat_t& get_fresh_block_stats() const {
     return fresh_block_stats;
   }
@@ -972,7 +992,6 @@ constexpr bool should_use_no_conflict_publish(const Transaction &t,
   return !t.force_rewrite_conflict && is_rewrite_transaction(t.get_src());
 }
 
-
 }
 
 #if FMT_VERSION >= 90000

diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc
index ae2b32cfd0dae6147b5c1812dff97e78f6cbe93c..a8d28e276f3ce06b9e9a70bfc1222fca13d7256b 100644 (file)
--- a/src/crimson/os/seastore/transaction_manager.cc
+++ b/src/crimson/os/seastore/transaction_manager.cc
@@ -593,6 +593,43 @@ TransactionManager::update_lba_mappings(
   });
 }
 
+seastar::future<TransactionManager::mutated_extents_locker>
+TransactionManager::lock_mutated_nodes(
+  Transaction &t)
+{
+  mutated_extents_set_t mutated_extents;
+  LOG_PREFIX(TransactionManager::lock_mutated_nodes);
+  t.for_each_mutated_extent(
+    [&mutated_extents](auto &extent) {
+      if (!is_root_type(extent.get_type())) {
+        std::ignore = mutated_extents.emplace(&extent);
+      }
+  });
+  if (unlikely(mutated_extents.empty())) {
+    co_return mutated_extents_locker();
+  }
+  std::vector<std::unique_lock<seastar::shared_mutex>> unique_locks;
+  std::vector<std::shared_lock<seastar::shared_mutex>> shared_locks;
+  if (auto &e = *mutated_extents.begin();
+      should_use_no_conflict_publish(t, e->get_type())) {
+    for (auto &ext : mutated_extents) {
+      assert(should_use_no_conflict_publish(t, ext->get_type()));
+      TRACET("locking {}", t, *ext);
+      auto lock = co_await seastar::get_unique_lock(ext->commit_lock);
+      unique_locks.emplace_back(std::move(lock));
+    }
+  } else {
+    for (auto &ext : mutated_extents) {
+      assert(!should_use_no_conflict_publish(t, ext->get_type()));
+      TRACET("shared locking {}", t, *ext);
+      auto lock = co_await seastar::get_shared_lock(ext->commit_lock);
+      shared_locks.emplace_back(std::move(lock));
+    }
+  }
+  co_return mutated_extents_locker(
+    std::move(mutated_extents), std::move(unique_locks), std::move(shared_locks));
+}
+
 TransactionManager::submit_transaction_direct_ret
 TransactionManager::do_submit_transaction(
   Transaction &tref,
@@ -619,6 +656,18 @@ TransactionManager::do_submit_transaction(
   tref.get_phase_durations().lba_update +=
     std::chrono::steady_clock::now() - lba_start;
 
+  // TODO: For now, we lock mutated extents after delayed ool writes
+  // and lba mappings updating, this is ok because:
+  // 1. at present, only lba/backref nodes might be modified by
+  //    no_conflict trans;
+  // 2. only ool lba/backref extents' persistence needs to be sync'd
+  //
+  // In the future, when no_conflict transactions may also modify
+  // logical extents, we should add something like "lock_logical_mutated_extents"
+  // and invoke it before writing ool extents.
+  auto locker = co_await trans_intr::make_interruptible(
+    lock_mutated_nodes(tref));
+
   auto num_extents = allocated_extents.size();
   SUBTRACET(seastore_t, "process {} allocated extents", tref, num_extents);
   ool_start = std::chrono::steady_clock::now();
@@ -631,6 +680,13 @@ TransactionManager::do_submit_transaction(
   co_await trans_intr::make_interruptible(
     tref.get_handle().enter(write_pipeline.prepare)
   );
+
+  // For conflicting transactions, we can release the lock
+  // now. Because other transactions accessing the same
+  // extents as the current one would be invalidated later
+  // in Cache::prepare_record()
+  locker.release_shared_lock();
+
   tref.get_phase_durations().prepare_enter +=
     std::chrono::steady_clock::now() - prepare_enter_start;
 
@@ -659,7 +715,7 @@ TransactionManager::do_submit_transaction(
     std::move(record),
     tref.get_handle(),
     tref.get_src(),
-    [this, FNAME, &tref](record_locator_t submit_result) {
+    [&locker, this, FNAME, &tref](record_locator_t submit_result) {
     SUBDEBUGT(seastore_t, "committed with {}", tref, submit_result);
     auto start_seq = submit_result.write_result.start_seq;
     journal->get_trimmer().set_journal_head(start_seq);
@@ -667,6 +723,7 @@ TransactionManager::do_submit_transaction(
       tref,
       submit_result.record_block_base,
       start_seq);
+    locker.release_lock();
     journal->get_trimmer().update_journal_tails(
       cache->get_oldest_dirty_from().value_or(start_seq),
       cache->get_oldest_backref_dirty_from().value_or(start_seq));

diff --git a/src/crimson/os/seastore/transaction_manager.h b/src/crimson/os/seastore/transaction_manager.h
index 3e22411f08014aef73ee9a4a0d04a937b1655602..a08734f0d215334b9c52feb1f96cdb3e5ff0def3 100644 (file)
--- a/src/crimson/os/seastore/transaction_manager.h
+++ b/src/crimson/os/seastore/transaction_manager.h
@@ -1303,6 +1303,52 @@ private:
     return cache->can_drop_backref();
   }
 
+  // use memory addresses as the key for comparing extents,
+  // this makes sure that mutexes of extents are always
+  // acquired in the same order, avoiding dead locks.
+  struct mem_addr_cmp_t {
+    bool operator()(const CachedExtentRef &lhs,
+                    const CachedExtentRef &rhs) const {
+      return lhs.get() < rhs.get();
+    }
+  };
+
+  using mutated_extents_set_t =
+    std::set<CachedExtentRef, mem_addr_cmp_t>;
+  struct mutated_extents_locker {
+    mutated_extents_set_t mutated_extents;
+    std::vector<std::unique_lock<seastar::shared_mutex>> unique_locks;
+    std::vector<std::shared_lock<seastar::shared_mutex>> shared_locks;
+
+    mutated_extents_locker(
+      mutated_extents_set_t &&mutated_extents,
+      std::vector<std::unique_lock<seastar::shared_mutex>> &&unique_locks,
+      std::vector<std::shared_lock<seastar::shared_mutex>> &&shared_locks)
+      : mutated_extents(mutated_extents),
+        unique_locks(std::move(unique_locks)),
+        shared_locks(std::move(shared_locks)) {}
+    mutated_extents_locker() = default;
+    mutated_extents_locker(mutated_extents_locker&&) = default;
+
+    void release_lock() {
+      unique_locks.clear();
+    }
+
+    void release_shared_lock() {
+      shared_locks.clear();
+    }
+
+    void release() {
+      unique_locks.clear();
+      shared_locks.clear();
+    }
+    ~mutated_extents_locker() {
+      release();
+    }
+  };
+  seastar::future<mutated_extents_locker> lock_mutated_nodes(
+    Transaction &t);
+
   using resolve_cursor_to_mapping_iertr = base_iertr;
   resolve_cursor_to_mapping_iertr::future<LBAMapping>
   resolve_cursor_to_mapping(