librbd: interlock image state machine and update features operations

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

diff --git a/src/librbd/operation/DisableFeaturesRequest.cc b/src/librbd/operation/DisableFeaturesRequest.cc
index e154d1e1b84ae362746bf7b68b1b6ba2a67bec7d..630a325690695a18cddcb325691b354b5899d482 100644 (file)
--- a/src/librbd/operation/DisableFeaturesRequest.cc
+++ b/src/librbd/operation/DisableFeaturesRequest.cc
@@ -8,6 +8,7 @@
 #include "librbd/AioImageRequestWQ.h"
 #include "librbd/ExclusiveLock.h"
 #include "librbd/ImageCtx.h"
+#include "librbd/ImageState.h"
 #include "librbd/Journal.h"
 #include "librbd/Utils.h"
 #include "librbd/image/SetFlagsRequest.h"
@@ -24,6 +25,7 @@
 namespace librbd {
 namespace operation {
 
+using util::create_async_context_callback;
 using util::create_context_callback;
 using util::create_rados_ack_callback;
 
@@ -44,7 +46,7 @@ void DisableFeaturesRequest<I>::send_op() {
   ldout(cct, 20) << this << " " << __func__ << ": features=" << m_features
                 << dendl;
 
-  send_block_writes();
+  send_prepare_lock();
 }
 
 template <typename I>
@@ -59,12 +61,40 @@ bool DisableFeaturesRequest<I>::should_complete(int r) {
   return true;
 }
 
+template <typename I>
+void DisableFeaturesRequest<I>::send_prepare_lock() {
+  I &image_ctx = this->m_image_ctx;
+  CephContext *cct = image_ctx.cct;
+  ldout(cct, 20) << this << " " << __func__ << dendl;
+
+  image_ctx.state->prepare_lock(create_async_context_callback(
+    image_ctx, create_context_callback<
+    DisableFeaturesRequest<I>,
+    &DisableFeaturesRequest<I>::handle_prepare_lock>(this)));
+}
+
+template <typename I>
+Context *DisableFeaturesRequest<I>::handle_prepare_lock(int *result) {
+  I &image_ctx = this->m_image_ctx;
+  CephContext *cct = image_ctx.cct;
+  ldout(cct, 20) << this << " " << __func__ << ": r=" << *result << dendl;
+
+  if (*result < 0) {
+    lderr(cct) << "failed to lock image: " << cpp_strerror(*result) << dendl;
+    return this->create_context_finisher(*result);
+  }
+
+  send_block_writes();
+  return nullptr;
+}
+
 template <typename I>
 void DisableFeaturesRequest<I>::send_block_writes() {
   I &image_ctx = this->m_image_ctx;
   CephContext *cct = image_ctx.cct;
   ldout(cct, 20) << this << " " << __func__ << dendl;
 
+  RWLock::WLocker locker(image_ctx.owner_lock);
   image_ctx.aio_work_queue->block_writes(create_context_callback<
     DisableFeaturesRequest<I>,
     &DisableFeaturesRequest<I>::handle_block_writes>(this));
@@ -78,9 +108,9 @@ Context *DisableFeaturesRequest<I>::handle_block_writes(int *result) {
 
   if (*result < 0) {
     lderr(cct) << "failed to block writes: " << cpp_strerror(*result) << dendl;
-    image_ctx.aio_work_queue->unblock_writes();
-    return this->create_context_finisher(*result);
+    return handle_finish(*result);
   }
+  m_writes_blocked = true;
 
   {
     RWLock::WLocker locker(image_ctx.owner_lock);
@@ -609,6 +639,7 @@ Context *DisableFeaturesRequest<I>::handle_finish(int r) {
 
     image_ctx.aio_work_queue->unblock_writes();
   }
+  image_ctx.state->handle_prepare_lock_complete();
 
   return this->create_context_finisher(r);
 }

diff --git a/src/librbd/operation/DisableFeaturesRequest.h b/src/librbd/operation/DisableFeaturesRequest.h
index 424a0558e1ecca88e47c43e00a6be435194c1cca..403e077040e82111395b79549f1abb740f51d2e7 100644 (file)
--- a/src/librbd/operation/DisableFeaturesRequest.h
+++ b/src/librbd/operation/DisableFeaturesRequest.h
@@ -47,6 +47,9 @@ private:
    * <start>
    *    |
    *    v
+   * STATE_PREPARE_LOCK
+   *    |
+   *    v
    * STATE_BLOCK_WRITES
    *    |
    *    v
@@ -101,6 +104,7 @@ private:
   uint64_t m_features;
 
   bool m_acquired_lock = false;
+  bool m_writes_blocked = false;
   bool m_snap_lock_acquired = false;
   bool m_requests_blocked = false;
   bool m_disabling_journal = false;
@@ -112,6 +116,9 @@ private:
   cls::rbd::MirrorMode m_mirror_mode = cls::rbd::MIRROR_MODE_DISABLED;
   bufferlist m_out_bl;
 
+  void send_prepare_lock();
+  Context *handle_prepare_lock(int *result);
+
   void send_block_writes();
   Context *handle_block_writes(int *result);
 

diff --git a/src/librbd/operation/EnableFeaturesRequest.cc b/src/librbd/operation/EnableFeaturesRequest.cc
index 91b077a73cd432f2884beaec4fed0f9ccb6af085..71fc4f1bb92a160b66a52b0333d1aea4397be359 100644 (file)
--- a/src/librbd/operation/EnableFeaturesRequest.cc
+++ b/src/librbd/operation/EnableFeaturesRequest.cc
@@ -7,6 +7,7 @@
 #include "librbd/AioImageRequestWQ.h"
 #include "librbd/ExclusiveLock.h"
 #include "librbd/ImageCtx.h"
+#include "librbd/ImageState.h"
 #include "librbd/Journal.h"
 #include "librbd/Utils.h"
 #include "librbd/image/SetFlagsRequest.h"
@@ -21,6 +22,7 @@
 namespace librbd {
 namespace operation {
 
+using util::create_async_context_callback;
 using util::create_context_callback;
 using util::create_rados_ack_callback;
 
@@ -40,7 +42,7 @@ void EnableFeaturesRequest<I>::send_op() {
 
   ldout(cct, 20) << this << " " << __func__ << ": features=" << m_features
                 << dendl;
-  send_block_writes();
+  send_prepare_lock();
 }
 
 template <typename I>
@@ -55,12 +57,40 @@ bool EnableFeaturesRequest<I>::should_complete(int r) {
   return true;
 }
 
+template <typename I>
+void EnableFeaturesRequest<I>::send_prepare_lock() {
+  I &image_ctx = this->m_image_ctx;
+  CephContext *cct = image_ctx.cct;
+  ldout(cct, 20) << this << " " << __func__ << dendl;
+
+  image_ctx.state->prepare_lock(create_async_context_callback(
+    image_ctx, create_context_callback<
+    EnableFeaturesRequest<I>,
+    &EnableFeaturesRequest<I>::handle_prepare_lock>(this)));
+}
+
+template <typename I>
+Context *EnableFeaturesRequest<I>::handle_prepare_lock(int *result) {
+  I &image_ctx = this->m_image_ctx;
+  CephContext *cct = image_ctx.cct;
+  ldout(cct, 20) << this << " " << __func__ << ": r=" << *result << dendl;
+
+  if (*result < 0) {
+    lderr(cct) << "failed to lock image: " << cpp_strerror(*result) << dendl;
+    return this->create_context_finisher(*result);
+  }
+
+  send_block_writes();
+  return nullptr;
+}
+
 template <typename I>
 void EnableFeaturesRequest<I>::send_block_writes() {
   I &image_ctx = this->m_image_ctx;
   CephContext *cct = image_ctx.cct;
   ldout(cct, 20) << this << " " << __func__ << dendl;
 
+  RWLock::WLocker locker(image_ctx.owner_lock);
   image_ctx.aio_work_queue->block_writes(create_context_callback<
     EnableFeaturesRequest<I>,
     &EnableFeaturesRequest<I>::handle_block_writes>(this));
@@ -74,9 +104,9 @@ Context *EnableFeaturesRequest<I>::handle_block_writes(int *result) {
 
   if (*result < 0) {
     lderr(cct) << "failed to block writes: " << cpp_strerror(*result) << dendl;
-    image_ctx.aio_work_queue->unblock_writes();
-    return this->create_context_finisher(*result);
+    return handle_finish(*result);
   }
+  m_writes_blocked = true;
 
   send_get_mirror_mode();
   return nullptr;
@@ -437,8 +467,11 @@ Context *EnableFeaturesRequest<I>::handle_finish(int r) {
     if (image_ctx.exclusive_lock != nullptr && m_requests_blocked) {
       image_ctx.exclusive_lock->unblock_requests();
     }
-    image_ctx.aio_work_queue->unblock_writes();
+    if (m_writes_blocked) {
+      image_ctx.aio_work_queue->unblock_writes();
+    }
   }
+  image_ctx.state->handle_prepare_lock_complete();
 
   return this->create_context_finisher(r);
 }

diff --git a/src/librbd/operation/EnableFeaturesRequest.h b/src/librbd/operation/EnableFeaturesRequest.h
index 8e093bc31afb1a759f8a58521f7b8d9acd5ca5b1..feda681b255a2fbe7f22833b8346b20e2fc1109b 100644 (file)
--- a/src/librbd/operation/EnableFeaturesRequest.h
+++ b/src/librbd/operation/EnableFeaturesRequest.h
@@ -46,6 +46,9 @@ private:
    * <start>
    *    |
    *    v
+   * STATE_PREPARE_LOCK
+   *    |
+   *    v
    * STATE_BLOCK_WRITES
    *    |
    *    v
@@ -83,6 +86,7 @@ private:
 
   bool m_enable_mirroring = false;
   bool m_requests_blocked = false;
+  bool m_writes_blocked = false;
 
   uint64_t m_new_features = 0;
   uint64_t m_enable_flags = 0;
@@ -90,6 +94,9 @@ private:
 
   bufferlist m_out_bl;
 
+  void send_prepare_lock();
+  Context *handle_prepare_lock(int *result);
+
   void send_block_writes();
   Context *handle_block_writes(int *result);
 

diff --git a/src/test/librbd/operation/test_mock_DisableFeaturesRequest.cc b/src/test/librbd/operation/test_mock_DisableFeaturesRequest.cc
index 433a08b294e197254860db5c7442b2f1ce27687d..465dde0ab96058fa68bbc417d7bcde4e19ab3da6 100644 (file)
--- a/src/test/librbd/operation/test_mock_DisableFeaturesRequest.cc
+++ b/src/test/librbd/operation/test_mock_DisableFeaturesRequest.cc
@@ -190,6 +190,18 @@ public:
     librados::IoCtx &m_ioctx;
   };
 
+  void expect_prepare_lock(MockOperationImageCtx &mock_image_ctx) {
+    EXPECT_CALL(*mock_image_ctx.state, prepare_lock(_))
+      .WillOnce(Invoke([](Context *on_ready) {
+           on_ready->complete(0);
+         }));
+    expect_op_work_queue(mock_image_ctx);
+  }
+
+  void expect_handle_prepare_lock_complete(MockOperationImageCtx &mock_image_ctx) {
+    EXPECT_CALL(*mock_image_ctx.state, handle_prepare_lock_complete());
+  }
+
   void expect_block_writes(MockOperationImageCtx &mock_image_ctx) {
     EXPECT_CALL(*mock_image_ctx.aio_work_queue, block_writes(_))
       .WillOnce(CompleteContext(0, mock_image_ctx.image_ctx->op_work_queue));
@@ -297,6 +309,7 @@ TEST_F(TestMockOperationDisableFeaturesRequest, All) {
   MockRemoveObjectMapRequest mock_remove_object_map_request;
 
   ::testing::InSequence seq;
+  expect_prepare_lock(mock_image_ctx);
   expect_block_writes(mock_image_ctx);
   if (mock_image_ctx.journal != nullptr) {
     expect_is_journal_replaying(*mock_image_ctx.journal);
@@ -324,6 +337,7 @@ TEST_F(TestMockOperationDisableFeaturesRequest, All) {
   }
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
+  expect_handle_prepare_lock_complete(mock_image_ctx);
 
   C_SaferCond cond_ctx;
   MockDisableFeaturesRequest *req = new MockDisableFeaturesRequest(
@@ -354,6 +368,7 @@ TEST_F(TestMockOperationDisableFeaturesRequest, ObjectMap) {
   MockRemoveObjectMapRequest mock_remove_object_map_request;
 
   ::testing::InSequence seq;
+  expect_prepare_lock(mock_image_ctx);
   expect_block_writes(mock_image_ctx);
   if (mock_image_ctx.journal != nullptr) {
     expect_is_journal_replaying(*mock_image_ctx.journal);
@@ -367,6 +382,7 @@ TEST_F(TestMockOperationDisableFeaturesRequest, ObjectMap) {
   expect_notify_update(mock_image_ctx);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
+  expect_handle_prepare_lock_complete(mock_image_ctx);
   expect_commit_op_event(mock_image_ctx, 0);
 
   C_SaferCond cond_ctx;
@@ -399,6 +415,7 @@ TEST_F(TestMockOperationDisableFeaturesRequest, ObjectMapError) {
   MockRemoveObjectMapRequest mock_remove_object_map_request;
 
   ::testing::InSequence seq;
+  expect_prepare_lock(mock_image_ctx);
   expect_block_writes(mock_image_ctx);
   if (mock_image_ctx.journal != nullptr) {
     expect_is_journal_replaying(*mock_image_ctx.journal);
@@ -409,6 +426,7 @@ TEST_F(TestMockOperationDisableFeaturesRequest, ObjectMapError) {
                                         mock_remove_object_map_request, -EINVAL);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
+  expect_handle_prepare_lock_complete(mock_image_ctx);
   expect_commit_op_event(mock_image_ctx, -EINVAL);
 
   C_SaferCond cond_ctx;
@@ -441,6 +459,7 @@ TEST_F(TestMockOperationDisableFeaturesRequest, Mirroring) {
   MockDisableMirrorRequest mock_disable_mirror_request;
 
   ::testing::InSequence seq;
+  expect_prepare_lock(mock_image_ctx);
   expect_block_writes(mock_image_ctx);
   expect_is_journal_replaying(*mock_image_ctx.journal);
   expect_block_requests(mock_image_ctx);
@@ -454,6 +473,7 @@ TEST_F(TestMockOperationDisableFeaturesRequest, Mirroring) {
   expect_set_journal_policy(mock_image_ctx);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
+  expect_handle_prepare_lock_complete(mock_image_ctx);
 
   C_SaferCond cond_ctx;
   MockDisableFeaturesRequest *req = new MockDisableFeaturesRequest(
@@ -484,6 +504,7 @@ TEST_F(TestMockOperationDisableFeaturesRequest, MirroringError) {
   MockDisableMirrorRequest mock_disable_mirror_request;
 
   ::testing::InSequence seq;
+  expect_prepare_lock(mock_image_ctx);
   expect_block_writes(mock_image_ctx);
   expect_is_journal_replaying(*mock_image_ctx.journal);
   expect_block_requests(mock_image_ctx);
@@ -497,6 +518,7 @@ TEST_F(TestMockOperationDisableFeaturesRequest, MirroringError) {
   expect_set_journal_policy(mock_image_ctx);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
+  expect_handle_prepare_lock_complete(mock_image_ctx);
 
   C_SaferCond cond_ctx;
   MockDisableFeaturesRequest *req = new MockDisableFeaturesRequest(

diff --git a/src/test/librbd/operation/test_mock_EnableFeaturesRequest.cc b/src/test/librbd/operation/test_mock_EnableFeaturesRequest.cc
index fe2368ca9578610cafecb95b5f34a66e44e1a4f1..90ee5f79fce02d5152e01b0e00b748e01d6679c9 100644 (file)
--- a/src/test/librbd/operation/test_mock_EnableFeaturesRequest.cc
+++ b/src/test/librbd/operation/test_mock_EnableFeaturesRequest.cc
@@ -199,6 +199,18 @@ public:
     ASSERT_EQ(0U, features & features_to_disable);
   }
 
+  void expect_prepare_lock(MockOperationImageCtx &mock_image_ctx) {
+    EXPECT_CALL(*mock_image_ctx.state, prepare_lock(_))
+      .WillOnce(Invoke([](Context *on_ready) {
+           on_ready->complete(0);
+         }));
+    expect_op_work_queue(mock_image_ctx);
+  }
+
+  void expect_handle_prepare_lock_complete(MockOperationImageCtx &mock_image_ctx) {
+    EXPECT_CALL(*mock_image_ctx.state, handle_prepare_lock_complete());
+  }
+
   void expect_block_writes(MockOperationImageCtx &mock_image_ctx) {
     EXPECT_CALL(*mock_image_ctx.aio_work_queue, block_writes(_))
       .WillOnce(CompleteContext(0, mock_image_ctx.image_ctx->op_work_queue));
@@ -288,6 +300,7 @@ TEST_F(TestMockOperationEnableFeaturesRequest, All) {
   MockCreateObjectMapRequest mock_create_object_map_request;
 
   ::testing::InSequence seq;
+  expect_prepare_lock(mock_image_ctx);
   expect_block_writes(mock_image_ctx);
   expect_block_requests(mock_image_ctx);
   if (features_to_enable & RBD_FEATURE_JOURNALING) {
@@ -305,6 +318,7 @@ TEST_F(TestMockOperationEnableFeaturesRequest, All) {
   expect_notify_update(mock_image_ctx);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
+  expect_handle_prepare_lock_complete(mock_image_ctx);
 
   C_SaferCond cond_ctx;
   MockEnableFeaturesRequest *req = new MockEnableFeaturesRequest(
@@ -341,6 +355,7 @@ TEST_F(TestMockOperationEnableFeaturesRequest, ObjectMap) {
   MockCreateObjectMapRequest mock_create_object_map_request;
 
   ::testing::InSequence seq;
+  expect_prepare_lock(mock_image_ctx);
   expect_block_writes(mock_image_ctx);
   if (mock_image_ctx.journal != nullptr) {
     expect_is_journal_replaying(*mock_image_ctx.journal);
@@ -354,6 +369,7 @@ TEST_F(TestMockOperationEnableFeaturesRequest, ObjectMap) {
   expect_notify_update(mock_image_ctx);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
+  expect_handle_prepare_lock_complete(mock_image_ctx);
   expect_commit_op_event(mock_image_ctx, 0);
 
   C_SaferCond cond_ctx;
@@ -391,6 +407,7 @@ TEST_F(TestMockOperationEnableFeaturesRequest, ObjectMapError) {
   MockCreateObjectMapRequest mock_create_object_map_request;
 
   ::testing::InSequence seq;
+  expect_prepare_lock(mock_image_ctx);
   expect_block_writes(mock_image_ctx);
   if (mock_image_ctx.journal != nullptr) {
     expect_is_journal_replaying(*mock_image_ctx.journal);
@@ -403,6 +420,7 @@ TEST_F(TestMockOperationEnableFeaturesRequest, ObjectMapError) {
     mock_image_ctx, mock_create_object_map_request, -EINVAL);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
+  expect_handle_prepare_lock_complete(mock_image_ctx);
   expect_commit_op_event(mock_image_ctx, -EINVAL);
 
   C_SaferCond cond_ctx;
@@ -440,6 +458,7 @@ TEST_F(TestMockOperationEnableFeaturesRequest, SetFlagsError) {
   MockCreateObjectMapRequest mock_create_object_map_request;
 
   ::testing::InSequence seq;
+  expect_prepare_lock(mock_image_ctx);
   expect_block_writes(mock_image_ctx);
   if (mock_image_ctx.journal != nullptr) {
     expect_is_journal_replaying(*mock_image_ctx.journal);
@@ -450,6 +469,7 @@ TEST_F(TestMockOperationEnableFeaturesRequest, SetFlagsError) {
                                 mock_set_flags_request, -EINVAL);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
+  expect_handle_prepare_lock_complete(mock_image_ctx);
   expect_commit_op_event(mock_image_ctx, -EINVAL);
 
   C_SaferCond cond_ctx;
@@ -488,6 +508,7 @@ TEST_F(TestMockOperationEnableFeaturesRequest, Mirroring) {
   MockEnableMirrorRequest mock_enable_mirror_request;
 
   ::testing::InSequence seq;
+  expect_prepare_lock(mock_image_ctx);
   expect_block_writes(mock_image_ctx);
   expect_block_requests(mock_image_ctx);
   expect_create_journal_request_send(mock_image_ctx,
@@ -497,6 +518,7 @@ TEST_F(TestMockOperationEnableFeaturesRequest, Mirroring) {
   expect_notify_update(mock_image_ctx);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
+  expect_handle_prepare_lock_complete(mock_image_ctx);
 
   C_SaferCond cond_ctx;
   MockEnableFeaturesRequest *req = new MockEnableFeaturesRequest(
@@ -534,12 +556,14 @@ TEST_F(TestMockOperationEnableFeaturesRequest, JournalingError) {
   MockEnableMirrorRequest mock_enable_mirror_request;
 
   ::testing::InSequence seq;
+  expect_prepare_lock(mock_image_ctx);
   expect_block_writes(mock_image_ctx);
   expect_block_requests(mock_image_ctx);
   expect_create_journal_request_send(mock_image_ctx,
                                      mock_create_journal_request, -EINVAL);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
+  expect_handle_prepare_lock_complete(mock_image_ctx);
 
   C_SaferCond cond_ctx;
   MockEnableFeaturesRequest *req = new MockEnableFeaturesRequest(
@@ -577,6 +601,7 @@ TEST_F(TestMockOperationEnableFeaturesRequest, MirroringError) {
   MockEnableMirrorRequest mock_enable_mirror_request;
 
   ::testing::InSequence seq;
+  expect_prepare_lock(mock_image_ctx);
   expect_block_writes(mock_image_ctx);
   expect_block_requests(mock_image_ctx);
   expect_create_journal_request_send(mock_image_ctx,
@@ -586,6 +611,7 @@ TEST_F(TestMockOperationEnableFeaturesRequest, MirroringError) {
   expect_notify_update(mock_image_ctx);
   expect_unblock_requests(mock_image_ctx);
   expect_unblock_writes(mock_image_ctx);
+  expect_handle_prepare_lock_complete(mock_image_ctx);
 
   C_SaferCond cond_ctx;
   MockEnableFeaturesRequest *req = new MockEnableFeaturesRequest(