From: Jason Dillaman Date: Thu, 4 Apr 2019 01:35:40 +0000 (-0400) Subject: librbd: copyup should restart delayed ops against the same object X-Git-Tag: v15.1.0~2975^2~3 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=31ed673a7484c8067dd928cabd5bcb923a35dbaf;p=ceph-ci.git 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 --- diff --git a/src/librbd/io/CopyupRequest.cc b/src/librbd/io/CopyupRequest.cc index d96b3a34492..4eeee3d4d5d 100644 --- a/src/librbd/io/CopyupRequest.cc +++ b/src/librbd/io/CopyupRequest.cc @@ -119,11 +119,16 @@ CopyupRequest::~CopyupRequest() { template void CopyupRequest::append_request(AbstractObjectWriteRequest *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 @@ -173,12 +178,12 @@ void CopyupRequest::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::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::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::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::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::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::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::handle_update_object_maps(int r) { template void CopyupRequest::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::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::finish(int r) { ldout(m_ictx->cct, 20) << "oid=" << m_oid << ", " << "r=" << r << dendl; - complete_requests(r); + complete_requests(true, r); delete this; } template -void CopyupRequest::complete_requests(int r) { - // already removed from copyup list +void CopyupRequest::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::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 +void CopyupRequest::disable_append_requests() { + ceph_assert(m_lock.is_locked()); + m_append_request_permitted = false; } template void CopyupRequest::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 bool CopyupRequest::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 24d53238491..5043ada66c5 100644 --- 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 *> PendingRequests; + typedef std::vector *> 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 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 61cd787c6cd..2d45173a702 100644 --- a/src/librbd/io/ObjectRequest.cc +++ b/src/librbd/io/ObjectRequest.cc @@ -604,14 +604,14 @@ void AbstractObjectWriteRequest::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 7cf6271907b..2f1d8777e0b 100644 --- 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);