From e79c0cf2009c69681ed4c70f52ade2f5fd570567 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Tue, 7 Nov 2017 14:36:10 -0500 Subject: [PATCH] librbd: object cacher should re-use read state machine This adds support for sparse-reads and ensures all object reads utilize a single, tested code path. Signed-off-by: Jason Dillaman --- src/librbd/LibrbdWriteback.cc | 44 +++++++-------- src/librbd/io/ImageRequest.cc | 4 +- src/librbd/io/ObjectRequest.cc | 11 +++- src/librbd/io/ObjectRequest.h | 10 ++-- src/librbd/io/ReadResult.cc | 5 +- src/librbd/io/ReadResult.h | 11 ++-- src/test/librbd/io/test_mock_ObjectRequest.cc | 56 ++++++++++++++++--- 7 files changed, 97 insertions(+), 44 deletions(-) diff --git a/src/librbd/LibrbdWriteback.cc b/src/librbd/LibrbdWriteback.cc index f0be832eb171f..7bc90cadefb85 100644 --- a/src/librbd/LibrbdWriteback.cc +++ b/src/librbd/LibrbdWriteback.cc @@ -20,6 +20,7 @@ #include "librbd/Utils.h" #include "librbd/io/AioCompletion.h" #include "librbd/io/ObjectRequest.h" +#include "librbd/io/ReadResult.h" #include "include/assert.h" @@ -204,30 +205,29 @@ namespace librbd { const ZTracer::Trace &parent_trace, Context *onfinish) { - // on completion, take the mutex and then call onfinish. - Context *req = new C_ReadRequest(m_ictx->cct, onfinish, &m_lock); - - { - RWLock::RLocker snap_locker(m_ictx->snap_lock); - if (m_ictx->object_map != nullptr && - !m_ictx->object_map->object_may_exist(object_no)) { - m_ictx->op_work_queue->queue(req, -ENOENT); - return; - } + ZTracer::Trace trace; + if (parent_trace.valid()) { + trace.init("", &m_ictx->trace_endpoint, &parent_trace); + trace.copy_name("cache read " + oid.name); + trace.event("start"); } - librados::ObjectReadOperation op; - op.read(off, len, pbl, NULL); - op.set_op_flags2(op_flags); - int flags = m_ictx->get_read_flags(snapid); - - librados::AioCompletion *rados_completion = - util::create_rados_callback(req); - int r = m_ictx->data_ctx.aio_operate( - oid.name, rados_completion, &op, flags, nullptr, - (parent_trace.valid() ? parent_trace.get_info() : nullptr)); - rados_completion->release(); - assert(r >= 0); + // on completion, take the mutex and then call onfinish. + onfinish = new C_ReadRequest(m_ictx->cct, onfinish, &m_lock); + + // re-use standard object read state machine + auto aio_comp = io::AioCompletion::create_and_start(onfinish, m_ictx, + io::AIO_TYPE_READ); + aio_comp->read_result = io::ReadResult{pbl}; + aio_comp->set_request_count(1); + + auto req_comp = new io::ReadResult::C_SparseReadRequest<>( + aio_comp, {{0, len}}, false); + auto req = io::ObjectReadRequest<>::create(m_ictx, oid.name, object_no, off, + len, snapid, op_flags, true, + trace, req_comp); + req_comp->request = req; + req->send(); } bool LibrbdWriteback::may_copy_on_write(const object_t& oid, uint64_t read_off, uint64_t read_len, snapid_t snapid) diff --git a/src/librbd/io/ImageRequest.cc b/src/librbd/io/ImageRequest.cc index 9c2b1989e1884..50a9f30ee448b 100644 --- a/src/librbd/io/ImageRequest.cc +++ b/src/librbd/io/ImageRequest.cc @@ -325,10 +325,10 @@ void ImageReadRequest::send_request() { << dendl; auto req_comp = new io::ReadResult::C_SparseReadRequest( - aio_comp, std::move(extent.buffer_extents)); + aio_comp, std::move(extent.buffer_extents), true); ObjectReadRequest *req = ObjectReadRequest::create( &image_ctx, extent.oid.name, extent.objectno, extent.offset, - extent.length, snap_id, m_op_flags, this->m_trace, req_comp); + extent.length, snap_id, m_op_flags, false, this->m_trace, req_comp); req_comp->request = req; req->send(); } diff --git a/src/librbd/io/ObjectRequest.cc b/src/librbd/io/ObjectRequest.cc index 608581b87f3be..12c7afd85b011 100644 --- a/src/librbd/io/ObjectRequest.cc +++ b/src/librbd/io/ObjectRequest.cc @@ -215,12 +215,12 @@ template ObjectReadRequest::ObjectReadRequest(I *ictx, const std::string &oid, uint64_t objectno, uint64_t offset, uint64_t len, librados::snap_t snap_id, - int op_flags, + int op_flags, bool cache_initiated, const ZTracer::Trace &parent_trace, Context *completion) : ObjectRequest(ictx, oid, objectno, offset, len, snap_id, false, "read", parent_trace, completion), - m_op_flags(op_flags) { + m_op_flags(op_flags), m_cache_initiated(cache_initiated) { } template @@ -228,7 +228,7 @@ void ObjectReadRequest::send() { I *image_ctx = this->m_ictx; ldout(image_ctx->cct, 20) << dendl; - if (image_ctx->object_cacher != nullptr) { + if (!m_cache_initiated && image_ctx->object_cacher != nullptr) { read_cache(); } else { read_object(); @@ -325,6 +325,11 @@ void ObjectReadRequest::handle_read_object(int r) { template void ObjectReadRequest::read_parent() { I *image_ctx = this->m_ictx; + if (m_cache_initiated) { + this->finish(-ENOENT); + return; + } + uint64_t object_overlap = 0; Extents parent_extents; { diff --git a/src/librbd/io/ObjectRequest.h b/src/librbd/io/ObjectRequest.h index 899196beb7218..93a373c40df67 100644 --- a/src/librbd/io/ObjectRequest.h +++ b/src/librbd/io/ObjectRequest.h @@ -152,17 +152,18 @@ public: static ObjectReadRequest* create(ImageCtxT *ictx, const std::string &oid, uint64_t objectno, uint64_t offset, uint64_t len, librados::snap_t snap_id, - int op_flags, + int op_flags, bool cache_initiated, const ZTracer::Trace &parent_trace, Context *completion) { return new ObjectReadRequest(ictx, oid, objectno, offset, len, - snap_id, op_flags, parent_trace, completion); + snap_id, op_flags, cache_initiated, + parent_trace, completion); } ObjectReadRequest(ImageCtxT *ictx, const std::string &oid, uint64_t objectno, uint64_t offset, uint64_t len, - librados::snap_t snap_id, - int op_flags, const ZTracer::Trace &parent_trace, + librados::snap_t snap_id, int op_flags, + bool cache_initiated, const ZTracer::Trace &parent_trace, Context *completion); bool should_complete(int r) override; @@ -217,6 +218,7 @@ private: */ int m_op_flags; + bool m_cache_initiated; ceph::bufferlist m_read_data; ExtentMap m_ext_map; diff --git a/src/librbd/io/ReadResult.cc b/src/librbd/io/ReadResult.cc index 448de54200a16..3b126871d1407 100644 --- a/src/librbd/io/ReadResult.cc +++ b/src/librbd/io/ReadResult.cc @@ -117,7 +117,10 @@ void ReadResult::C_SparseReadRequestBase::finish(ExtentMap &extent_map, ldout(cct, 10) << "C_SparseReadRequestBase: r = " << r << dendl; - if (r >= 0 || r == -ENOENT) { + if (ignore_enoent && r == -ENOENT) { + r = 0; + } + if (r >= 0) { ldout(cct, 10) << " got " << extent_map << " for " << buffer_extents << " bl " << bl.length() << dendl; diff --git a/src/librbd/io/ReadResult.h b/src/librbd/io/ReadResult.h index f11e83286a6f4..ab0e4dbe4f182 100644 --- a/src/librbd/io/ReadResult.h +++ b/src/librbd/io/ReadResult.h @@ -48,8 +48,10 @@ public: }; struct C_SparseReadRequestBase : public C_ReadRequest { - C_SparseReadRequestBase(AioCompletion *aio_completion) - : C_ReadRequest(aio_completion) { + bool ignore_enoent; + + C_SparseReadRequestBase(AioCompletion *aio_completion, bool ignore_enoent) + : C_ReadRequest(aio_completion), ignore_enoent(ignore_enoent) { } using C_ReadRequest::finish; @@ -62,8 +64,9 @@ public: ObjectReadRequest *request; Extents buffer_extents; - C_SparseReadRequest(AioCompletion *aio_completion, Extents&& buffer_extents) - : C_SparseReadRequestBase(aio_completion), + C_SparseReadRequest(AioCompletion *aio_completion, Extents&& buffer_extents, + bool ignore_enoent) + : C_SparseReadRequestBase(aio_completion, ignore_enoent), buffer_extents(std::move(buffer_extents)) { } diff --git a/src/test/librbd/io/test_mock_ObjectRequest.cc b/src/test/librbd/io/test_mock_ObjectRequest.cc index 3f0b4f5713254..7e6eb15d1a8a2 100644 --- a/src/test/librbd/io/test_mock_ObjectRequest.cc +++ b/src/test/librbd/io/test_mock_ObjectRequest.cc @@ -214,7 +214,7 @@ TEST_F(TestMockIoObjectRequest, Read) { C_SaferCond ctx; auto req = MockObjectReadRequest::create( - &mock_image_ctx, "object0", 0, 0, 4096, CEPH_NOSNAP, 0, {}, &ctx); + &mock_image_ctx, "object0", 0, 0, 4096, CEPH_NOSNAP, 0, false, {}, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } @@ -242,7 +242,7 @@ TEST_F(TestMockIoObjectRequest, SparseReadThreshold) { C_SaferCond ctx; auto req = MockObjectReadRequest::create( &mock_image_ctx, "object0", 0, 0, ictx->sparse_read_threshold_bytes, - CEPH_NOSNAP, 0, {}, &ctx); + CEPH_NOSNAP, 0, false, {}, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } @@ -267,7 +267,7 @@ TEST_F(TestMockIoObjectRequest, ReadError) { C_SaferCond ctx; auto req = MockObjectReadRequest::create( - &mock_image_ctx, "object0", 0, 0, 4096, CEPH_NOSNAP, 0, {}, &ctx); + &mock_image_ctx, "object0", 0, 0, 4096, CEPH_NOSNAP, 0, false, {}, &ctx); req->send(); ASSERT_EQ(-EPERM, ctx.wait()); } @@ -291,7 +291,7 @@ TEST_F(TestMockIoObjectRequest, CacheRead) { C_SaferCond ctx; auto req = MockObjectReadRequest::create( - &mock_image_ctx, "object0", 0, 0, 4096, CEPH_NOSNAP, 0, {}, &ctx); + &mock_image_ctx, "object0", 0, 0, 4096, CEPH_NOSNAP, 0, false, {}, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } @@ -315,7 +315,7 @@ TEST_F(TestMockIoObjectRequest, CacheReadError) { C_SaferCond ctx; auto req = MockObjectReadRequest::create( - &mock_image_ctx, "object0", 0, 0, 4096, CEPH_NOSNAP, 0, {}, &ctx); + &mock_image_ctx, "object0", 0, 0, 4096, CEPH_NOSNAP, 0, false, {}, &ctx); req->send(); ASSERT_EQ(-EPERM, ctx.wait()); } @@ -361,7 +361,7 @@ TEST_F(TestMockIoObjectRequest, ParentRead) { C_SaferCond ctx; auto req = MockObjectReadRequest::create( - &mock_image_ctx, "object0", 0, 0, 4096, CEPH_NOSNAP, 0, {}, &ctx); + &mock_image_ctx, "object0", 0, 0, 4096, CEPH_NOSNAP, 0, false, {}, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } @@ -407,11 +407,51 @@ TEST_F(TestMockIoObjectRequest, ParentReadError) { C_SaferCond ctx; auto req = MockObjectReadRequest::create( - &mock_image_ctx, "object0", 0, 0, 4096, CEPH_NOSNAP, 0, {}, &ctx); + &mock_image_ctx, "object0", 0, 0, 4096, CEPH_NOSNAP, 0, false, {}, &ctx); req->send(); ASSERT_EQ(-EPERM, ctx.wait()); } +TEST_F(TestMockIoObjectRequest, CacheInitiated) { + 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)); + ictx->sparse_read_threshold_bytes = 8096; + ictx->clone_copy_on_read = false; + + MockTestImageCtx mock_image_ctx(*ictx); + mock_image_ctx.parent = &mock_image_ctx; + + MockObjectMap mock_object_map; + if (ictx->test_features(RBD_FEATURE_OBJECT_MAP)) { + mock_image_ctx.object_map = &mock_object_map; + } + + InSequence seq; + expect_object_may_exist(mock_image_ctx, 0, true); + expect_get_read_flags(mock_image_ctx, CEPH_NOSNAP, 0); + expect_read(mock_image_ctx, "object0", 0, 4096, "", -ENOENT); + + C_SaferCond ctx; + auto req = MockObjectReadRequest::create( + &mock_image_ctx, "object0", 0, 0, 4096, CEPH_NOSNAP, 0, true, {}, &ctx); + req->send(); + ASSERT_EQ(-ENOENT, ctx.wait()); +} + TEST_F(TestMockIoObjectRequest, CopyOnRead) { REQUIRE_FEATURE(RBD_FEATURE_LAYERING); @@ -458,7 +498,7 @@ TEST_F(TestMockIoObjectRequest, CopyOnRead) { C_SaferCond ctx; auto req = MockObjectReadRequest::create( - &mock_image_ctx, "object0", 0, 0, 4096, CEPH_NOSNAP, 0, {}, &ctx); + &mock_image_ctx, "object0", 0, 0, 4096, CEPH_NOSNAP, 0, false, {}, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } -- 2.39.5