librbd: refresh image after acquiring exclusive lock

It's possible that the object map or journaling features have
been disabled while the lock was not owned.

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

diff --git a/src/librbd/exclusive_lock/AcquireRequest.cc b/src/librbd/exclusive_lock/AcquireRequest.cc
index 4a4ee6d051124d73609aebd75da344c845f99b3a..c91cced0bd1b31c9a0b3eebbe4865c12d6c77ca1 100644 (file)
--- a/src/librbd/exclusive_lock/AcquireRequest.cc
+++ b/src/librbd/exclusive_lock/AcquireRequest.cc
@@ -10,6 +10,7 @@
 #include "include/stringify.h"
 #include "librbd/ExclusiveLock.h"
 #include "librbd/ImageCtx.h"
+#include "librbd/ImageState.h"
 #include "librbd/ImageWatcher.h"
 #include "librbd/Journal.h"
 #include "librbd/ObjectMap.h"
@@ -123,7 +124,7 @@ Context *AcquireRequest<I>::handle_lock(int *ret_val) {
   ldout(cct, 10) << __func__ << ": r=" << *ret_val << dendl;
 
   if (*ret_val == 0) {
-    return send_open_object_map();
+    return send_refresh();
   } else if (*ret_val != -EBUSY) {
     lderr(cct) << "failed to lock: " << cpp_strerror(*ret_val) << dendl;
     return m_on_finish;
@@ -133,6 +134,37 @@ Context *AcquireRequest<I>::handle_lock(int *ret_val) {
   return nullptr;
 }
 
+template <typename I>
+Context *AcquireRequest<I>::send_refresh() {
+  if (!m_image_ctx.state->is_refresh_required()) {
+    return send_open_object_map();
+  }
+
+  CephContext *cct = m_image_ctx.cct;
+  ldout(cct, 10) << __func__ << dendl;
+
+  using klass = AcquireRequest<I>;
+  Context *ctx = create_context_callback<klass, &klass::handle_refresh>(this);
+  m_image_ctx.state->refresh(ctx);
+  return nullptr;
+}
+
+template <typename I>
+Context *AcquireRequest<I>::handle_refresh(int *ret_val) {
+  CephContext *cct = m_image_ctx.cct;
+  ldout(cct, 10) << __func__ << ": r=" << *ret_val << dendl;
+
+  if (*ret_val < 0) {
+    lderr(cct) << "failed to refresh image: " << cpp_strerror(*ret_val)
+               << dendl;
+    m_error_result = *ret_val;
+    send_unlock();
+    return nullptr;
+  }
+
+  return send_open_object_map();
+}
+
 template <typename I>
 Context *AcquireRequest<I>::send_open_journal() {
   // alert caller that we now own the exclusive lock

diff --git a/src/librbd/exclusive_lock/AcquireRequest.h b/src/librbd/exclusive_lock/AcquireRequest.h
index 9a16fc56e2ab5904911c4bb8382f99bb1e07300d..4990abbd811c04d5adeb94fc2a9bc68fe61e4493 100644 (file)
--- a/src/librbd/exclusive_lock/AcquireRequest.h
+++ b/src/librbd/exclusive_lock/AcquireRequest.h
@@ -45,32 +45,34 @@ private:
    *    |     |   . . . . . . . . . . . . . . . . . . . . . .             |
    *    |     |   .                                         .             |
    *    |     v   v      (EBUSY)                            .             |
-   *    \--> LOCK_IMAGE * * * * * * * >   GET_LOCKERS . . . .             |
-   *          .   |                         |                             |
-   *    . . . .   |                         |                             |
-   *    .         v                         v                             |
-   *    .     OPEN_OBJECT_MAP (skip if    GET_WATCHERS . . .              |
-   *    .         |            disabled)    |              .              |
-   *    .         v                         v              .              |
-   *    . . > OPEN_JOURNAL (skip if       BLACKLIST        . (blacklist   |
-   *    .         |   *     disabled)       |              .  disabled)   |
-   *    .         |   *                     v              .              |
-   *    .         |   * * * * * * * *     BREAK_LOCK < . . .              |
-   *    .         v                 *       |                             |
-   *    .     ALLOCATE_JOURNAL_TAG  *       \-----------------------------/
-   *    .         |            *    *
-   *    .         |            *    *
-   *    .         |            v    v
-   *    .         |         CLOSE_JOURNAL
-   *    .         |               |
-   *    .         |               v
-   *    .         |         CLOSE_OBJECT_MAP
-   *    .         |               |
-   *    .         |               v
-   *    .         |         UNLOCK_IMAGE
-   *    .         |               |
-   *    .         v               |
-   *    . . > <finish> <----------/
+   *    \--> LOCK_IMAGE * * * * * * * * > GET_LOCKERS . . . .             |
+   *              |                         |                             |
+   *              v                         v                             |
+   *         REFRESH (skip if not         GET_WATCHERS                    |
+   *              |   needed)               |                             |
+   *              v                         v                             |
+   *         OPEN_OBJECT_MAP (skip if     BLACKLIST (skip if blacklist    |
+   *              |           disabled)     |        disabled)            |
+   *              v                         v                             |
+   *         OPEN_JOURNAL (skip if        BREAK_LOCK                      |
+   *              |   *     disabled)       |                             |
+   *              |   *                     \-----------------------------/
+   *              |   * * * * * * * *
+   *              v                 *
+   *          ALLOCATE_JOURNAL_TAG  *
+   *              |            *    *
+   *              |            *    *
+   *              |            v    v
+   *              |         CLOSE_JOURNAL
+   *              |               |
+   *              |               v
+   *              |         CLOSE_OBJECT_MAP
+   *              |               |
+   *              |               v
+   *              |         UNLOCK_IMAGE
+   *              |               |
+   *              v               |
+   *          <finish> <----------/
    *
    * @endverbatim
    */
@@ -104,6 +106,9 @@ private:
   void send_lock();
   Context *handle_lock(int *ret_val);
 
+  Context *send_refresh();
+  Context *handle_refresh(int *ret_val);
+
   Context *send_open_journal();
   Context *handle_open_journal(int *ret_val);
 

diff --git a/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc b/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc
index e4ba0ec81e71bb9f35ae4a0d595b7e98d38b0d06..a86fbb31aca44cebb6bab8763610dacad206c4d2 100644 (file)
--- a/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc
+++ b/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc
@@ -4,6 +4,7 @@
 #include "test/librbd/test_mock_fixture.h"
 #include "test/librbd/test_support.h"
 #include "test/librbd/mock/MockImageCtx.h"
+#include "test/librbd/mock/MockImageState.h"
 #include "test/librbd/mock/MockJournal.h"
 #include "test/librbd/mock/MockJournalPolicy.h"
 #include "test/librbd/mock/MockObjectMap.h"
@@ -58,6 +59,16 @@ public:
                   .WillOnce(Return(r));
   }
 
+  void expect_is_refresh_required(MockImageCtx &mock_image_ctx, bool required) {
+    EXPECT_CALL(*mock_image_ctx.state, is_refresh_required())
+      .WillOnce(Return(required));
+  }
+
+  void expect_refresh(MockImageCtx &mock_image_ctx, int r) {
+    EXPECT_CALL(*mock_image_ctx.state, refresh(_))
+                  .WillOnce(CompleteContext(r, mock_image_ctx.image_ctx->op_work_queue));
+  }
+
   void expect_create_object_map(MockImageCtx &mock_image_ctx,
                                 MockObjectMap *mock_object_map) {
     EXPECT_CALL(mock_image_ctx, create_object_map(_))
@@ -188,6 +199,7 @@ TEST_F(TestMockExclusiveLockAcquireRequest, Success) {
   InSequence seq;
   expect_flush_notifies(mock_image_ctx);
   expect_lock(mock_image_ctx, 0);
+  expect_is_refresh_required(mock_image_ctx, false);
 
   MockObjectMap mock_object_map;
   expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true);
@@ -212,6 +224,35 @@ TEST_F(TestMockExclusiveLockAcquireRequest, Success) {
   ASSERT_EQ(0, ctx.wait());
 }
 
+TEST_F(TestMockExclusiveLockAcquireRequest, SuccessRefresh) {
+  REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK);
+
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(m_image_name, &ictx));
+
+  MockImageCtx mock_image_ctx(*ictx);
+  expect_op_work_queue(mock_image_ctx);
+
+  InSequence seq;
+  expect_flush_notifies(mock_image_ctx);
+  expect_lock(mock_image_ctx, 0);
+  expect_is_refresh_required(mock_image_ctx, true);
+  expect_refresh(mock_image_ctx, 0);
+
+  MockObjectMap mock_object_map;
+  expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, false);
+  expect_test_features(mock_image_ctx, RBD_FEATURE_JOURNALING, false);
+
+  C_SaferCond acquire_ctx;
+  C_SaferCond ctx;
+  MockAcquireRequest *req = MockAcquireRequest::create(mock_image_ctx,
+                                                       TEST_COOKIE,
+                                                       &acquire_ctx, &ctx);
+  req->send();
+  ASSERT_EQ(0, acquire_ctx.wait());
+  ASSERT_EQ(0, ctx.wait());
+}
+
 TEST_F(TestMockExclusiveLockAcquireRequest, SuccessJournalDisabled) {
   REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK);
 
@@ -224,6 +265,7 @@ TEST_F(TestMockExclusiveLockAcquireRequest, SuccessJournalDisabled) {
   InSequence seq;
   expect_flush_notifies(mock_image_ctx);
   expect_lock(mock_image_ctx, 0);
+  expect_is_refresh_required(mock_image_ctx, false);
 
   MockObjectMap mock_object_map;
   expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true);
@@ -254,6 +296,7 @@ TEST_F(TestMockExclusiveLockAcquireRequest, SuccessObjectMapDisabled) {
   InSequence seq;
   expect_flush_notifies(mock_image_ctx);
   expect_lock(mock_image_ctx, 0);
+  expect_is_refresh_required(mock_image_ctx, false);
 
   expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, false);
 
@@ -275,6 +318,31 @@ TEST_F(TestMockExclusiveLockAcquireRequest, SuccessObjectMapDisabled) {
   ASSERT_EQ(0, ctx.wait());
 }
 
+TEST_F(TestMockExclusiveLockAcquireRequest, RefreshError) {
+  REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK);
+
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(m_image_name, &ictx));
+
+  MockImageCtx mock_image_ctx(*ictx);
+  expect_op_work_queue(mock_image_ctx);
+
+  InSequence seq;
+  expect_flush_notifies(mock_image_ctx);
+  expect_lock(mock_image_ctx, 0);
+  expect_is_refresh_required(mock_image_ctx, true);
+  expect_refresh(mock_image_ctx, -EINVAL);
+  expect_unlock(mock_image_ctx, 0);
+
+  C_SaferCond *acquire_ctx = new C_SaferCond();
+  C_SaferCond ctx;
+  MockAcquireRequest *req = MockAcquireRequest::create(mock_image_ctx,
+                                                       TEST_COOKIE,
+                                                       acquire_ctx, &ctx);
+  req->send();
+  ASSERT_EQ(-EINVAL, ctx.wait());
+}
+
 TEST_F(TestMockExclusiveLockAcquireRequest, JournalError) {
   REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK);
 
@@ -287,6 +355,7 @@ TEST_F(TestMockExclusiveLockAcquireRequest, JournalError) {
   InSequence seq;
   expect_flush_notifies(mock_image_ctx);
   expect_lock(mock_image_ctx, 0);
+  expect_is_refresh_required(mock_image_ctx, false);
 
   MockObjectMap *mock_object_map = new MockObjectMap();
   expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true);
@@ -322,6 +391,7 @@ TEST_F(TestMockExclusiveLockAcquireRequest, AllocateJournalTagError) {
   InSequence seq;
   expect_flush_notifies(mock_image_ctx);
   expect_lock(mock_image_ctx, 0);
+  expect_is_refresh_required(mock_image_ctx, false);
 
   MockObjectMap *mock_object_map = new MockObjectMap();
   expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true);
@@ -665,6 +735,7 @@ TEST_F(TestMockExclusiveLockAcquireRequest, OpenObjectMapError) {
   InSequence seq;
   expect_flush_notifies(mock_image_ctx);
   expect_lock(mock_image_ctx, 0);
+  expect_is_refresh_required(mock_image_ctx, false);
 
   MockObjectMap *mock_object_map = new MockObjectMap();
   expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true);