librbd: do not shut down exclusive lock while acquiring

Fixes: http://tracker.ceph.com/issues/16260
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit c5694fc6766fb8e213c4b65d2cd7b9d066b07ff7)

diff --git a/src/librbd/ImageState.cc b/src/librbd/ImageState.cc
index 0bf2e45f7c15edf1fb4374a86a938edbd0ba0fdb..39f4ee610664c2b73cf412cc2c13ac4189830350 100644 (file)
--- a/src/librbd/ImageState.cc
+++ b/src/librbd/ImageState.cc
@@ -105,6 +105,18 @@ template <typename I>
 void ImageState<I>::refresh(Context *on_finish) {
   CephContext *cct = m_image_ctx->cct;
   ldout(cct, 20) << __func__ << dendl;
+  refresh(false, on_finish);
+}
+
+template <typename I>
+void ImageState<I>::acquire_lock_refresh(Context *on_finish) {
+  CephContext *cct = m_image_ctx->cct;
+  ldout(cct, 20) << __func__ << dendl;
+  refresh(true, on_finish);
+}
+
+template <typename I>
+void ImageState<I>::refresh(bool acquiring_lock, Context *on_finish) {
 
   m_lock.Lock();
   if (is_closed()) {
@@ -115,6 +127,7 @@ void ImageState<I>::refresh(Context *on_finish) {
 
   Action action(ACTION_TYPE_REFRESH);
   action.refresh_seq = m_refresh_seq;
+  action.refresh_acquiring_lock = acquiring_lock;
   execute_action_unlock(action, on_finish);
 }
 
@@ -326,12 +339,15 @@ void ImageState<I>::send_refresh_unlock() {
   ldout(cct, 10) << this << " " << __func__ << dendl;
 
   m_state = STATE_REFRESHING;
+  assert(!m_actions_contexts.empty());
+  auto &action_context = m_actions_contexts.front().first;
+  assert(action_context.action_type == ACTION_TYPE_REFRESH);
 
   Context *ctx = create_async_context_callback(
     *m_image_ctx, create_context_callback<
       ImageState<I>, &ImageState<I>::handle_refresh>(this));
   image::RefreshRequest<I> *req = image::RefreshRequest<I>::create(
-    *m_image_ctx, ctx);
+    *m_image_ctx, action_context.refresh_acquiring_lock, ctx);
 
   m_lock.Unlock();
   req->send();
@@ -348,7 +364,13 @@ void ImageState<I>::handle_refresh(int r) {
   ActionContexts &action_contexts(m_actions_contexts.front());
   assert(action_contexts.first.action_type == ACTION_TYPE_REFRESH);
   assert(m_last_refresh <= action_contexts.first.refresh_seq);
-  m_last_refresh = action_contexts.first.refresh_seq;
+
+  if (r == -ERESTART) {
+    ldout(cct, 5) << "incomplete refresh: not updating sequence" << dendl;
+    r = 0;
+  } else {
+    m_last_refresh = action_contexts.first.refresh_seq;
+  }
 
   complete_action_unlock(STATE_OPEN, r);
 }

diff --git a/src/librbd/ImageState.h b/src/librbd/ImageState.h
index 2834116ad98dd5d99c96c25c860ad5b98ef5b4b9..b60172f0b0a773785893c531b67711647a4403e8 100644 (file)
--- a/src/librbd/ImageState.h
+++ b/src/librbd/ImageState.h
@@ -34,8 +34,9 @@ public:
   bool is_refresh_required() const;
 
   int refresh();
-  void refresh(Context *on_finish);
   int refresh_if_required();
+  void refresh(Context *on_finish);
+  void acquire_lock_refresh(Context *on_finish);
 
   void snap_set(const std::string &snap_name, Context *on_finish);
 
@@ -59,10 +60,11 @@ private:
 
   struct Action {
     ActionType action_type;
-    uint64_t refresh_seq;
+    uint64_t refresh_seq = 0;
+    bool refresh_acquiring_lock = false;
     std::string snap_name;
 
-    Action(ActionType action_type) : action_type(action_type), refresh_seq(0) {
+    Action(ActionType action_type) : action_type(action_type) {
     }
     inline bool operator==(const Action &action) const {
       if (action_type != action.action_type) {
@@ -70,7 +72,8 @@ private:
       }
       switch (action_type) {
       case ACTION_TYPE_REFRESH:
-        return refresh_seq == action.refresh_seq;
+        return (refresh_seq == action.refresh_seq &&
+                refresh_acquiring_lock == action.refresh_acquiring_lock);
       case ACTION_TYPE_SET_SNAP:
         return snap_name == action.snap_name;
       default:
@@ -95,6 +98,8 @@ private:
   bool is_transition_state() const;
   bool is_closed() const;
 
+  void refresh(bool acquiring_lock, Context *on_finish);
+
   void append_context(const Action &action, Context *context);
   void execute_next_action_unlock();
   void execute_action_unlock(const Action &action, Context *context);

diff --git a/src/librbd/exclusive_lock/AcquireRequest.cc b/src/librbd/exclusive_lock/AcquireRequest.cc
index c91cced0bd1b31c9a0b3eebbe4865c12d6c77ca1..f030b0ea3bce1d25c1556b68031d62ff16c7611f 100644 (file)
--- a/src/librbd/exclusive_lock/AcquireRequest.cc
+++ b/src/librbd/exclusive_lock/AcquireRequest.cc
@@ -145,7 +145,7 @@ Context *AcquireRequest<I>::send_refresh() {
 
   using klass = AcquireRequest<I>;
   Context *ctx = create_context_callback<klass, &klass::handle_refresh>(this);
-  m_image_ctx.state->refresh(ctx);
+  m_image_ctx.state->acquire_lock_refresh(ctx);
   return nullptr;
 }
 

diff --git a/src/librbd/image/OpenRequest.cc b/src/librbd/image/OpenRequest.cc
index 41f753e1c378b781137bdf662e6041861e501fde..7714722cb4e9568c8c8e7b9594e20ddfa2284674 100644 (file)
--- a/src/librbd/image/OpenRequest.cc
+++ b/src/librbd/image/OpenRequest.cc
@@ -374,7 +374,7 @@ void OpenRequest<I>::send_refresh() {
 
   using klass = OpenRequest<I>;
   RefreshRequest<I> *ctx = RefreshRequest<I>::create(
-    *m_image_ctx,
+    *m_image_ctx, false,
     create_context_callback<klass, &klass::handle_refresh>(this));
   ctx->send();
 }

diff --git a/src/librbd/image/RefreshRequest.cc b/src/librbd/image/RefreshRequest.cc
index dedf3073832a62553986046bdb3aa6ac19fbd368..ebd86efe7e7730c4e845729d1eb1d1c5ad35e24c 100644 (file)
--- a/src/librbd/image/RefreshRequest.cc
+++ b/src/librbd/image/RefreshRequest.cc
@@ -28,8 +28,9 @@ using util::create_async_context_callback;
 using util::create_context_callback;
 
 template <typename I>
-RefreshRequest<I>::RefreshRequest(I &image_ctx, Context *on_finish)
-  : m_image_ctx(image_ctx),
+RefreshRequest<I>::RefreshRequest(I &image_ctx, bool acquiring_lock,
+                                  Context *on_finish)
+  : m_image_ctx(image_ctx), m_acquiring_lock(acquiring_lock),
     m_on_finish(create_async_context_callback(m_image_ctx, on_finish)),
     m_error_result(0), m_flush_aio(false), m_exclusive_lock(nullptr),
     m_object_map(nullptr), m_journal(nullptr), m_refresh_parent(nullptr) {
@@ -271,6 +272,12 @@ Context *RefreshRequest<I>::handle_v2_get_mutable_metadata(int *result) {
     return m_on_finish;
   }
 
+  if (m_acquiring_lock && (m_features & RBD_FEATURE_EXCLUSIVE_LOCK) == 0) {
+    ldout(cct, 5) << "ignoring dynamically disabled exclusive lock" << dendl;
+    m_features |= RBD_FEATURE_EXCLUSIVE_LOCK;
+    m_incomplete_update = true;
+  }
+
   send_v2_get_flags();
   return nullptr;
 }
@@ -636,6 +643,7 @@ Context *RefreshRequest<I>::handle_v2_apply(int *result) {
   ldout(cct, 10) << this << " " << __func__ << dendl;
 
   apply();
+
   return send_v2_finalize_refresh_parent();
 }
 
@@ -779,6 +787,11 @@ Context *RefreshRequest<I>::handle_v2_close_object_map(int *result) {
 
 template <typename I>
 Context *RefreshRequest<I>::send_flush_aio() {
+  if (m_incomplete_update && m_error_result == 0) {
+    // if this was a partial refresh, notify ImageState
+    m_error_result = -ERESTART;
+  }
+
   if (m_flush_aio) {
     CephContext *cct = m_image_ctx.cct;
     ldout(cct, 10) << this << " " << __func__ << dendl;

diff --git a/src/librbd/image/RefreshRequest.h b/src/librbd/image/RefreshRequest.h
index 63c858b9141f99b8f9b784426ff4c34e1c723b3d..58b6487e1e767acd05cd21f49a178ae77ce91741 100644 (file)
--- a/src/librbd/image/RefreshRequest.h
+++ b/src/librbd/image/RefreshRequest.h
@@ -27,11 +27,12 @@ template<typename> class RefreshParentRequest;
 template<typename ImageCtxT = ImageCtx>
 class RefreshRequest {
 public:
-  static RefreshRequest *create(ImageCtxT &image_ctx, Context *on_finish) {
-    return new RefreshRequest(image_ctx, on_finish);
+  static RefreshRequest *create(ImageCtxT &image_ctx, bool acquiring_lock,
+                                Context *on_finish) {
+    return new RefreshRequest(image_ctx, acquiring_lock, on_finish);
   }
 
-  RefreshRequest(ImageCtxT &image_ctx, Context *on_finish);
+  RefreshRequest(ImageCtxT &image_ctx, bool acquiring_lock, Context *on_finish);
   ~RefreshRequest();
 
   void send();
@@ -97,6 +98,7 @@ private:
    */
 
   ImageCtxT &m_image_ctx;
+  bool m_acquiring_lock;
   Context *m_on_finish;
 
   int m_error_result;
@@ -129,6 +131,7 @@ private:
   bool m_exclusive_locked;
 
   bool m_blocked_writes = false;
+  bool m_incomplete_update = false;
 
   void send_v1_read_header();
   Context *handle_v1_read_header(int *result);

diff --git a/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc b/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc
index a86fbb31aca44cebb6bab8763610dacad206c4d2..c98d69463e9ba6ebe96552acb9dee372a479333e 100644 (file)
--- a/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc
+++ b/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc
@@ -65,7 +65,7 @@ public:
   }
 
   void expect_refresh(MockImageCtx &mock_image_ctx, int r) {
-    EXPECT_CALL(*mock_image_ctx.state, refresh(_))
+    EXPECT_CALL(*mock_image_ctx.state, acquire_lock_refresh(_))
                   .WillOnce(CompleteContext(r, mock_image_ctx.image_ctx->op_work_queue));
   }
 

diff --git a/src/test/librbd/image/test_mock_RefreshRequest.cc b/src/test/librbd/image/test_mock_RefreshRequest.cc
index 01a7381064628d7b8d3e45bf450c52896be4406e..05034919b149fda74b6b428ca137c2b29dff4472 100644 (file)
--- a/src/test/librbd/image/test_mock_RefreshRequest.cc
+++ b/src/test/librbd/image/test_mock_RefreshRequest.cc
@@ -308,7 +308,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessV1) {
   expect_init_layout(mock_image_ctx);
 
   C_SaferCond ctx;
-  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx);
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx);
   req->send();
 
   ASSERT_EQ(0, ctx.wait());
@@ -334,7 +334,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessSnapshotV1) {
   expect_add_snap(mock_image_ctx, "snap", ictx->snap_ids.begin()->second);
 
   C_SaferCond ctx;
-  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx);
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx);
   req->send();
 
   ASSERT_EQ(0, ctx.wait());
@@ -361,7 +361,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessV2) {
   }
 
   C_SaferCond ctx;
-  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx);
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx);
   req->send();
 
   ASSERT_EQ(0, ctx.wait());
@@ -392,7 +392,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessSnapshotV2) {
   expect_add_snap(mock_image_ctx, "snap", ictx->snap_ids.begin()->second);
 
   C_SaferCond ctx;
-  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx);
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx);
   req->send();
 
   ASSERT_EQ(0, ctx.wait());
@@ -425,7 +425,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessSetSnapshotV2) {
   expect_get_snap_id(mock_image_ctx, "snap", ictx->snap_ids.begin()->second);
 
   C_SaferCond ctx;
-  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx);
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx);
   req->send();
 
   ASSERT_EQ(0, ctx.wait());
@@ -475,7 +475,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessChild) {
   expect_refresh_parent_finalize(mock_image_ctx, *mock_refresh_parent_request, 0);
 
   C_SaferCond ctx;
-  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx);
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx);
   req->send();
 
   ASSERT_EQ(0, ctx.wait());
@@ -521,12 +521,46 @@ TEST_F(TestMockImageRefreshRequest, DisableExclusiveLock) {
   expect_shut_down_exclusive_lock(mock_image_ctx, *mock_exclusive_lock, 0);
 
   C_SaferCond ctx;
-  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx);
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx);
   req->send();
 
   ASSERT_EQ(0, ctx.wait());
 }
 
+TEST_F(TestMockImageRefreshRequest, DisableExclusiveLockWhileAcquiringLock) {
+  REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK);
+
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(m_image_name, &ictx));
+
+  MockRefreshImageCtx mock_image_ctx(*ictx);
+  MockRefreshParentRequest mock_refresh_parent_request;
+
+  MockExclusiveLock mock_exclusive_lock;
+  mock_image_ctx.exclusive_lock = &mock_exclusive_lock;
+
+  ASSERT_EQ(0, update_features(ictx,
+                               RBD_FEATURE_EXCLUSIVE_LOCK |
+                               RBD_FEATURE_OBJECT_MAP |
+                               RBD_FEATURE_FAST_DIFF |
+                               RBD_FEATURE_JOURNALING, false));
+
+  expect_op_work_queue(mock_image_ctx);
+  expect_test_features(mock_image_ctx);
+
+  // verify that exclusive lock is properly handled when object map
+  // and journaling were never enabled (or active)
+  InSequence seq;
+  expect_get_mutable_metadata(mock_image_ctx, 0);
+  expect_get_flags(mock_image_ctx, 0);
+  expect_refresh_parent_is_required(mock_refresh_parent_request, false);
+
+  C_SaferCond ctx;
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, true, &ctx);
+  req->send();
+
+  ASSERT_EQ(-ERESTART, ctx.wait());
+}
 TEST_F(TestMockImageRefreshRequest, EnableJournalWithExclusiveLock) {
   REQUIRE_FEATURE(RBD_FEATURE_JOURNALING);
 
@@ -557,7 +591,7 @@ TEST_F(TestMockImageRefreshRequest, EnableJournalWithExclusiveLock) {
   expect_open_journal(mock_image_ctx, mock_journal, 0);
 
   C_SaferCond ctx;
-  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx);
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx);
   req->send();
 
   ASSERT_EQ(0, ctx.wait());
@@ -591,7 +625,7 @@ TEST_F(TestMockImageRefreshRequest, EnableJournalWithoutExclusiveLock) {
   expect_set_require_lock_on_read(mock_image_ctx);
 
   C_SaferCond ctx;
-  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx);
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx);
   req->send();
 
   ASSERT_EQ(0, ctx.wait());
@@ -633,7 +667,7 @@ TEST_F(TestMockImageRefreshRequest, DisableJournal) {
   expect_unblock_writes(mock_image_ctx);
 
   C_SaferCond ctx;
-  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx);
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx);
   req->send();
 
   ASSERT_EQ(0, ctx.wait());
@@ -667,7 +701,7 @@ TEST_F(TestMockImageRefreshRequest, EnableObjectMapWithExclusiveLock) {
   expect_open_object_map(mock_image_ctx, &mock_object_map, 0);
 
   C_SaferCond ctx;
-  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx);
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx);
   req->send();
 
   ASSERT_EQ(0, ctx.wait());
@@ -698,7 +732,7 @@ TEST_F(TestMockImageRefreshRequest, EnableObjectMapWithoutExclusiveLock) {
   expect_refresh_parent_is_required(mock_refresh_parent_request, false);
 
   C_SaferCond ctx;
-  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx);
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx);
   req->send();
 
   ASSERT_EQ(0, ctx.wait());
@@ -739,7 +773,7 @@ TEST_F(TestMockImageRefreshRequest, DisableObjectMap) {
   expect_close_object_map(mock_image_ctx, *mock_object_map, 0);
 
   C_SaferCond ctx;
-  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx);
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx);
   req->send();
 
   ASSERT_EQ(0, ctx.wait());
@@ -773,7 +807,7 @@ TEST_F(TestMockImageRefreshRequest, OpenObjectMapError) {
   expect_open_object_map(mock_image_ctx, mock_object_map, -EFBIG);
 
   C_SaferCond ctx;
-  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx);
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx);
   req->send();
 
   ASSERT_EQ(0, ctx.wait());

diff --git a/src/test/librbd/mock/MockImageState.h b/src/test/librbd/mock/MockImageState.h
index bf341f58033a9b15c2fd2fb7ce96f431188cc08f..3ba1ee5d7dd0eb191b74845fbe9bbfc8c5a28b53 100644 (file)
--- a/src/test/librbd/mock/MockImageState.h
+++ b/src/test/librbd/mock/MockImageState.h
@@ -13,6 +13,7 @@ namespace librbd {
 struct MockImageState {
   MOCK_CONST_METHOD0(is_refresh_required, bool());
   MOCK_METHOD1(refresh, void(Context*));
+  MOCK_METHOD1(acquire_lock_refresh, void(Context*));
 
   MOCK_METHOD1(open, void(Context*));