crimson/common/tri_mutex: make lock() atomic if doesn't need wait

Otherwise, promotion cannot be atomic with the 1st locker.

Identified by: Matan Breizman <[email protected]>

Signed-off-by: Yingxin Cheng <[email protected]>

Author: cyx1231st

src/crimson/common/tri_mutex.cc

@@ -8,7 +8,8 @@
 SET_SUBSYS(osd);
 //TODO: SET_SUBSYS(crimson_tri_mutex);
 
-seastar::future<> read_lock::lock()
+std::optional<seastar::future<>>
+read_lock::lock()
 {
   return static_cast<tri_mutex*>(this)->lock_for_read();
 }
@@ -18,7 +19,8 @@ void read_lock::unlock()
   static_cast<tri_mutex*>(this)->unlock_for_read();
 }
 
-seastar::future<> write_lock::lock()
+std::optional<seastar::future<>>
+write_lock::lock()
 {
   return static_cast<tri_mutex*>(this)->lock_for_write();
 }
@@ -28,7 +30,8 @@ void write_lock::unlock()
   static_cast<tri_mutex*>(this)->unlock_for_write();
 }
 
-seastar::future<> excl_lock::lock()
+std::optional<seastar::future<>>
+excl_lock::lock()
 {
   return static_cast<tri_mutex*>(this)->lock_for_excl();
 }
@@ -65,13 +68,14 @@ tri_mutex::~tri_mutex()
   assert(!is_acquired());
 }
 
-seastar::future<> tri_mutex::lock_for_read()
+std::optional<seastar::future<>>
+tri_mutex::lock_for_read()
 {
   LOG_PREFIX(tri_mutex::lock_for_read());
   DEBUGDPP("", *this);
   if (try_lock_for_read()) {
     DEBUGDPP("lock_for_read successfully", *this);
-    return seastar::now();
+    return std::nullopt;
   }
   DEBUGDPP("can't lock_for_read, adding to waiters", *this);
   waiters.emplace_back(seastar::promise<>(), type_t::read, name);
@@ -117,13 +121,14 @@ void tri_mutex::demote_to_read()
   ++readers;
 }
 
-seastar::future<> tri_mutex::lock_for_write()
+std::optional<seastar::future<>>
+tri_mutex::lock_for_write()
 {
   LOG_PREFIX(tri_mutex::lock_for_write());
   DEBUGDPP("", *this);
   if (try_lock_for_write()) {
     DEBUGDPP("lock_for_write successfully", *this);
-    return seastar::now();
+    return std::nullopt;
   }
   DEBUGDPP("can't lock_for_write, adding to waiters", *this);
   waiters.emplace_back(seastar::promise<>(), type_t::write, name);
@@ -170,13 +175,14 @@ void tri_mutex::demote_to_write()
 }
 
 // for exclusive users
-seastar::future<> tri_mutex::lock_for_excl()
+std::optional<seastar::future<>>
+tri_mutex::lock_for_excl()
 {
   LOG_PREFIX(tri_mutex::lock_for_excl());
   DEBUGDPP("", *this);
   if (try_lock_for_excl()) {
     DEBUGDPP("lock_for_excl, successfully", *this);
-    return seastar::now();
+    return std::nullopt;
   }
   DEBUGDPP("can't lock_for_excl, adding to waiters", *this);
   waiters.emplace_back(seastar::promise<>(), type_t::exclusive, name);

src/crimson/common/tri_mutex.h

@@ -3,25 +3,27 @@
 
 #pragma once
 
+#include <optional>
+
 #include <seastar/core/future.hh>
 #include <seastar/core/circular_buffer.hh>
 #include "crimson/common/log.h"
 
 class read_lock {
 public:
-  seastar::future<> lock();
+  std::optional<seastar::future<>> lock();
   void unlock();
 };
 
 class write_lock {
 public:
-  seastar::future<> lock();
+  std::optional<seastar::future<>> lock();
   void unlock();
 };
 
 class excl_lock {
 public:
-  seastar::future<> lock();
+  std::optional<seastar::future<>> lock();
   void unlock();
 };
 
@@ -87,7 +89,7 @@ class tri_mutex : private read_lock,
   }
 
   // for shared readers
-  seastar::future<> lock_for_read();
+  std::optional<seastar::future<>> lock_for_read();
   bool try_lock_for_read() noexcept;
   void unlock_for_read();
   void promote_from_read();
@@ -97,7 +99,7 @@ class tri_mutex : private read_lock,
   }
 
   // for shared writers
-  seastar::future<> lock_for_write();
+  std::optional<seastar::future<>> lock_for_write();
   bool try_lock_for_write() noexcept;
   void unlock_for_write();
   void promote_from_write();
@@ -107,7 +109,7 @@ class tri_mutex : private read_lock,
   }
 
   // for exclusive users
-  seastar::future<> lock_for_excl();
+  std::optional<seastar::future<>> lock_for_excl();
   bool try_lock_for_excl() noexcept;
   void unlock_for_excl();
   bool is_excl_acquired() const {

src/crimson/osd/object_context.h

@@ -138,10 +138,21 @@ class ObjectContext : public ceph::common::intrusive_lru_base<
   template <typename Lock, typename 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] {
-	lock.unlock();
-      });
+    auto maybe_fut = lock.lock();
+    return seastar::futurize_invoke([
+        maybe_fut=std::move(maybe_fut),
+        func=std::forward<Func>(func)]() mutable {
+      if (maybe_fut) {
+        return std::move(*maybe_fut
+        ).then([func=std::forward<Func>(func)]() mutable {
+          return seastar::futurize_invoke(func);
+        });
+      } else {
+        // atomically calling func upon locking
+        return seastar::futurize_invoke(func);
+      }
+    }).finally([&lock, obc] {
+      lock.unlock();
     });
   }