Merge pull request ceph#56844 from cyx1231st/wip-crimson-fix-tri-mutex

crimson/common/tri_mutex: make locking/promotion atomic if possible

Reviewed-by: Samuel Just <[email protected]>
Reviewed-by: Kefu Chai <[email protected]>

Author: cyx1231st

src/crimson/common/tri_mutex.cc

@@ -3,6 +3,8 @@
 
 #include "tri_mutex.h"
 
+#include <seastar/util/later.hh>
+
 seastar::future<> read_lock::lock()
 {
   return static_cast<tri_mutex*>(this)->lock_for_read();
@@ -15,7 +17,7 @@ void read_lock::unlock()
 
 seastar::future<> write_lock::lock()
 {
-  return static_cast<tri_mutex*>(this)->lock_for_write(false);
+  return static_cast<tri_mutex*>(this)->lock_for_write();
 }
 
 void write_lock::unlock()
@@ -33,37 +35,26 @@ void excl_lock::unlock()
   static_cast<tri_mutex*>(this)->unlock_for_excl();
 }
 
-seastar::future<> excl_lock_from_read::lock()
+void excl_lock_from_read::lock()
 {
   static_cast<tri_mutex*>(this)->promote_from_read();
-  return seastar::make_ready_future<>();
 }
 
 void excl_lock_from_read::unlock()
 {
   static_cast<tri_mutex*>(this)->demote_to_read();
 }
 
-seastar::future<> excl_lock_from_write::lock()
+void excl_lock_from_write::lock()
 {
   static_cast<tri_mutex*>(this)->promote_from_write();
-  return seastar::make_ready_future<>();
 }
 
 void excl_lock_from_write::unlock()
 {
   static_cast<tri_mutex*>(this)->demote_to_write();
 }
 
-seastar::future<> excl_lock_from_excl::lock()
-{
-  return seastar::make_ready_future<>();
-}
-
-void excl_lock_from_excl::unlock()
-{
-}
-
 tri_mutex::~tri_mutex()
 {
   assert(!is_acquired());
@@ -72,7 +63,7 @@ tri_mutex::~tri_mutex()
 seastar::future<> tri_mutex::lock_for_read()
 {
   if (try_lock_for_read()) {
-    return seastar::make_ready_future<>();
+    return seastar::now();
   }
   waiters.emplace_back(seastar::promise<>(), type_t::read);
   return waiters.back().pr.get_future();
@@ -110,19 +101,19 @@ void tri_mutex::demote_to_read()
   ++readers;
 }
 
-seastar::future<> tri_mutex::lock_for_write(bool greedy)
+seastar::future<> tri_mutex::lock_for_write()
 {
-  if (try_lock_for_write(greedy)) {
-    return seastar::make_ready_future<>();
+  if (try_lock_for_write()) {
+    return seastar::now();
   }
   waiters.emplace_back(seastar::promise<>(), type_t::write);
   return waiters.back().pr.get_future();
 }
 
-bool tri_mutex::try_lock_for_write(bool greedy) noexcept
+bool tri_mutex::try_lock_for_write() noexcept
 {
   if (!readers && !exclusively_used) {
-    if (greedy || waiters.empty()) {
+    if (waiters.empty()) {
       ++writers;
       return true;
     }
@@ -156,7 +147,7 @@ void tri_mutex::demote_to_write()
 seastar::future<> tri_mutex::lock_for_excl()
 {
   if (try_lock_for_excl()) {
-    return seastar::make_ready_future<>();
+    return seastar::now();
   }
   waiters.emplace_back(seastar::promise<>(), type_t::exclusive);
   return waiters.back().pr.get_future();

src/crimson/common/tri_mutex.h

@@ -27,43 +27,38 @@ class excl_lock {
 // promote from read to excl
 class excl_lock_from_read {
 public:
-  seastar::future<> lock();
+  void lock();
   void unlock();
 };
 
 // promote from write to excl
 class excl_lock_from_write {
 public:
-  seastar::future<> lock();
-  void unlock();
-};
-
-// promote from excl to excl
-class excl_lock_from_excl {
-public:
-  seastar::future<> lock();
+  void lock();
   void unlock();
 };
 
 /// shared/exclusive mutual exclusion
 ///
-/// this lock design uses reader and writer is entirely and completely
-/// independent of the conventional reader/writer lock usage. Here, what we
-/// mean is that we can pipeline reads, and we can pipeline writes, but we
-/// cannot allow a read while writes are in progress or a write while reads are
-/// in progress. Any rmw operation is therefore exclusive.
+/// Unlike reader/write lock, tri_mutex does not enforce the exclusive access
+/// of write operations, on the contrary, multiple write operations are allowed
+/// to hold the same tri_mutex at the same time. Here, what we mean is that we
+/// can pipeline reads, and we can pipeline writes, but we cannot allow a read
+/// while writes are in progress or a write while reads are in progress.
 ///
 /// tri_mutex is based on seastar::shared_mutex, but instead of two kinds of
 /// waiters, tri_mutex keeps track of three kinds of lock users:
 /// - readers
 /// - writers
 /// - exclusive users
+///
+/// For lock promotion, a read or a write lock is only allowed to be promoted
+/// atomically upon the first locking.
 class tri_mutex : private read_lock,
                           write_lock,
                           excl_lock,
                           excl_lock_from_read,
-                          excl_lock_from_write,
-                          excl_lock_from_excl
+                          excl_lock_from_write
 {
 public:
   tri_mutex() = default;
@@ -84,9 +79,6 @@ class tri_mutex : private read_lock,
   excl_lock_from_write& excl_from_write() {
     return *this;
   }
-  excl_lock_from_excl& excl_from_excl() {
-    return *this;
-  }
 
   // for shared readers
   seastar::future<> lock_for_read();
@@ -99,8 +91,8 @@ class tri_mutex : private read_lock,
   }
 
   // for shared writers
-  seastar::future<> lock_for_write(bool greedy);
-  bool try_lock_for_write(bool greedy) noexcept;
+  seastar::future<> lock_for_write();
+  bool try_lock_for_write() noexcept;
   void unlock_for_write();
   void promote_from_write();
   void demote_to_write();

src/crimson/osd/object_context.h

@@ -121,7 +121,7 @@ class ObjectContext : public ceph::common::intrusive_lru_base<
   bool recovery_read_marker = false;
 
   template <typename Lock, typename Func>
-  auto _with_lock(Lock&& lock, Func&& func) {
+  auto _with_lock(Lock& lock, Func&& func) {
     Ref obc = this;
     return lock.lock().then([&lock, func = std::forward<Func>(func), obc]() mutable {
       return seastar::futurize_invoke(func).finally([&lock, obc] {
@@ -130,6 +130,15 @@ class ObjectContext : public ceph::common::intrusive_lru_base<
     });
   }
 
+  template <typename Lock, typename Func>
+  auto _with_promoted_lock(Lock& lock, Func&& func) {
+    Ref obc = this;
+    lock.lock();
+    return seastar::futurize_invoke(func).finally([&lock, obc] {
+      lock.unlock();
+    });
+  }
+
   boost::intrusive::list_member_hook<> obc_accessing_hook;
   uint64_t list_link_cnt = 0;
   bool fully_loaded = false;
@@ -196,11 +205,11 @@ class ObjectContext : public ceph::common::intrusive_lru_base<
       auto wrapper = ::crimson::interruptible::interruptor<InterruptCond>::wrap_function(std::forward<Func>(func));
       switch (Type) {
       case RWState::RWWRITE:
-	return _with_lock(lock.excl_from_write(), std::move(wrapper));
+	return _with_promoted_lock(lock.excl_from_write(), std::move(wrapper));
       case RWState::RWREAD:
-	return _with_lock(lock.excl_from_read(), std::move(wrapper));
+	return _with_promoted_lock(lock.excl_from_read(), std::move(wrapper));
       case RWState::RWEXCL:
-	return _with_lock(lock.excl_from_excl(), std::move(wrapper));
+	return seastar::futurize_invoke(std::move(wrapper));
       case RWState::RWNONE:
 	return _with_lock(lock.for_excl(), std::move(wrapper));
        default:
@@ -209,11 +218,11 @@ class ObjectContext : public ceph::common::intrusive_lru_base<
     } else {
       switch (Type) {
       case RWState::RWWRITE:
-	return _with_lock(lock.excl_from_write(), std::forward<Func>(func));
+	return _with_promoted_lock(lock.excl_from_write(), std::forward<Func>(func));
       case RWState::RWREAD:
-	return _with_lock(lock.excl_from_read(), std::forward<Func>(func));
+	return _with_promoted_lock(lock.excl_from_read(), std::forward<Func>(func));
       case RWState::RWEXCL:
-	return _with_lock(lock.excl_from_excl(), std::forward<Func>(func));
+	return seastar::futurize_invoke(std::forward<Func>(func));
       case RWState::RWNONE:
 	return _with_lock(lock.for_excl(), std::forward<Func>(func));
        default: