From 9edc30366b04801590bf73b60367dce06daba67d Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Tue, 27 Oct 2020 14:57:54 -0400 Subject: [PATCH] librbd: replace direct reads from ImageRequest with ImageDispatchSpec Read requests will always need to go via the image dispatch layer to ensure migrations are properly handled. Signed-off-by: Jason Dillaman --- src/librbd/internal.cc | 23 ++-- src/librbd/io/CopyupRequest.cc | 8 +- src/librbd/io/Utils.cc | 10 +- src/test/librbd/io/test_mock_CopyupRequest.cc | 106 +++++++----------- 4 files changed, 60 insertions(+), 87 deletions(-) diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc index 50a3a0f38d3..1de2e14e9ad 100644 --- a/src/librbd/internal.cc +++ b/src/librbd/internal.cc @@ -45,7 +45,7 @@ #include "librbd/image/GetMetadataRequest.h" #include "librbd/image/Types.h" #include "librbd/io/AioCompletion.h" -#include "librbd/io/ImageRequest.h" +#include "librbd/io/ImageDispatchSpec.h" #include "librbd/io/ImageDispatcherInterface.h" #include "librbd/io/ObjectDispatcherInterface.h" #include "librbd/io/ObjectRequest.h" @@ -1295,7 +1295,6 @@ int validate_pool(IoCtx &io_ctx, CephContext *cct) { trace.init("copy", &src->trace_endpoint); } - std::shared_lock owner_lock{src->owner_lock}; SimpleThrottle throttle(src->config.get_val("rbd_concurrent_management_ops"), false); uint64_t period = src->get_stripe_period(); unsigned fadvise_flags = LIBRADOS_OP_FLAG_FADVISE_SEQUENTIAL | @@ -1330,13 +1329,14 @@ int validate_pool(IoCtx &io_ctx, CephContext *cct) { auto ctx = new C_CopyRead(&throttle, dest, offset, bl, sparse_size); auto comp = io::AioCompletion::create_and_start( ctx, src, io::AIO_TYPE_READ); + auto req = io::ImageDispatchSpec::create_read( + *src, io::IMAGE_DISPATCH_LAYER_NONE, comp, + {{offset, len}}, io::ReadResult{bl}, + src->get_data_io_context(), fadvise_flags, 0, trace); - io::ImageReadRequest<> req(*src, comp, {{offset, len}}, - io::ReadResult{bl}, src->get_data_io_context(), - fadvise_flags, 0, std::move(trace)); - ctx->read_trace = req.get_trace(); + ctx->read_trace = trace; + req->send(); - req.send(); prog_ctx.update_progress(offset, src_size); } @@ -1542,10 +1542,11 @@ int validate_pool(IoCtx &io_ctx, CephContext *cct) { C_SaferCond ctx; auto c = io::AioCompletion::create_and_start(&ctx, ictx, io::AIO_TYPE_READ); - io::ImageRequest<>::aio_read(ictx, c, {{off, read_len}}, - io::ReadResult{&bl}, - ictx->get_data_io_context(), 0, 0, - std::move(trace)); + auto req = io::ImageDispatchSpec::create_read( + *ictx, io::IMAGE_DISPATCH_LAYER_NONE, c, + {{off, read_len}}, io::ReadResult{&bl}, + ictx->get_data_io_context(), 0, 0, trace); + req->send(); int ret = ctx.wait(); if (ret < 0) { diff --git a/src/librbd/io/CopyupRequest.cc b/src/librbd/io/CopyupRequest.cc index f4cadd3c394..93effcb1cbb 100644 --- a/src/librbd/io/CopyupRequest.cc +++ b/src/librbd/io/CopyupRequest.cc @@ -17,7 +17,7 @@ #include "librbd/asio/Utils.h" #include "librbd/deep_copy/ObjectCopyRequest.h" #include "librbd/io/AioCompletion.h" -#include "librbd/io/ImageRequest.h" +#include "librbd/io/ImageDispatchSpec.h" #include "librbd/io/ObjectDispatcherInterface.h" #include "librbd/io/ObjectRequest.h" #include "librbd/io/ReadResult.h" @@ -180,10 +180,12 @@ void CopyupRequest::read_from_parent() { ldout(cct, 20) << "completion=" << comp << ", " << "extents=" << m_image_extents << dendl; - ImageRequest::aio_read( - m_image_ctx->parent, comp, std::move(m_image_extents), + auto req = io::ImageDispatchSpec::create_read( + *m_image_ctx->parent, io::IMAGE_DISPATCH_LAYER_INTERNAL_START, comp, + std::move(m_image_extents), ReadResult{&m_copyup_extent_map, &m_copyup_data}, m_image_ctx->parent->get_data_io_context(), 0, 0, m_trace); + req->send(); } template diff --git a/src/librbd/io/Utils.cc b/src/librbd/io/Utils.cc index bfb89cfecc3..80e8032ef5f 100644 --- a/src/librbd/io/Utils.cc +++ b/src/librbd/io/Utils.cc @@ -9,7 +9,7 @@ #include "librbd/internal.h" #include "librbd/Utils.h" #include "librbd/io/AioCompletion.h" -#include "librbd/io/ImageRequest.h" +#include "librbd/io/ImageDispatchSpec.h" #include "osd/osd_types.h" #include "osdc/Striper.h" @@ -128,11 +128,11 @@ void read_parent(I *image_ctx, uint64_t object_no, ReadExtents* extents, AIO_TYPE_READ); ldout(cct, 20) << "completion " << comp << ", extents " << parent_extents << dendl; - - ImageRequest::aio_read( - image_ctx->parent, comp, std::move(parent_extents), - ReadResult{parent_read_bl}, + auto req = io::ImageDispatchSpec::create_read( + *image_ctx->parent, io::IMAGE_DISPATCH_LAYER_INTERNAL_START, comp, + std::move(parent_extents), ReadResult{parent_read_bl}, image_ctx->parent->get_data_io_context(), 0, 0, trace); + req->send(); } template diff --git a/src/test/librbd/io/test_mock_CopyupRequest.cc b/src/test/librbd/io/test_mock_CopyupRequest.cc index 70f1f17fd63..ee7a911c43e 100644 --- a/src/test/librbd/io/test_mock_CopyupRequest.cc +++ b/src/test/librbd/io/test_mock_CopyupRequest.cc @@ -13,7 +13,7 @@ #include "librbd/api/Io.h" #include "librbd/deep_copy/ObjectCopyRequest.h" #include "librbd/io/CopyupRequest.h" -#include "librbd/io/ImageRequest.h" +#include "librbd/io/ImageDispatchSpec.h" #include "librbd/io/ObjectRequest.h" #include "librbd/io/ReadResult.h" @@ -100,25 +100,6 @@ struct AbstractObjectWriteRequest { MOCK_METHOD1(add_copyup_ops, void(neorados::WriteOp*)); }; -template <> -struct ImageRequest { - static ImageRequest *s_instance; - static void aio_read(librbd::MockImageCtx *ictx, AioCompletion *c, - Extents &&image_extents, ReadResult &&read_result, - IOContext io_context, int op_flags, int read_flags, - const ZTracer::Trace &parent_trace) { - s_instance->aio_read(c, image_extents, &read_result); - } - - MOCK_METHOD3(aio_read, void(AioCompletion *, const Extents&, ReadResult*)); - - ImageRequest() { - s_instance = this; - } -}; - -ImageRequest* ImageRequest::s_instance = nullptr; - } // namespace io } // namespace librbd @@ -129,6 +110,11 @@ static bool operator==(const SnapContext& rhs, const SnapContext& lhs) { #include "librbd/AsyncObjectThrottle.cc" #include "librbd/io/CopyupRequest.cc" +MATCHER_P(IsRead, image_extents, "") { + auto req = boost::get(&arg->request); + return (req != nullptr && image_extents == arg->image_extents); +} + namespace librbd { namespace io { @@ -143,7 +129,6 @@ using ::testing::WithoutArgs; struct TestMockIoCopyupRequest : public TestMockFixture { typedef CopyupRequest MockCopyupRequest; - typedef ImageRequest MockImageRequest; typedef ObjectRequest MockObjectRequest; typedef AbstractObjectWriteRequest MockAbstractObjectWriteRequest; typedef deep_copy::ObjectCopyRequest MockObjectCopyRequest; @@ -191,22 +176,33 @@ struct TestMockIoCopyupRequest : public TestMockFixture { }))); } - void expect_read_parent(MockTestImageCtx& mock_image_ctx, - MockImageRequest& mock_image_request, + void expect_read_parent(librbd::MockTestImageCtx& mock_image_ctx, const Extents& image_extents, const std::string& data, int r) { - EXPECT_CALL(mock_image_request, aio_read(_, image_extents, _)) - .WillOnce(WithArgs<0, 2>(Invoke( - [&mock_image_ctx, image_extents, data, r]( - AioCompletion* aio_comp, ReadResult* read_result) { - aio_comp->read_result = std::move(*read_result); + EXPECT_CALL(*mock_image_ctx.io_image_dispatcher, + send(IsRead(image_extents))) + .WillOnce(Invoke( + [&mock_image_ctx, image_extents, data, r](io::ImageDispatchSpec* spec) { + auto req = boost::get( + &spec->request); + ASSERT_TRUE(req != nullptr); + + if (r < 0) { + spec->fail(r); + return; + } + + spec->dispatch_result = DISPATCH_RESULT_COMPLETE; + + auto aio_comp = spec->aio_comp; + aio_comp->read_result = std::move(req->read_result); aio_comp->read_result.set_image_extents(image_extents); aio_comp->set_request_count(1); auto ctx = new ReadResult::C_ImageReadRequest(aio_comp, image_extents); ctx->bl.append(data); mock_image_ctx.image_ctx->op_work_queue->queue(ctx, r); - }))); + })); } void expect_copyup(MockTestImageCtx& mock_image_ctx, uint64_t snap_id, @@ -388,10 +384,8 @@ TEST_F(TestMockIoCopyupRequest, Standard) { InSequence seq; - MockImageRequest mock_image_request; std::string data(4096, '1'); - expect_read_parent(mock_parent_image_ctx, mock_image_request, {{0, 4096}}, - data, 0); + expect_read_parent(mock_parent_image_ctx, {{0, 4096}}, data, 0); expect_prepare_copyup(mock_image_ctx); MockAbstractObjectWriteRequest mock_write_request; @@ -445,10 +439,8 @@ TEST_F(TestMockIoCopyupRequest, StandardWithSnaps) { InSequence seq; - MockImageRequest mock_image_request; std::string data(4096, '1'); - expect_read_parent(mock_parent_image_ctx, mock_image_request, {{0, 4096}}, - data, 0); + expect_read_parent(mock_parent_image_ctx, {{0, 4096}}, data, 0); expect_prepare_copyup(mock_image_ctx); MockAbstractObjectWriteRequest mock_write_request; @@ -494,10 +486,8 @@ TEST_F(TestMockIoCopyupRequest, CopyOnRead) { InSequence seq; - MockImageRequest mock_image_request; std::string data(4096, '1'); - expect_read_parent(mock_parent_image_ctx, mock_image_request, {{0, 4096}}, - data, 0); + expect_read_parent(mock_parent_image_ctx, {{0, 4096}}, data, 0); expect_prepare_copyup(mock_image_ctx); expect_object_map_at(mock_image_ctx, 0, OBJECT_NONEXISTENT); @@ -541,10 +531,8 @@ TEST_F(TestMockIoCopyupRequest, CopyOnReadWithSnaps) { InSequence seq; - MockImageRequest mock_image_request; std::string data(4096, '1'); - expect_read_parent(mock_parent_image_ctx, mock_image_request, {{0, 4096}}, - data, 0); + expect_read_parent(mock_parent_image_ctx, {{0, 4096}}, data, 0); expect_prepare_copyup(mock_image_ctx); expect_object_map_at(mock_image_ctx, 0, OBJECT_NONEXISTENT); @@ -849,10 +837,8 @@ TEST_F(TestMockIoCopyupRequest, ZeroedCopyOnRead) { InSequence seq; - MockImageRequest mock_image_request; std::string data(4096, '\0'); - expect_read_parent(mock_parent_image_ctx, mock_image_request, {{0, 4096}}, - data, 0); + expect_read_parent(mock_parent_image_ctx, {{0, 4096}}, data, 0); expect_prepare_copyup(mock_image_ctx); expect_object_map_at(mock_image_ctx, 0, OBJECT_NONEXISTENT); @@ -889,9 +875,7 @@ TEST_F(TestMockIoCopyupRequest, NoOpCopyup) { InSequence seq; - MockImageRequest mock_image_request; - expect_read_parent(mock_parent_image_ctx, mock_image_request, {{0, 4096}}, - "", -ENOENT); + expect_read_parent(mock_parent_image_ctx, {{0, 4096}}, "", -ENOENT); expect_prepare_copyup(mock_image_ctx); @@ -927,10 +911,8 @@ TEST_F(TestMockIoCopyupRequest, RestartWrite) { InSequence seq; - MockImageRequest mock_image_request; std::string data(4096, '1'); - expect_read_parent(mock_parent_image_ctx, mock_image_request, {{0, 4096}}, - data, 0); + expect_read_parent(mock_parent_image_ctx, {{0, 4096}}, data, 0); expect_prepare_copyup(mock_image_ctx); MockAbstractObjectWriteRequest mock_write_request1; @@ -983,9 +965,7 @@ TEST_F(TestMockIoCopyupRequest, ReadFromParentError) { InSequence seq; - MockImageRequest mock_image_request; - expect_read_parent(mock_parent_image_ctx, mock_image_request, {{0, 4096}}, - "", -EPERM); + expect_read_parent(mock_parent_image_ctx, {{0, 4096}}, "", -EPERM); auto req = new MockCopyupRequest(&mock_image_ctx, 0, {{0, 4096}}, {}); @@ -1055,10 +1035,8 @@ TEST_F(TestMockIoCopyupRequest, UpdateObjectMapError) { InSequence seq; - MockImageRequest mock_image_request; std::string data(4096, '1'); - expect_read_parent(mock_parent_image_ctx, mock_image_request, {{0, 4096}}, - data, 0); + expect_read_parent(mock_parent_image_ctx, {{0, 4096}}, data, 0); expect_prepare_copyup(mock_image_ctx); MockAbstractObjectWriteRequest mock_write_request; @@ -1104,10 +1082,8 @@ TEST_F(TestMockIoCopyupRequest, CopyupError) { InSequence seq; - MockImageRequest mock_image_request; std::string data(4096, '1'); - expect_read_parent(mock_parent_image_ctx, mock_image_request, {{0, 4096}}, - data, 0); + expect_read_parent(mock_parent_image_ctx, {{0, 4096}}, data, 0); expect_prepare_copyup(mock_image_ctx); MockAbstractObjectWriteRequest mock_write_request; @@ -1154,10 +1130,8 @@ TEST_F(TestMockIoCopyupRequest, SparseCopyupNotSupported) { InSequence seq; - MockImageRequest mock_image_request; std::string data(4096, '1'); - expect_read_parent(mock_parent_image_ctx, mock_image_request, {{0, 4096}}, - data, 0); + expect_read_parent(mock_parent_image_ctx, {{0, 4096}}, data, 0); expect_prepare_copyup(mock_image_ctx); MockAbstractObjectWriteRequest mock_write_request; @@ -1199,10 +1173,8 @@ TEST_F(TestMockIoCopyupRequest, ProcessCopyup) { InSequence seq; - MockImageRequest mock_image_request; std::string data(4096, '1'); - expect_read_parent(mock_parent_image_ctx, mock_image_request, {{0, 4096}}, - data, 0); + expect_read_parent(mock_parent_image_ctx, {{0, 4096}}, data, 0); bufferlist in_prepare_bl; in_prepare_bl.append(std::string(3072, '1')); @@ -1263,10 +1235,8 @@ TEST_F(TestMockIoCopyupRequest, ProcessCopyupOverwrite) { InSequence seq; - MockImageRequest mock_image_request; std::string data(4096, '1'); - expect_read_parent(mock_parent_image_ctx, mock_image_request, {{0, 4096}}, - data, 0); + expect_read_parent(mock_parent_image_ctx, {{0, 4096}}, data, 0); bufferlist in_prepare_bl; in_prepare_bl.append(data);