librbd: avoid blocking writes when initializing exclusive-lock

The exclusive-lock dispatch layer will already block IOs as required
so this second layer of blocking just increases the complexity and
the potential for deadlocks when attempting to flush.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>

diff --git a/src/librbd/ExclusiveLock.cc b/src/librbd/ExclusiveLock.cc
index 92ddc665b7f69462bb365fb92af66b9a32197fd6..76945d8472e60ac4ab23485e3a09514b5fe21379 100644 (file)
--- a/src/librbd/ExclusiveLock.cc
+++ b/src/librbd/ExclusiveLock.cc
@@ -125,17 +125,33 @@ void ExclusiveLock<I>::init(uint64_t features, Context *on_init) {
 
   on_init = create_context_callback<Context>(on_init, this);
 
-  ldout(m_image_ctx.cct, 10) << dendl;
+  ldout(m_image_ctx.cct, 10) << ": features=" << features << dendl;
 
   {
     std::lock_guard locker{ML<I>::m_lock};
     ML<I>::set_state_initializing();
   }
 
-  auto ctx = new LambdaContext([this, features, on_init](int r) {
-      handle_init_complete(r, features, on_init);
+  m_image_dispatch = exclusive_lock::ImageDispatch<I>::create(&m_image_ctx);
+  m_image_ctx.io_image_dispatcher->register_dispatch(m_image_dispatch);
+
+  on_init = new LambdaContext([this, on_init](int r) {
+      {
+        std::lock_guard locker{ML<I>::m_lock};
+        ML<I>::set_state_unlocked();
+      }
+
+      on_init->complete(r);
     });
-  m_image_ctx.io_image_dispatcher->block_writes(ctx);
+
+  bool pwl_enabled = cache::util::is_pwl_enabled(m_image_ctx);
+  if (m_image_ctx.clone_copy_on_read ||
+      (features & RBD_FEATURE_JOURNALING) != 0 ||
+      pwl_enabled) {
+    m_image_dispatch->set_require_lock(true, io::DIRECTION_BOTH, on_init);
+  } else {
+    m_image_dispatch->set_require_lock(true, io::DIRECTION_WRITE, on_init);
+  }
 }
 
 template <typename I>
@@ -181,41 +197,6 @@ Context *ExclusiveLock<I>::start_op(int* ret_val) {
     });
 }
 
-template <typename I>
-void ExclusiveLock<I>::handle_init_complete(int r, uint64_t features,
-                                            Context* on_finish) {
-  if (r < 0) {
-    m_image_ctx.io_image_dispatcher->unblock_writes();
-    on_finish->complete(r);
-    return;
-  }
-
-  ldout(m_image_ctx.cct, 10) << ": features=" << features << dendl;
-
-  m_image_dispatch = exclusive_lock::ImageDispatch<I>::create(&m_image_ctx);
-  m_image_ctx.io_image_dispatcher->register_dispatch(m_image_dispatch);
-
-  on_finish = new LambdaContext([this, on_finish](int r) {
-      m_image_ctx.io_image_dispatcher->unblock_writes();
-
-      {
-        std::lock_guard locker{ML<I>::m_lock};
-        ML<I>::set_state_unlocked();
-      }
-
-      on_finish->complete(r);
-    });
-
-  bool rwl_enabled = cache::util::is_pwl_enabled(m_image_ctx);
-  if (m_image_ctx.clone_copy_on_read ||
-      (features & RBD_FEATURE_JOURNALING) != 0 ||
-      rwl_enabled) {
-    m_image_dispatch->set_require_lock(true, io::DIRECTION_BOTH, on_finish);
-  } else {
-    m_image_dispatch->set_require_lock(true, io::DIRECTION_WRITE, on_finish);
-  }
-}
-
 template <typename I>
 void ExclusiveLock<I>::shutdown_handler(int r, Context *on_finish) {
   ldout(m_image_ctx.cct, 10) << dendl;

diff --git a/src/librbd/ExclusiveLock.h b/src/librbd/ExclusiveLock.h
index 4a3215ce98c361ff187b7c73cdb1d0b2f999505e..9915262f9d97dd4c57b9d8fb75c34ed058d40320 100644 (file)
--- a/src/librbd/ExclusiveLock.h
+++ b/src/librbd/ExclusiveLock.h
@@ -108,7 +108,6 @@ private:
 
   bool accept_ops(const ceph::mutex &lock) const;
 
-  void handle_init_complete(int r, uint64_t features, Context* on_finish);
   void handle_post_acquiring_lock(int r);
   void handle_post_acquired_lock(int r);
 };

diff --git a/src/test/librbd/test_mock_ExclusiveLock.cc b/src/test/librbd/test_mock_ExclusiveLock.cc
index 33f7d975cd39d9a20ce45fdd50a841973636666e..8d1f35c3b6bf1538cd3d6f494a00711feeb3dae5 100644 (file)
--- a/src/test/librbd/test_mock_ExclusiveLock.cc
+++ b/src/test/librbd/test_mock_ExclusiveLock.cc
@@ -278,16 +278,6 @@ public:
     EXPECT_CALL(mock_image_dispatch, unset_require_lock(io::DIRECTION_BOTH));
   }
 
-  void expect_block_writes(MockExclusiveLockImageCtx &mock_image_ctx,
-                           MockImageDispatch& mock_image_dispatch) {
-    EXPECT_CALL(*mock_image_ctx.io_image_dispatcher, block_writes(_))
-                  .WillOnce(CompleteContext(0, mock_image_ctx.image_ctx->op_work_queue));
-  }
-
-  void expect_unblock_writes(MockExclusiveLockImageCtx &mock_image_ctx) {
-    EXPECT_CALL(*mock_image_ctx.io_image_dispatcher, unblock_writes());
-  }
-
   void expect_register_dispatch(MockExclusiveLockImageCtx &mock_image_ctx) {
     EXPECT_CALL(*mock_image_ctx.io_image_dispatcher, register_dispatch(_));
   }
@@ -435,10 +425,8 @@ TEST_F(TestMockExclusiveLock, StateTransitions) {
   InSequence seq;
   expect_set_state_initializing(exclusive_lock);
   MockImageDispatch mock_image_dispatch;
-  expect_block_writes(mock_image_ctx, mock_image_dispatch);
   expect_register_dispatch(mock_image_ctx);
   expect_set_require_lock(mock_image_ctx, mock_image_dispatch, true);
-  expect_unblock_writes(mock_image_ctx);
   expect_set_state_unlocked(exclusive_lock);
   ASSERT_EQ(0, when_init(mock_image_ctx, exclusive_lock));
 
@@ -506,10 +494,8 @@ TEST_F(TestMockExclusiveLock, TryLockAlreadyLocked) {
 
   expect_set_state_initializing(exclusive_lock);
   MockImageDispatch mock_image_dispatch;
-  expect_block_writes(mock_image_ctx, mock_image_dispatch);
   expect_register_dispatch(mock_image_ctx);
   expect_set_require_lock(mock_image_ctx, mock_image_dispatch, true);
-  expect_unblock_writes(mock_image_ctx);
   expect_set_state_unlocked(exclusive_lock);
   ASSERT_EQ(0, when_init(mock_image_ctx, exclusive_lock));
 
@@ -541,10 +527,8 @@ TEST_F(TestMockExclusiveLock, TryLockError) {
   InSequence seq;
   expect_set_state_initializing(exclusive_lock);
   MockImageDispatch mock_image_dispatch;
-  expect_block_writes(mock_image_ctx, mock_image_dispatch);
   expect_register_dispatch(mock_image_ctx);
   expect_set_require_lock(mock_image_ctx, mock_image_dispatch, true);
-  expect_unblock_writes(mock_image_ctx);
   expect_set_state_unlocked(exclusive_lock);
   ASSERT_EQ(0, when_init(mock_image_ctx, exclusive_lock));
 
@@ -576,10 +560,8 @@ TEST_F(TestMockExclusiveLock, AcquireLockAlreadyLocked) {
   InSequence seq;
   expect_set_state_initializing(exclusive_lock);
   MockImageDispatch mock_image_dispatch;
-  expect_block_writes(mock_image_ctx, mock_image_dispatch);
   expect_register_dispatch(mock_image_ctx);
   expect_set_require_lock(mock_image_ctx, mock_image_dispatch, true);
-  expect_unblock_writes(mock_image_ctx);
   expect_set_state_unlocked(exclusive_lock);
   ASSERT_EQ(0, when_init(mock_image_ctx, exclusive_lock));
 
@@ -614,10 +596,8 @@ TEST_F(TestMockExclusiveLock, AcquireLockBusy) {
   InSequence seq;
   expect_set_state_initializing(exclusive_lock);
   MockImageDispatch mock_image_dispatch;
-  expect_block_writes(mock_image_ctx, mock_image_dispatch);
   expect_register_dispatch(mock_image_ctx);
   expect_set_require_lock(mock_image_ctx, mock_image_dispatch, true);
-  expect_unblock_writes(mock_image_ctx);
   expect_set_state_unlocked(exclusive_lock);
   ASSERT_EQ(0, when_init(mock_image_ctx, exclusive_lock));
 
@@ -652,10 +632,8 @@ TEST_F(TestMockExclusiveLock, AcquireLockError) {
   InSequence seq;
   expect_set_state_initializing(exclusive_lock);
   MockImageDispatch mock_image_dispatch;
-  expect_block_writes(mock_image_ctx, mock_image_dispatch);
   expect_register_dispatch(mock_image_ctx);
   expect_set_require_lock(mock_image_ctx, mock_image_dispatch, true);
-  expect_unblock_writes(mock_image_ctx);
   expect_set_state_unlocked(exclusive_lock);
   ASSERT_EQ(0, when_init(mock_image_ctx, exclusive_lock));
 
@@ -688,10 +666,8 @@ TEST_F(TestMockExclusiveLock, PostAcquireLockError) {
   InSequence seq;
   expect_set_state_initializing(exclusive_lock);
   MockImageDispatch mock_image_dispatch;
-  expect_block_writes(mock_image_ctx, mock_image_dispatch);
   expect_register_dispatch(mock_image_ctx);
   expect_set_require_lock(mock_image_ctx, mock_image_dispatch, true);
-  expect_unblock_writes(mock_image_ctx);
   expect_set_state_unlocked(exclusive_lock);
   ASSERT_EQ(0, when_init(mock_image_ctx, exclusive_lock));
 
@@ -724,10 +700,8 @@ TEST_F(TestMockExclusiveLock, PreReleaseLockError) {
   InSequence seq;
   expect_set_state_initializing(exclusive_lock);
   MockImageDispatch mock_image_dispatch;
-  expect_block_writes(mock_image_ctx, mock_image_dispatch);
   expect_register_dispatch(mock_image_ctx);
   expect_set_require_lock(mock_image_ctx, mock_image_dispatch, true);
-  expect_unblock_writes(mock_image_ctx);
   expect_set_state_unlocked(exclusive_lock);
   ASSERT_EQ(0, when_init(mock_image_ctx, exclusive_lock));
 
@@ -758,10 +732,8 @@ TEST_F(TestMockExclusiveLock, ReacquireLock) {
   InSequence seq;
   expect_set_state_initializing(exclusive_lock);
   MockImageDispatch mock_image_dispatch;
-  expect_block_writes(mock_image_ctx, mock_image_dispatch);
   expect_register_dispatch(mock_image_ctx);
   expect_set_require_lock(mock_image_ctx, mock_image_dispatch, true);
-  expect_unblock_writes(mock_image_ctx);
   expect_set_state_unlocked(exclusive_lock);
   ASSERT_EQ(0, when_init(mock_image_ctx, exclusive_lock));
 
@@ -815,10 +787,8 @@ TEST_F(TestMockExclusiveLock, BlockRequests) {
   InSequence seq;
   expect_set_state_initializing(exclusive_lock);
   MockImageDispatch mock_image_dispatch;
-  expect_block_writes(mock_image_ctx, mock_image_dispatch);
   expect_register_dispatch(mock_image_ctx);
   expect_set_require_lock(mock_image_ctx, mock_image_dispatch, true);
-  expect_unblock_writes(mock_image_ctx);
   expect_set_state_unlocked(exclusive_lock);
   ASSERT_EQ(0, when_init(mock_image_ctx, exclusive_lock));