Merge pull request #10945 from dillaman/wip-17188

librbd: deadlock when replaying journal during image open

Reviewed-by: Mykola Golub <mgolub@mirantis.com>

src/librbd/ExclusiveLock.cc

@@ -416,12 +416,8 @@ void ExclusiveLock<I>::send_acquire_lock() {
     m_image_ctx, m_cookie,
     util::create_context_callback<el, &el::handle_acquiring_lock>(this),
     util::create_context_callback<el, &el::handle_acquire_lock>(this));
-
-  // acquire the lock if the image is not busy performing other actions
-  m_image_ctx.state->prepare_lock(new FunctionContext([this, req](int r) {
-      m_image_ctx.op_work_queue->queue(
-        new C_SendRequest<AcquireRequest<I> >(req), 0);
-    }));
+  m_image_ctx.op_work_queue->queue(new C_SendRequest<AcquireRequest<I> >(req),
+                                   0);
 }
 
 template <typename I>
@@ -450,7 +446,6 @@ void ExclusiveLock<I>::handle_acquire_lock(int r) {
     ldout(cct, 5) << "successfully acquired exclusive lock" << dendl;
   }
 
-  m_image_ctx.state->handle_prepare_lock_complete();
   {
     m_lock.Lock();
     assert(m_state == STATE_ACQUIRING ||
@@ -587,13 +582,10 @@ void ExclusiveLock<I>::send_release_lock() {
   ReleaseRequest<I>* req = ReleaseRequest<I>::create(
     m_image_ctx, m_cookie,
     util::create_context_callback<el, &el::handle_releasing_lock>(this),
-    util::create_context_callback<el, &el::handle_release_lock>(this));
-
-  // release the lock if the image is not busy performing other actions
-  m_image_ctx.state->prepare_lock(new FunctionContext([this, req](int r) {
-      m_image_ctx.op_work_queue->queue(
-        new C_SendRequest<ReleaseRequest<I> >(req), 0);
-    }));
+    util::create_context_callback<el, &el::handle_release_lock>(this),
+    false);
+  m_image_ctx.op_work_queue->queue(new C_SendRequest<ReleaseRequest<I> >(req),
+                                   0);
 }
 
 template <typename I>
@@ -610,8 +602,6 @@ void ExclusiveLock<I>::handle_releasing_lock(int r) {
 
 template <typename I>
 void ExclusiveLock<I>::handle_release_lock(int r) {
-  m_image_ctx.state->handle_prepare_lock_complete();
-
   bool lock_request_needed = false;
   {
     Mutex::Locker locker(m_lock);
@@ -670,7 +660,8 @@ void ExclusiveLock<I>::send_shutdown_release() {
   ReleaseRequest<I>* req = ReleaseRequest<I>::create(
     m_image_ctx, cookie,
     util::create_context_callback<el, &el::handle_shutdown_releasing>(this),
-    util::create_context_callback<el, &el::handle_shutdown_released>(this));
+    util::create_context_callback<el, &el::handle_shutdown_released>(this),
+    true);
   req->send();
 }
 

src/librbd/ImageState.cc

@@ -16,7 +16,7 @@
 
 #define dout_subsys ceph_subsys_rbd
 #undef dout_prefix
-#define dout_prefix *_dout << "librbd::ImageState: "
+#define dout_prefix *_dout << "librbd::ImageState: " << this << " "
 
 namespace librbd {
 

src/librbd/exclusive_lock/AcquireRequest.cc

@@ -72,12 +72,35 @@ AcquireRequest<I>::AcquireRequest(I &image_ctx, const std::string &cookie,
 
 template <typename I>
 AcquireRequest<I>::~AcquireRequest() {
+  if (!m_prepare_lock_completed) {
+    m_image_ctx.state->handle_prepare_lock_complete();
+  }
   delete m_on_acquire;
 }
 
 template <typename I>
 void AcquireRequest<I>::send() {
+  send_prepare_lock();
+}
+
+template <typename I>
+void AcquireRequest<I>::send_prepare_lock() {
+  CephContext *cct = m_image_ctx.cct;
+  ldout(cct, 10) << __func__ << dendl;
+
+  // acquire the lock if the image is not busy performing other actions
+  Context *ctx = create_context_callback<
+    AcquireRequest<I>, &AcquireRequest<I>::handle_prepare_lock>(this);
+  m_image_ctx.state->prepare_lock(ctx);
+}
+
+template <typename I>
+Context *AcquireRequest<I>::handle_prepare_lock(int *ret_val) {
+  CephContext *cct = m_image_ctx.cct;
+  ldout(cct, 10) << __func__ << ": r=" << *ret_val << dendl;
+
   send_flush_notifies();
+  return nullptr;
 }
 
 template <typename I>
@@ -563,12 +586,17 @@ Context *AcquireRequest<I>::handle_break_lock(int *ret_val) {
 
 template <typename I>
 void AcquireRequest<I>::apply() {
-  RWLock::WLocker snap_locker(m_image_ctx.snap_lock);
-  assert(m_image_ctx.object_map == nullptr);
-  m_image_ctx.object_map = m_object_map;
+  {
+    RWLock::WLocker snap_locker(m_image_ctx.snap_lock);
+    assert(m_image_ctx.object_map == nullptr);
+    m_image_ctx.object_map = m_object_map;
+
+    assert(m_image_ctx.journal == nullptr);
+    m_image_ctx.journal = m_journal;
+  }
 
-  assert(m_image_ctx.journal == nullptr);
-  m_image_ctx.journal = m_journal;
+  m_prepare_lock_completed = true;
+  m_image_ctx.state->handle_prepare_lock_complete();
 }
 
 template <typename I>

src/librbd/exclusive_lock/AcquireRequest.h

@@ -35,6 +35,9 @@ class AcquireRequest {
    * <start>
    *    |
    *    v
+   * PREPARE_LOCK
+   *    |
+   *    v
    * FLUSH_NOTIFIES
    *    |
    *    |     /-----------------------------------------------------------\
@@ -97,6 +100,10 @@ class AcquireRequest {
   uint64_t m_locker_handle;
 
   int m_error_result;
+  bool m_prepare_lock_completed = false;
+
+  void send_prepare_lock();
+  Context *handle_prepare_lock(int *ret_val);
 
   void send_flush_notifies();
   Context *handle_flush_notifies(int *ret_val);

src/librbd/exclusive_lock/ReleaseRequest.cc

@@ -8,6 +8,7 @@
 #include "common/errno.h"
 #include "librbd/AioImageRequestWQ.h"
 #include "librbd/ExclusiveLock.h"
+#include "librbd/ImageState.h"
 #include "librbd/ImageWatcher.h"
 #include "librbd/Journal.h"
 #include "librbd/ObjectMap.h"
@@ -28,26 +29,57 @@ template <typename I>
 ReleaseRequest<I>* ReleaseRequest<I>::create(I &image_ctx,
                                              const std::string &cookie,
                                              Context *on_releasing,
-                                             Context *on_finish) {
-  return new ReleaseRequest(image_ctx, cookie, on_releasing, on_finish);
+                                             Context *on_finish,
+                                             bool shutting_down) {
+  return new ReleaseRequest(image_ctx, cookie, on_releasing, on_finish,
+                            shutting_down);
 }
 
 template <typename I>
 ReleaseRequest<I>::ReleaseRequest(I &image_ctx, const std::string &cookie,
-                                  Context *on_releasing, Context *on_finish)
+                                  Context *on_releasing, Context *on_finish,
+                                  bool shutting_down)
   : m_image_ctx(image_ctx), m_cookie(cookie), m_on_releasing(on_releasing),
     m_on_finish(create_async_context_callback(image_ctx, on_finish)),
-    m_object_map(nullptr), m_journal(nullptr) {
+    m_shutting_down(shutting_down), m_object_map(nullptr), m_journal(nullptr) {
 }
 
 template <typename I>
 ReleaseRequest<I>::~ReleaseRequest() {
+  if (!m_shutting_down) {
+    m_image_ctx.state->handle_prepare_lock_complete();
+  }
   delete m_on_releasing;
 }
 
 template <typename I>
 void ReleaseRequest<I>::send() {
+  send_prepare_lock();
+}
+
+template <typename I>
+void ReleaseRequest<I>::send_prepare_lock() {
+  if (m_shutting_down) {
+    send_cancel_op_requests();
+    return;
+  }
+
+  CephContext *cct = m_image_ctx.cct;
+  ldout(cct, 10) << __func__ << dendl;
+
+  // release the lock if the image is not busy performing other actions
+  Context *ctx = create_context_callback<
+    ReleaseRequest<I>, &ReleaseRequest<I>::handle_prepare_lock>(this);
+  m_image_ctx.state->prepare_lock(ctx);
+}
+
+template <typename I>
+Context *ReleaseRequest<I>::handle_prepare_lock(int *ret_val) {
+  CephContext *cct = m_image_ctx.cct;
+  ldout(cct, 10) << __func__ << ": r=" << *ret_val << dendl;
+
   send_cancel_op_requests();
+  return nullptr;
 }
 
 template <typename I>

src/librbd/exclusive_lock/ReleaseRequest.h

@@ -20,7 +20,8 @@ template <typename ImageCtxT = ImageCtx>
 class ReleaseRequest {
 public:
   static ReleaseRequest* create(ImageCtxT &image_ctx, const std::string &cookie,
-                                Context *on_releasing, Context *on_finish);
+                                Context *on_releasing, Context *on_finish,
+                                bool shutting_down);
 
   ~ReleaseRequest();
   void send();
@@ -32,6 +33,9 @@ class ReleaseRequest {
    * <start>
    *    |
    *    v
+   * PREPARE_LOCK
+   *    |
+   *    v
    * CANCEL_OP_REQUESTS
    *    |
    *    v
@@ -56,16 +60,21 @@ class ReleaseRequest {
    */
 
   ReleaseRequest(ImageCtxT &image_ctx, const std::string &cookie,
-                 Context *on_releasing, Context *on_finish);
+                 Context *on_releasing, Context *on_finish,
+                 bool shutting_down);
 
   ImageCtxT &m_image_ctx;
   std::string m_cookie;
   Context *m_on_releasing;
   Context *m_on_finish;
+  bool m_shutting_down;
 
   decltype(m_image_ctx.object_map) m_object_map;
   decltype(m_image_ctx.journal) m_journal;
 
+  void send_prepare_lock();
+  Context *handle_prepare_lock(int *ret_val);
+
   void send_cancel_op_requests();
   Context *handle_cancel_op_requests(int *ret_val);