librbd: copyup should restart delayed ops against the same object

This avoids the potential for a race condition where an in-flight
copyup is removed from the in-flight copyup list and a subsequent
IO against the same object causes a second in-flight copyup.

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

diff --git a/src/librbd/io/CopyupRequest.cc b/src/librbd/io/CopyupRequest.cc
index d96b3a344920fa4c705949b7c4cedc52bad96050..4eeee3d4d5d7bbf3500f660ebac5bad9a0f6a918 100644 (file)
--- a/src/librbd/io/CopyupRequest.cc
+++ b/src/librbd/io/CopyupRequest.cc
@@ -119,11 +119,16 @@ CopyupRequest<I>::~CopyupRequest() {
 
 template <typename I>
 void CopyupRequest<I>::append_request(AbstractObjectWriteRequest<I> *req) {
-  ceph_assert(m_ictx->copyup_list_lock.is_locked());
+  Mutex::Locker locker(m_lock);
 
   ldout(m_ictx->cct, 20) << "oid=" << m_oid << ", "
-                         << "object_request=" << req << dendl;
-  m_pending_requests.push_back(req);
+                         << "object_request=" << req << ", "
+                         << "append=" << m_append_request_permitted << dendl;
+  if (m_append_request_permitted) {
+    m_pending_requests.push_back(req);
+  } else {
+    m_restart_requests.push_back(req);
+  }
 }
 
 template <typename I>
@@ -173,12 +178,12 @@ void CopyupRequest<I>::handle_read_from_parent(int r) {
   ldout(m_ictx->cct, 20) << "oid=" << m_oid << ", "
                          << "r=" << r << dendl;
 
-  m_ictx->copyup_list_lock.Lock();
+  m_lock.Lock();
   m_copyup_required = is_copyup_required();
-  remove_from_list();
+  disable_append_requests();
 
   if (r < 0 && r != -ENOENT) {
-    m_ictx->copyup_list_lock.Unlock();
+    m_lock.Unlock();
 
     lderr(m_ictx->cct) << "error reading from parent: " << cpp_strerror(r)
                        << dendl;
@@ -187,13 +192,13 @@ void CopyupRequest<I>::handle_read_from_parent(int r) {
   }
 
   if (!m_copyup_required) {
-    m_ictx->copyup_list_lock.Unlock();
+    m_lock.Unlock();
 
     ldout(m_ictx->cct, 20) << "no-op, skipping" << dendl;
     finish(0);
     return;
   }
-  m_ictx->copyup_list_lock.Unlock();
+  m_lock.Unlock();
 
   update_object_maps();
 }
@@ -204,9 +209,9 @@ void CopyupRequest<I>::deep_copy() {
   ceph_assert(m_ictx->parent_lock.is_locked());
   ceph_assert(m_ictx->parent != nullptr);
 
-  m_ictx->copyup_list_lock.Lock();
+  m_lock.Lock();
   m_flatten = is_copyup_required() ? true : m_ictx->migration_info.flatten;
-  m_ictx->copyup_list_lock.Unlock();
+  m_lock.Unlock();
 
   ldout(m_ictx->cct, 20) << "oid=" << m_oid << ", "
                          << "flatten=" << m_flatten << dendl;
@@ -229,10 +234,10 @@ void CopyupRequest<I>::handle_deep_copy(int r) {
                          << "r=" << r << dendl;
 
   m_ictx->snap_lock.get_read();
-  m_ictx->copyup_list_lock.Lock();
+  m_lock.Lock();
   m_copyup_required = is_copyup_required();
   if (r == -ENOENT && !m_flatten && m_copyup_required) {
-    m_ictx->copyup_list_lock.Unlock();
+    m_lock.Unlock();
     m_ictx->snap_lock.put_read();
 
     ldout(m_ictx->cct, 10) << "restart deep-copy with flatten" << dendl;
@@ -240,10 +245,10 @@ void CopyupRequest<I>::handle_deep_copy(int r) {
     return;
   }
 
-  remove_from_list();
+  disable_append_requests();
 
   if (r < 0 && r != -ENOENT) {
-    m_ictx->copyup_list_lock.Unlock();
+    m_lock.Unlock();
     m_ictx->snap_lock.put_read();
 
     lderr(m_ictx->cct) << "error encountered during deep-copy: "
@@ -253,7 +258,7 @@ void CopyupRequest<I>::handle_deep_copy(int r) {
   }
 
   if (!m_copyup_required && !is_update_object_map_required(r)) {
-    m_ictx->copyup_list_lock.Unlock();
+    m_lock.Unlock();
     m_ictx->snap_lock.put_read();
 
     if (r == -ENOENT) {
@@ -265,7 +270,7 @@ void CopyupRequest<I>::handle_deep_copy(int r) {
     return;
   }
 
-  m_ictx->copyup_list_lock.Unlock();
+  m_lock.Unlock();
   m_ictx->snap_lock.put_read();
 
   update_object_maps();
@@ -348,15 +353,15 @@ void CopyupRequest<I>::handle_update_object_maps(int r) {
 
 template <typename I>
 void CopyupRequest<I>::copyup() {
-  m_ictx->copyup_list_lock.Lock();
+  m_lock.Lock();
   if (!m_copyup_required) {
-    m_ictx->copyup_list_lock.Unlock();
+    m_lock.Unlock();
 
     ldout(m_ictx->cct, 20) << "skipping copyup" << dendl;
     finish(0);
     return;
   }
-  m_ictx->copyup_list_lock.Unlock();
+  m_lock.Unlock();
 
   ldout(m_ictx->cct, 20) << "oid=" << m_oid << dendl;
 
@@ -453,7 +458,7 @@ void CopyupRequest<I>::handle_copyup(int r) {
   } else if (r < 0) {
     lderr(m_ictx->cct) << "failed to copyup object: "
                        << cpp_strerror(r) << dendl;
-    complete_requests(r);
+    complete_requests(false, r);
   }
 
   if (pending_copyups == 0) {
@@ -466,13 +471,14 @@ void CopyupRequest<I>::finish(int r) {
   ldout(m_ictx->cct, 20) << "oid=" << m_oid << ", "
                          << "r=" << r << dendl;
 
-  complete_requests(r);
+  complete_requests(true, r);
   delete this;
 }
 
 template <typename I>
-void CopyupRequest<I>::complete_requests(int r) {
-  // already removed from copyup list
+void CopyupRequest<I>::complete_requests(bool override_restart_retval, int r) {
+  remove_from_list();
+
   while (!m_pending_requests.empty()) {
     auto it = m_pending_requests.begin();
     auto req = *it;
@@ -480,20 +486,39 @@ void CopyupRequest<I>::complete_requests(int r) {
     req->handle_copyup(r);
     m_pending_requests.erase(it);
   }
+
+  if (override_restart_retval) {
+    r = -ERESTART;
+  }
+
+  while (!m_restart_requests.empty()) {
+    auto it = m_restart_requests.begin();
+    auto req = *it;
+    ldout(m_ictx->cct, 20) << "restarting request " << req << dendl;
+    req->handle_copyup(r);
+    m_restart_requests.erase(it);
+  }
+}
+
+template <typename I>
+void CopyupRequest<I>::disable_append_requests() {
+  ceph_assert(m_lock.is_locked());
+  m_append_request_permitted = false;
 }
 
 template <typename I>
 void CopyupRequest<I>::remove_from_list() {
-  assert(m_ictx->copyup_list_lock.is_locked());
+  Mutex::Locker copyup_list_locker(m_ictx->copyup_list_lock);
 
   auto it = m_ictx->copyup_list.find(m_object_no);
-  ceph_assert(it != m_ictx->copyup_list.end());
-  m_ictx->copyup_list.erase(it);
+  if (it != m_ictx->copyup_list.end()) {
+    m_ictx->copyup_list.erase(it);
+  }
 }
 
 template <typename I>
 bool CopyupRequest<I>::is_copyup_required() {
-  ceph_assert(m_ictx->copyup_list_lock.is_locked());
+  ceph_assert(m_lock.is_locked());
 
   bool copy_on_read = m_pending_requests.empty();
   if (copy_on_read) {

diff --git a/src/librbd/io/CopyupRequest.h b/src/librbd/io/CopyupRequest.h
index 24d5323849171a8b0a7f8a003c9340e8b8c9de65..5043ada66c5aff6c67b2ac87e1584f4c04193238 100644 (file)
--- a/src/librbd/io/CopyupRequest.h
+++ b/src/librbd/io/CopyupRequest.h
@@ -76,7 +76,7 @@ private:
    * no data was read from the parent *and* there are no additional ops.
    */
 
-  typedef std::vector<AbstractObjectWriteRequest<ImageCtxT> *> PendingRequests;
+  typedef std::vector<AbstractObjectWriteRequest<ImageCtxT> *> WriteRequests;
 
   ImageCtx *m_ictx;
   std::string m_oid;
@@ -89,14 +89,17 @@ private:
   bool m_copyup_required = true;
 
   ceph::bufferlist m_copyup_data;
-  PendingRequests m_pending_requests;
-  unsigned m_pending_copyups = 0;
 
   AsyncOperation m_async_op;
 
   std::vector<uint64_t> m_snap_ids;
 
   Mutex m_lock;
+  WriteRequests m_pending_requests;
+  unsigned m_pending_copyups = 0;
+
+  WriteRequests m_restart_requests;
+  bool m_append_request_permitted = true;
 
   void read_from_parent();
   void handle_read_from_parent(int r);
@@ -111,8 +114,9 @@ private:
   void handle_copyup(int r);
 
   void finish(int r);
-  void complete_requests(int r);
+  void complete_requests(bool override_restart_retval, int r);
 
+  void disable_append_requests();
   void remove_from_list();
 
   bool is_copyup_required();

diff --git a/src/librbd/io/ObjectRequest.cc b/src/librbd/io/ObjectRequest.cc
index 61cd787c6cd1cb30507f93d91422b0e1c6d1b3e1..2d45173a7022061e995ce10c3296a09d3ef10f95 100644 (file)
--- a/src/librbd/io/ObjectRequest.cc
+++ b/src/librbd/io/ObjectRequest.cc
@@ -604,14 +604,14 @@ void AbstractObjectWriteRequest<I>::handle_copyup(int r) {
   ceph_assert(m_copyup_in_progress);
   m_copyup_in_progress = false;
 
-  if (r < 0) {
+  if (r < 0 && r != -ERESTART) {
     lderr(image_ctx->cct) << "failed to copyup object: " << cpp_strerror(r)
                           << dendl;
     this->finish(r);
     return;
   }
 
-  if (is_post_copyup_write_required()) {
+  if (r == -ERESTART || is_post_copyup_write_required()) {
     write_object();
     return;
   }

diff --git a/src/test/librbd/io/test_mock_ObjectRequest.cc b/src/test/librbd/io/test_mock_ObjectRequest.cc
index 7cf6271907bb1f1fe4dfa724443afb6b88ebc8d8..2f1d8777e0b9f6a9cc288a464e5a3799accbec9e 100644 (file)
--- a/src/test/librbd/io/test_mock_ObjectRequest.cc
+++ b/src/test/librbd/io/test_mock_ObjectRequest.cc
@@ -723,6 +723,62 @@ TEST_F(TestMockIoObjectRequest, Copyup) {
   ASSERT_EQ(0, ctx.wait());
 }
 
+TEST_F(TestMockIoObjectRequest, CopyupRestart) {
+  REQUIRE_FEATURE(RBD_FEATURE_LAYERING);
+
+  librbd::Image image;
+  librbd::RBD rbd;
+  ASSERT_EQ(0, rbd.open(m_ioctx, image, m_image_name.c_str(), NULL));
+  ASSERT_EQ(0, image.snap_create("one"));
+  ASSERT_EQ(0, image.snap_protect("one"));
+  image.close();
+
+  std::string clone_name = get_temp_image_name();
+  int order = 0;
+  ASSERT_EQ(0, rbd.clone(m_ioctx, m_image_name.c_str(), "one", m_ioctx,
+                         clone_name.c_str(), RBD_FEATURE_LAYERING, &order));
+
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(clone_name, &ictx));
+
+  MockTestImageCtx mock_image_ctx(*ictx);
+  expect_get_object_size(mock_image_ctx);
+
+  MockExclusiveLock mock_exclusive_lock;
+  if (ictx->test_features(RBD_FEATURE_EXCLUSIVE_LOCK)) {
+    mock_image_ctx.exclusive_lock = &mock_exclusive_lock;
+    expect_is_lock_owner(mock_exclusive_lock);
+  }
+
+  MockObjectMap mock_object_map;
+  if (ictx->test_features(RBD_FEATURE_OBJECT_MAP)) {
+    mock_image_ctx.object_map = &mock_object_map;
+  }
+
+  bufferlist bl;
+  bl.append(std::string(4096, '1'));
+
+  InSequence seq;
+  expect_get_parent_overlap(mock_image_ctx, CEPH_NOSNAP, 4096, 0);
+  expect_prune_parent_extents(mock_image_ctx, {{0, 4096}}, 4096, 4096);
+  expect_object_may_exist(mock_image_ctx, 0, true);
+  expect_object_map_update(mock_image_ctx, 0, 1, OBJECT_EXISTS, {}, false, 0);
+  expect_assert_exists(mock_image_ctx, -ENOENT);
+
+  MockAbstractObjectWriteRequest *write_request = nullptr;
+  MockCopyupRequest mock_copyup_request;
+  expect_copyup(mock_copyup_request, &write_request, -ERESTART);
+  expect_assert_exists(mock_image_ctx, 0);
+  expect_write(mock_image_ctx, 0, 4096, 0);
+
+  C_SaferCond ctx;
+  auto req = MockObjectWriteRequest::create_write(
+    &mock_image_ctx, ictx->get_object_name(0), 0, 0, std::move(bl),
+    mock_image_ctx.snapc, 0, {}, &ctx);
+  req->send();
+  ASSERT_EQ(0, ctx.wait());
+}
+
 TEST_F(TestMockIoObjectRequest, CopyupOptimization) {
   REQUIRE_FEATURE(RBD_FEATURE_LAYERING | RBD_FEATURE_OBJECT_MAP);