librbd: acquire exclusive-lock during copy on read

Fixes: http://tracker.ceph.com/issues/18888
Signed-off-by: Venky Shankar <vshankar@redhat.com>
(cherry picked from commit 7dba5311b12011a4a6e8564e68150e54c5af5ddd)

Conflicts:
    src/librbd/AioImageRequestWQ.h:
      - in master this file has morphed into src/librbd/io/ImageRequestWQ.h
      - jewel has AioImageRequest<ImageCtx> instead of ImageRequest<ImageCtx>
    src/librbd/image/RefreshRequest.cc:
      - rename image context element to "aio_work_queue" (from "io_work_queue")
        because jewel doesn't have de95d862f57b56738e04d77f2351622f83f17f4a
    src/test/librbd/image/test_mock_RefreshRequest.cc:
      - rename image context element to "aio_work_queue" (from "io_work_queue")
        because jewel doesn't have de95d862f57b56738e04d77f2351622f83f17f4a

diff --git a/src/librbd/AioImageRequestWQ.h b/src/librbd/AioImageRequestWQ.h
index 42817c0d799cee56b640c219e0e396982f7bcb26..d101378f10e6a45d3d1bb08a7f38556dcc0106eb 100644 (file)
--- a/src/librbd/AioImageRequestWQ.h
+++ b/src/librbd/AioImageRequestWQ.h
@@ -38,6 +38,7 @@ public:
 
   void shut_down(Context *on_shutdown);
 
+  bool is_lock_required() const;
   bool is_lock_request_needed() const;
 
   inline bool writes_blocked() const {
@@ -109,7 +110,6 @@ private:
   int start_in_flight_op(AioCompletion *c);
   void finish_in_flight_op();
 
-  bool is_lock_required() const;
   void queue(AioImageRequest<ImageCtx> *req);
 
   void handle_refreshed(int r, AioImageRequest<ImageCtx> *req);

diff --git a/src/librbd/image/RefreshRequest.cc b/src/librbd/image/RefreshRequest.cc
index 70c43b97aacc3277cd884588f64ff562a66ec5c7..3fcedeab33a90c5ca2d41c954d5cd294e6679505 100644 (file)
--- a/src/librbd/image/RefreshRequest.cc
+++ b/src/librbd/image/RefreshRequest.cc
@@ -929,6 +929,7 @@ void RefreshRequest<I>::apply() {
       // object map and journaling
       assert(m_exclusive_lock == nullptr);
       m_exclusive_lock = m_image_ctx.exclusive_lock;
+      m_image_ctx.aio_work_queue->clear_require_lock_on_read();
     } else {
       if (m_exclusive_lock != nullptr) {
         assert(m_image_ctx.exclusive_lock == nullptr);
@@ -948,6 +949,10 @@ void RefreshRequest<I>::apply() {
           m_object_map != nullptr) {
         std::swap(m_object_map, m_image_ctx.object_map);
       }
+      if (m_image_ctx.clone_copy_on_read &&
+          m_image_ctx.aio_work_queue->is_lock_required()) {
+        m_image_ctx.aio_work_queue->set_require_lock_on_read();
+      }
     }
   }
 }

diff --git a/src/test/librbd/image/test_mock_RefreshRequest.cc b/src/test/librbd/image/test_mock_RefreshRequest.cc
index 6b0ff52ab53418495debb60d3955fb6c335b750f..93dc2a5fa0d227b635830050edfe24b3ed240163 100644 (file)
--- a/src/test/librbd/image/test_mock_RefreshRequest.cc
+++ b/src/test/librbd/image/test_mock_RefreshRequest.cc
@@ -97,6 +97,10 @@ public:
   typedef RefreshRequest<MockRefreshImageCtx> MockRefreshRequest;
   typedef RefreshParentRequest<MockRefreshImageCtx> MockRefreshParentRequest;
 
+  void expect_is_lock_required(MockRefreshImageCtx &mock_image_ctx, bool require_lock) {
+    EXPECT_CALL(*mock_image_ctx.aio_work_queue, is_lock_required()).WillOnce(Return(require_lock));
+  }
+
   void expect_set_require_lock_on_read(MockRefreshImageCtx &mock_image_ctx) {
     EXPECT_CALL(*mock_image_ctx.aio_work_queue, set_require_lock_on_read());
   }
@@ -586,6 +590,7 @@ TEST_F(TestMockImageRefreshRequest, DisableExclusiveLock) {
   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);
+  expect_clear_require_lock_on_read(mock_image_ctx);
   expect_shut_down_exclusive_lock(mock_image_ctx, *mock_exclusive_lock, 0);
 
   C_SaferCond ctx;
@@ -682,6 +687,58 @@ TEST_F(TestMockImageRefreshRequest, JournalDisabledByPolicy) {
   ASSERT_EQ(0, ctx.wait());
 }
 
+TEST_F(TestMockImageRefreshRequest, ExclusiveLockWithCoR) {
+  REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK);
+
+  std::string val;
+  ASSERT_EQ(0, _rados.conf_get("rbd_clone_copy_on_read", val));
+  if (val == "false") {
+    std::cout << "SKIPPING due to disabled rbd_copy_on_read" << std::endl;
+    return;
+  }
+
+  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;
+
+  if (ictx->test_features(RBD_FEATURE_JOURNALING)) {
+    ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_JOURNALING,
+                                                   false));
+  }
+
+  if (ictx->test_features(RBD_FEATURE_FAST_DIFF)) {
+    ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_FAST_DIFF,
+                                                   false));
+  }
+
+  if (ictx->test_features(RBD_FEATURE_OBJECT_MAP)) {
+    ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_OBJECT_MAP,
+                                                   false));
+  }
+
+  expect_op_work_queue(mock_image_ctx);
+  expect_test_features(mock_image_ctx);
+
+  InSequence seq;
+  expect_get_mutable_metadata(mock_image_ctx, 0);
+  expect_get_flags(mock_image_ctx, 0);
+  expect_get_group(mock_image_ctx, 0);
+  expect_refresh_parent_is_required(mock_refresh_parent_request, false);
+  expect_is_lock_required(mock_image_ctx, true);
+  expect_set_require_lock_on_read(mock_image_ctx);
+
+  C_SaferCond ctx;
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, false, &ctx);
+  req->send();
+
+  ASSERT_EQ(0, ctx.wait());
+}
+
 TEST_F(TestMockImageRefreshRequest, EnableJournalWithExclusiveLock) {
   REQUIRE_FEATURE(RBD_FEATURE_JOURNALING);
 

diff --git a/src/test/librbd/mock/MockAioImageRequestWQ.h b/src/test/librbd/mock/MockAioImageRequestWQ.h
index 77419c04d8f4615405860095270abfdb7b5c4759..2b1cefee405fba1024f2c546907116feda89ad60 100644 (file)
--- a/src/test/librbd/mock/MockAioImageRequestWQ.h
+++ b/src/test/librbd/mock/MockAioImageRequestWQ.h
@@ -17,6 +17,7 @@ struct MockAioImageRequestWQ {
   MOCK_METHOD0(set_require_lock_on_read, void());
   MOCK_METHOD0(clear_require_lock_on_read, void());
 
+  MOCK_CONST_METHOD0(is_lock_required, bool());
   MOCK_CONST_METHOD0(is_lock_request_needed, bool());
 };
 

diff --git a/src/test/librbd/mock/MockImageCtx.h b/src/test/librbd/mock/MockImageCtx.h
index 040ab5701dfa167ce2920ed2e6f8f61d6ed963da..a4c7e5980ca9b055a24d8d79e6648c4f10fd6900 100644 (file)
--- a/src/test/librbd/mock/MockImageCtx.h
+++ b/src/test/librbd/mock/MockImageCtx.h
@@ -51,6 +51,7 @@ struct MockImageCtx {
       object_set(image_ctx.object_set),
       old_format(image_ctx.old_format),
       read_only(image_ctx.read_only),
+      clone_copy_on_read(image_ctx.clone_copy_on_read),
       lockers(image_ctx.lockers),
       exclusive_locked(image_ctx.exclusive_locked),
       lock_tag(image_ctx.lock_tag),
@@ -201,6 +202,8 @@ struct MockImageCtx {
   bool old_format;
   bool read_only;
 
+  bool clone_copy_on_read;
+
   std::map<rados::cls::lock::locker_id_t,
            rados::cls::lock::locker_info_t> lockers;
   bool exclusive_locked;