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 <dillaman@redhat.com>

diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc
index 50a3a0f38d31688ffdf42e43e5f55c13718f4eb8..1de2e14e9adbe861201e01540d6516240281d9ec 100644 (file)
--- 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<uint64_t>("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<Context>(
        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 f4cadd3c394c17765007ce6a5168f8467e4c8835..93effcb1cbb6858e5a01285786d00326404b3edd 100644 (file)
--- 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<I>::read_from_parent() {
   ldout(cct, 20) << "completion=" << comp << ", "
                  << "extents=" << m_image_extents
                  << dendl;
-  ImageRequest<I>::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 <typename I>

diff --git a/src/librbd/io/Utils.cc b/src/librbd/io/Utils.cc
index bfb89cfecc37b97e6be1c6fce45b005e7fd6d069..80e8032ef5f369a836c7e58a2ac3457b305a5b3b 100644 (file)
--- 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<I>::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 <typename I>

diff --git a/src/test/librbd/io/test_mock_CopyupRequest.cc b/src/test/librbd/io/test_mock_CopyupRequest.cc
index 70f1f17fd63aecdcd1d174704730557031eefa59..ee7a911c43e2caf6ddf7e2b0e9b0fb6d6ae8d823 100644 (file)
--- 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<librbd::MockTestImageCtx> {
   MOCK_METHOD1(add_copyup_ops, void(neorados::WriteOp*));
 };
 
-template <>
-struct ImageRequest<librbd::MockTestImageCtx> {
-  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<librbd::MockTestImageCtx>* ImageRequest<librbd::MockTestImageCtx>::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<librbd::io::ImageDispatchSpec::Read>(&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<librbd::MockTestImageCtx> MockCopyupRequest;
-  typedef ImageRequest<librbd::MockTestImageCtx> MockImageRequest;
   typedef ObjectRequest<librbd::MockTestImageCtx> MockObjectRequest;
   typedef AbstractObjectWriteRequest<librbd::MockTestImageCtx> MockAbstractObjectWriteRequest;
   typedef deep_copy::ObjectCopyRequest<librbd::MockTestImageCtx> 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<librbd::io::ImageDispatchSpec::Read>(
+            &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);