From: Jason Dillaman Date: Fri, 29 Mar 2019 17:23:37 +0000 (-0400) Subject: librbd: properly hold snap/parent locks during IO X-Git-Tag: v14.2.2~183^2~7 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=fec9d4d723aea113ee7445b1be7b264948f9b0cc;p=ceph.git librbd: properly hold snap/parent locks during IO The ImageCtx::parent pointer was dereferenced without holding the lock which could lead to a crash. The ImageCtx::migration_info structure was also dereferenced without holding a lock. Signed-off-by: Jason Dillaman (cherry picked from commit 6dad1acee3f0a4b4b3c88e25c0cfafe21bc066c4) --- diff --git a/src/librbd/io/CopyupRequest.cc b/src/librbd/io/CopyupRequest.cc index 334f0ec41623f..dc764a1d7fbbc 100644 --- a/src/librbd/io/CopyupRequest.cc +++ b/src/librbd/io/CopyupRequest.cc @@ -6,7 +6,7 @@ #include "common/dout.h" #include "common/errno.h" #include "common/Mutex.h" - +#include "common/WorkQueue.h" #include "librbd/AsyncObjectThrottle.h" #include "librbd/ExclusiveLock.h" #include "librbd/ImageCtx.h" @@ -101,12 +101,15 @@ CopyupRequest::~CopyupRequest() { template void CopyupRequest::append_request(AbstractObjectWriteRequest *req) { + ceph_assert(m_ictx->copyup_list_lock.is_locked()); + ldout(m_ictx->cct, 20) << req << dendl; m_pending_requests.push_back(req); } template void CopyupRequest::complete_requests(int r) { + // already removed from copyup list while (!m_pending_requests.empty()) { auto it = m_pending_requests.begin(); auto req = *it; @@ -235,6 +238,8 @@ bool CopyupRequest::is_update_object_map_required(int r) { template bool CopyupRequest::is_deep_copy() const { + ceph_assert(m_ictx->snap_lock.is_locked()); + return !m_ictx->migration_info.empty(); } @@ -243,20 +248,40 @@ void CopyupRequest::send() { m_state = STATE_READ_FROM_PARENT; + m_ictx->snap_lock.get_read(); + m_ictx->parent_lock.get_read(); + if (m_ictx->parent == nullptr) { + ldout(m_ictx->cct, 5) << "parent detached" << dendl; + m_ictx->parent_lock.put_read(); + m_ictx->snap_lock.put_read(); + + m_ictx->op_work_queue->queue(util::create_context_callback(this), -ENOENT); + return; + } + if (is_deep_copy()) { + m_ictx->copyup_list_lock.Lock(); m_flatten = is_copyup_required() ? true : m_ictx->migration_info.flatten; + m_ictx->copyup_list_lock.Unlock(); + + m_deep_copy = true; auto req = deep_copy::ObjectCopyRequest::create( - m_ictx->parent, m_ictx, m_ictx->migration_info.snap_map, m_object_no, - m_flatten, util::create_context_callback(this)); + m_ictx->parent, m_ictx, m_ictx->migration_info.snap_map, m_object_no, + m_flatten, util::create_context_callback(this)); + ldout(m_ictx->cct, 20) << "deep copy object req " << req << ", object_no " << m_object_no << ", flatten " << m_flatten << dendl; req->send(); + + m_ictx->parent_lock.put_read(); + m_ictx->snap_lock.put_read(); return; } - AioCompletion *comp = AioCompletion::create_and_start( + m_deep_copy = false; + auto comp = AioCompletion::create_and_start( this, m_ictx, AIO_TYPE_READ); ldout(m_ictx->cct, 20) << "completion " << comp @@ -265,6 +290,9 @@ void CopyupRequest::send() << dendl; ImageRequest<>::aio_read(m_ictx->parent, comp, std::move(m_image_extents), ReadResult{&m_copyup_data}, 0, m_trace); + + m_ictx->parent_lock.put_read(); + m_ictx->snap_lock.put_read(); } template @@ -286,24 +314,27 @@ bool CopyupRequest::should_complete(int *r) { switch (m_state) { case STATE_READ_FROM_PARENT: ldout(cct, 20) << "READ_FROM_PARENT" << dendl; + m_ictx->copyup_list_lock.Lock(); - if (*r == -ENOENT && is_deep_copy() && !m_flatten && is_copyup_required()) { + if (*r == -ENOENT && m_deep_copy && !m_flatten && is_copyup_required()) { ldout(cct, 5) << "restart deep copy with flatten" << dendl; m_ictx->copyup_list_lock.Unlock(); + send(); return false; } + remove_from_list(m_ictx->copyup_list_lock); m_ictx->copyup_list_lock.Unlock(); + if (*r >= 0 || *r == -ENOENT) { if (!is_copyup_required() && !is_update_object_map_required(*r)) { - if (*r == -ENOENT && is_deep_copy()) { + if (*r == -ENOENT && m_deep_copy) { *r = 0; } ldout(cct, 20) << "skipping" << dendl; return true; } - return send_object_map_head(); } break; diff --git a/src/librbd/io/CopyupRequest.h b/src/librbd/io/CopyupRequest.h index d091e8775293b..0926f904d6697 100644 --- a/src/librbd/io/CopyupRequest.h +++ b/src/librbd/io/CopyupRequest.h @@ -92,7 +92,8 @@ private: ZTracer::Trace m_trace; State m_state; - bool m_flatten; + bool m_deep_copy = false; + bool m_flatten = false; ceph::bufferlist m_copyup_data; std::vector *> m_pending_requests; unsigned m_pending_copyups = 0; diff --git a/src/librbd/io/ObjectRequest.cc b/src/librbd/io/ObjectRequest.cc index 611e83677a6f5..61cd787c6cd1c 100644 --- a/src/librbd/io/ObjectRequest.cc +++ b/src/librbd/io/ObjectRequest.cc @@ -266,38 +266,42 @@ template void ObjectReadRequest::read_parent() { I *image_ctx = this->m_ictx; - uint64_t object_overlap = 0; + image_ctx->snap_lock.get_read(); + image_ctx->parent_lock.get_read(); + + // calculate reverse mapping onto the image Extents parent_extents; - { - RWLock::RLocker snap_locker(image_ctx->snap_lock); - RWLock::RLocker parent_locker(image_ctx->parent_lock); + Striper::extent_to_file(image_ctx->cct, &image_ctx->layout, + this->m_object_no, this->m_object_off, + this->m_object_len, parent_extents); - // calculate reverse mapping onto the image - Striper::extent_to_file(image_ctx->cct, &image_ctx->layout, - this->m_object_no, this->m_object_off, - this->m_object_len, parent_extents); - - uint64_t parent_overlap = 0; - int r = image_ctx->get_parent_overlap(this->m_snap_id, &parent_overlap); - if (r == 0) { - object_overlap = image_ctx->prune_parent_extents(parent_extents, - parent_overlap); - } + uint64_t parent_overlap = 0; + uint64_t object_overlap = 0; + int r = image_ctx->get_parent_overlap(this->m_snap_id, &parent_overlap); + if (r == 0) { + object_overlap = image_ctx->prune_parent_extents(parent_extents, + parent_overlap); } if (object_overlap == 0) { + image_ctx->parent_lock.put_read(); + image_ctx->snap_lock.put_read(); + this->finish(-ENOENT); return; } ldout(image_ctx->cct, 20) << dendl; - AioCompletion *parent_completion = AioCompletion::create_and_start< + auto parent_completion = AioCompletion::create_and_start< ObjectReadRequest, &ObjectReadRequest::handle_read_parent>( this, util::get_image_ctx(image_ctx->parent), AIO_TYPE_READ); ImageRequest::aio_read(image_ctx->parent, parent_completion, std::move(parent_extents), ReadResult{m_read_data}, 0, this->m_trace); + + image_ctx->parent_lock.put_read(); + image_ctx->snap_lock.put_read(); } template @@ -384,6 +388,12 @@ AbstractObjectWriteRequest::AbstractObjectWriteRequest( } compute_parent_info(); + + ictx->snap_lock.get_read(); + if (!ictx->migration_info.empty()) { + m_guarding_migration_write = true; + } + ictx->snap_lock.put_read(); } template @@ -498,8 +508,7 @@ void AbstractObjectWriteRequest::write_object() { librados::ObjectWriteOperation write; if (m_copyup_enabled) { ldout(image_ctx->cct, 20) << "guarding write" << dendl; - if (!image_ctx->migration_info.empty()) { - m_guarding_migration_write = true; + if (m_guarding_migration_write) { cls_client::assert_snapc_seq( &write, m_snap_seq, cls::rbd::ASSERT_SNAPC_SEQ_LE_SNAPSET_SEQ); } else { @@ -533,11 +542,14 @@ void AbstractObjectWriteRequest::handle_write_object(int r) { return; } } else if (r == -ERANGE && m_guarding_migration_write) { - if (!image_ctx->migration_info.empty()) { + image_ctx->snap_lock.get_read(); + m_guarding_migration_write = !image_ctx->migration_info.empty(); + image_ctx->snap_lock.put_read(); + + if (m_guarding_migration_write) { copyup(); } else { ldout(image_ctx->cct, 10) << "migration parent gone, restart io" << dendl; - m_guarding_migration_write = false; compute_parent_info(); write_object(); } diff --git a/src/test/librbd/io/test_mock_ObjectRequest.cc b/src/test/librbd/io/test_mock_ObjectRequest.cc index 98cd102629bd2..7cf6271907bb1 100644 --- a/src/test/librbd/io/test_mock_ObjectRequest.cc +++ b/src/test/librbd/io/test_mock_ObjectRequest.cc @@ -180,13 +180,13 @@ struct TestMockIoObjectRequest : public TestMockFixture { } } - void expect_aio_read(MockImageRequest& mock_image_request, + void expect_aio_read(MockTestImageCtx &mock_image_ctx, + MockImageRequest& mock_image_request, Extents&& extents, int r) { EXPECT_CALL(mock_image_request, aio_read(_, extents)) - .WillOnce(WithArg<0>(Invoke([r](AioCompletion* aio_comp) { + .WillOnce(WithArg<0>(Invoke([&mock_image_ctx, r](AioCompletion* aio_comp) { aio_comp->set_request_count(1); - aio_comp->add_request(); - aio_comp->complete_request(r); + mock_image_ctx.image_ctx->op_work_queue->queue(new C_AioRequest(aio_comp), r); }))); } @@ -430,7 +430,7 @@ TEST_F(TestMockIoObjectRequest, ParentRead) { MockImageRequest mock_image_request; expect_get_parent_overlap(mock_image_ctx, CEPH_NOSNAP, 4096, 0); expect_prune_parent_extents(mock_image_ctx, {{0, 4096}}, 4096, 4096); - expect_aio_read(mock_image_request, {{0, 4096}}, 0); + expect_aio_read(mock_image_ctx, mock_image_request, {{0, 4096}}, 0); bufferlist bl; ExtentMap extent_map; @@ -478,7 +478,7 @@ TEST_F(TestMockIoObjectRequest, ParentReadError) { MockImageRequest mock_image_request; expect_get_parent_overlap(mock_image_ctx, CEPH_NOSNAP, 4096, 0); expect_prune_parent_extents(mock_image_ctx, {{0, 4096}}, 4096, 4096); - expect_aio_read(mock_image_request, {{0, 4096}}, -EPERM); + expect_aio_read(mock_image_ctx, mock_image_request, {{0, 4096}}, -EPERM); bufferlist bl; ExtentMap extent_map; @@ -526,7 +526,7 @@ TEST_F(TestMockIoObjectRequest, CopyOnRead) { MockImageRequest mock_image_request; expect_get_parent_overlap(mock_image_ctx, CEPH_NOSNAP, 4096, 0); expect_prune_parent_extents(mock_image_ctx, {{0, 4096}}, 4096, 4096); - expect_aio_read(mock_image_request, {{0, 4096}}, 0); + expect_aio_read(mock_image_ctx, mock_image_request, {{0, 4096}}, 0); MockCopyupRequest mock_copyup_request; expect_get_parent_overlap(mock_image_ctx, CEPH_NOSNAP, 4096, 0);