From 1269aac1f3076954864e4cce31eafabde2248a06 Mon Sep 17 00:00:00 2001
From: Yingxin Cheng <yingxin.cheng@intel.com>
Date: Mon, 15 Apr 2024 10:03:35 +0800
Subject: [PATCH] crimson/common/tri_mutex: make promotion atomic with func

Specifically, make promotion atomic with load-obc to fix
assert(readers/writers == 1) failures.

Fixes: https://tracker.ceph.com/issues/65451
Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>
(cherry picked from commit 676947e1f73a9d7f900df5a6854c283c4eef24a9)
---
 src/crimson/common/tri_mutex.cc  | 15 ++-------------
 src/crimson/common/tri_mutex.h   | 20 ++++++--------------
 src/crimson/osd/object_context.h | 23 ++++++++++++++++-------
 3 files changed, 24 insertions(+), 34 deletions(-)

diff --git a/src/crimson/common/tri_mutex.cc b/src/crimson/common/tri_mutex.cc
index 172406fd81553..f79d256688543 100644
--- a/src/crimson/common/tri_mutex.cc
+++ b/src/crimson/common/tri_mutex.cc
@@ -35,10 +35,9 @@ 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::now();
 }
 
 void excl_lock_from_read::unlock()
@@ -46,10 +45,9 @@ 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::now();
 }
 
 void excl_lock_from_write::unlock()
@@ -57,15 +55,6 @@ void excl_lock_from_write::unlock()
   static_cast<tri_mutex*>(this)->demote_to_write();
 }
 
-seastar::future<> excl_lock_from_excl::lock()
-{
-  return seastar::now();
-}
-
-void excl_lock_from_excl::unlock()
-{
-}
-
 tri_mutex::~tri_mutex()
 {
   assert(!is_acquired());
diff --git a/src/crimson/common/tri_mutex.h b/src/crimson/common/tri_mutex.h
index a75895fa8decc..d1c215be27e20 100644
--- a/src/crimson/common/tri_mutex.h
+++ b/src/crimson/common/tri_mutex.h
@@ -27,21 +27,14 @@ public:
 // 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();
 };
 
@@ -58,12 +51,14 @@ public:
 /// - 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 @@ public:
   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();
diff --git a/src/crimson/osd/object_context.h b/src/crimson/osd/object_context.h
index f38e52f009ce4..dc10223de1e60 100644
--- a/src/crimson/osd/object_context.h
+++ b/src/crimson/osd/object_context.h
@@ -121,7 +121,7 @@ private:
   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 @@ private:
     });
   }
 
+  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 @@ public:
       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 @@ public:
     } 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:
-- 
2.39.5