From 475f0fd9ef79c518372c7fd51a10a5551f6be232 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Mon, 5 Mar 2018 08:41:07 -0500 Subject: [PATCH] librbd: moved skip partial discard logic to object request This allows the journal object dispatch layer to properly complete skipped extents and commit the associated event. Signed-off-by: Jason Dillaman --- src/librbd/io/ImageRequest.cc | 34 +++--------- src/librbd/io/ImageRequest.h | 9 ++-- src/librbd/io/ObjectDispatch.cc | 8 +-- src/librbd/io/ObjectRequest.cc | 23 ++++++-- src/librbd/io/ObjectRequest.h | 20 ++++--- src/librbd/io/Types.h | 3 +- src/test/librbd/io/test_mock_ImageRequest.cc | 4 +- src/test/librbd/io/test_mock_ObjectRequest.cc | 53 +++++++++++++++---- 8 files changed, 90 insertions(+), 64 deletions(-) diff --git a/src/librbd/io/ImageRequest.cc b/src/librbd/io/ImageRequest.cc index ea53f44c566..496b43dcec3 100644 --- a/src/librbd/io/ImageRequest.cc +++ b/src/librbd/io/ImageRequest.cc @@ -276,7 +276,7 @@ void AbstractImageWriteRequest::send_request() { image_ctx.journal->is_journal_appending()); } - int ret = prune_object_extents(object_extents); + int ret = validate_object_extents(object_extents); if (ret < 0) { aio_comp->fail(ret); return; @@ -406,28 +406,6 @@ uint64_t ImageDiscardRequest::append_journal_event(bool synchronous) { return tid; } -template -int ImageDiscardRequest::prune_object_extents(ObjectExtents &object_extents) { - I &image_ctx = this->m_image_ctx; - CephContext *cct = image_ctx.cct; - if (!this->m_skip_partial_discard) { - return 0; - } - - for (auto p = object_extents.begin(); p != object_extents.end(); ) { - if (p->offset + p->length < image_ctx.layout.object_size) { - ldout(cct, 20) << "oid " << p->oid << " " << p->offset << "~" - << p->length << " from " << p->buffer_extents - << ": skip partial discard" << dendl; - p = object_extents.erase(p); - } else { - ++p; - } - } - - return 0; -} - template void ImageDiscardRequest::send_image_cache_request() { I &image_ctx = this->m_image_ctx; @@ -447,11 +425,14 @@ ObjectDispatchSpec *ImageDiscardRequest::create_object_request( const ObjectExtent &object_extent, const ::SnapContext &snapc, uint64_t journal_tid, Context *on_finish) { I &image_ctx = this->m_image_ctx; + int discard_flags = OBJECT_DISCARD_FLAG_DISABLE_CLONE_REMOVE; + if (m_skip_partial_discard) { + discard_flags |= OBJECT_DISCARD_FLAG_SKIP_PARTIAL; + } auto req = ObjectDispatchSpec::create_discard( &image_ctx, OBJECT_DISPATCH_LAYER_NONE, object_extent.oid.name, object_extent.objectno, object_extent.offset, object_extent.length, snapc, - OBJECT_DISCARD_FLAG_DISABLE_CLONE_REMOVE, journal_tid, - this->m_trace, on_finish); + discard_flags, journal_tid, this->m_trace, on_finish); return req; } @@ -663,7 +644,8 @@ void ImageCompareAndWriteRequest::update_stats(size_t length) { } template -int ImageCompareAndWriteRequest::prune_object_extents(ObjectExtents &object_extents) { +int ImageCompareAndWriteRequest::validate_object_extents( + const ObjectExtents &object_extents) const { if (object_extents.size() > 1) return -EINVAL; diff --git a/src/librbd/io/ImageRequest.h b/src/librbd/io/ImageRequest.h index 3123ebd9c6d..7a934e5bebb 100644 --- a/src/librbd/io/ImageRequest.h +++ b/src/librbd/io/ImageRequest.h @@ -139,7 +139,8 @@ protected: void send_request() override; - virtual int prune_object_extents(ObjectExtents &object_extents) { + virtual int validate_object_extents( + const ObjectExtents &object_extents) const { return 0; } @@ -219,8 +220,6 @@ protected: return "aio_discard"; } - int prune_object_extents(ObjectExtents &object_extents) override; - void send_image_cache_request() override; ObjectDispatchSpec *create_object_request( @@ -335,7 +334,9 @@ protected: return "aio_compare_and_write"; } - int prune_object_extents(ObjectExtents &object_extents) override; + int validate_object_extents( + const ObjectExtents &object_extents) const override; + private: bufferlist m_cmp_bl; bufferlist m_bl; diff --git a/src/librbd/io/ObjectDispatch.cc b/src/librbd/io/ObjectDispatch.cc index 31090238ed8..85c8b034ec4 100644 --- a/src/librbd/io/ObjectDispatch.cc +++ b/src/librbd/io/ObjectDispatch.cc @@ -58,16 +58,10 @@ bool ObjectDispatch::discard( auto cct = m_image_ctx->cct; ldout(cct, 20) << oid << " " << object_off << "~" << object_len << dendl; - bool disable_clone_remove = ( - (discard_flags & OBJECT_DISCARD_FLAG_DISABLE_CLONE_REMOVE) != 0); - bool update_object_map = ( - (discard_flags & OBJECT_DISCARD_FLAG_DISABLE_OBJECT_MAP_UPDATE) == 0); - *dispatch_result = DISPATCH_RESULT_COMPLETE; auto req = new ObjectDiscardRequest(m_image_ctx, oid, object_no, object_off, object_len, snapc, - disable_clone_remove, - update_object_map, parent_trace, + discard_flags, parent_trace, on_dispatched); req->send(); return true; diff --git a/src/librbd/io/ObjectRequest.cc b/src/librbd/io/ObjectRequest.cc index 320daedb25c..23f38ee6414 100644 --- a/src/librbd/io/ObjectRequest.cc +++ b/src/librbd/io/ObjectRequest.cc @@ -64,14 +64,12 @@ ObjectRequest::create_discard(I *ictx, const std::string &oid, uint64_t object_no, uint64_t object_off, uint64_t object_len, const ::SnapContext &snapc, - bool disable_clone_remove, - bool update_object_map, + int discard_flags, const ZTracer::Trace &parent_trace, Context *completion) { return new ObjectDiscardRequest(ictx, oid, object_no, object_off, - object_len, snapc, disable_clone_remove, - update_object_map, parent_trace, - completion); + object_len, snapc, discard_flags, + parent_trace, completion); } template @@ -624,6 +622,21 @@ void ObjectWriteRequest::add_write_ops(librados::ObjectWriteOperation *wr) { wr->set_op_flags2(m_op_flags); } +template +void ObjectDiscardRequest::send() { + I *image_ctx = this->m_ictx; + auto cct = image_ctx->cct; + if ((m_discard_flags & OBJECT_DISCARD_FLAG_SKIP_PARTIAL) != 0 && + this->m_object_off + this->m_object_len < image_ctx->layout.object_size) { + ldout(cct, 20) << "oid " << this->m_oid << " " << this->m_object_off << "~" + << this->m_object_len << ": skip partial discard" << dendl; + this->async_finish(0); + return; + } + + AbstractObjectWriteRequest::send(); +} + template void ObjectWriteSameRequest::add_write_ops( librados::ObjectWriteOperation *wr) { diff --git a/src/librbd/io/ObjectRequest.h b/src/librbd/io/ObjectRequest.h index 9e8f3996c5e..3dc98b7df26 100644 --- a/src/librbd/io/ObjectRequest.h +++ b/src/librbd/io/ObjectRequest.h @@ -40,8 +40,8 @@ public: static ObjectRequest* create_discard( ImageCtxT *ictx, const std::string &oid, uint64_t object_no, uint64_t object_off, uint64_t object_len, const ::SnapContext &snapc, - bool disable_clone_remove, bool update_object_map, - const ZTracer::Trace &parent_trace, Context *completion); + int discard_flags, const ZTracer::Trace &parent_trace, + Context *completion); static ObjectRequest* create_write_same( ImageCtxT *ictx, const std::string &oid, uint64_t object_no, uint64_t object_off, uint64_t object_len, ceph::bufferlist&& data, @@ -294,14 +294,15 @@ public: ObjectDiscardRequest(ImageCtxT *ictx, const std::string &oid, uint64_t object_no, uint64_t object_off, uint64_t object_len, const ::SnapContext &snapc, - bool disable_clone_remove, bool update_object_map, - const ZTracer::Trace &parent_trace, Context *completion) + int discard_flags, const ZTracer::Trace &parent_trace, + Context *completion) : AbstractObjectWriteRequest(ictx, oid, object_no, object_off, object_len, snapc, "discard", parent_trace, completion), - m_update_object_map(update_object_map) { + m_discard_flags(discard_flags) { if (this->m_full_object) { - if (disable_clone_remove && this->has_parent()) { + if ((m_discard_flags & OBJECT_DISCARD_FLAG_DISABLE_CLONE_REMOVE) != 0 && + this->has_parent()) { // need to hide the parent object instead of child object m_discard_action = DISCARD_ACTION_REMOVE_TRUNCATE; this->m_object_len = 0; @@ -337,12 +338,15 @@ public: return OBJECT_EXISTS; } + void send() override; + protected: bool is_no_op_for_nonexistent_object() const override { return (!this->has_parent()); } bool is_object_map_update_enabled() const override { - return m_update_object_map; + return ( + (m_discard_flags & OBJECT_DISCARD_FLAG_DISABLE_OBJECT_MAP_UPDATE) == 0); } bool is_non_existent_post_write_object_map_state() const override { return (m_discard_action == DISCARD_ACTION_REMOVE); @@ -379,7 +383,7 @@ private: }; DiscardAction m_discard_action; - bool m_update_object_map; + int m_discard_flags; }; diff --git a/src/librbd/io/Types.h b/src/librbd/io/Types.h index b32a5372d66..ac623170994 100644 --- a/src/librbd/io/Types.h +++ b/src/librbd/io/Types.h @@ -52,7 +52,8 @@ enum ObjectDispatchLayer { enum { OBJECT_DISCARD_FLAG_DISABLE_CLONE_REMOVE = 1UL << 0, - OBJECT_DISCARD_FLAG_DISABLE_OBJECT_MAP_UPDATE = 1UL << 1 + OBJECT_DISCARD_FLAG_DISABLE_OBJECT_MAP_UPDATE = 1UL << 1, + OBJECT_DISCARD_FLAG_SKIP_PARTIAL = 1UL << 2 }; enum { diff --git a/src/test/librbd/io/test_mock_ImageRequest.cc b/src/test/librbd/io/test_mock_ImageRequest.cc index c84ca96f7ce..a785de2dbaa 100644 --- a/src/test/librbd/io/test_mock_ImageRequest.cc +++ b/src/test/librbd/io/test_mock_ImageRequest.cc @@ -129,9 +129,7 @@ TEST_F(TestMockIoImageRequest, AioDiscardJournalAppendDisabled) { InSequence seq; expect_is_journal_appending(mock_journal, false); - if (!ictx->skip_partial_discard) { - expect_object_request_send(mock_image_ctx, 0); - } + expect_object_request_send(mock_image_ctx, 0); C_SaferCond aio_comp_ctx; AioCompletion *aio_comp = AioCompletion::create_and_start( diff --git a/src/test/librbd/io/test_mock_ObjectRequest.cc b/src/test/librbd/io/test_mock_ObjectRequest.cc index 0958ac2fe63..496af7fb72b 100644 --- a/src/test/librbd/io/test_mock_ObjectRequest.cc +++ b/src/test/librbd/io/test_mock_ObjectRequest.cc @@ -836,8 +836,7 @@ TEST_F(TestMockIoObjectRequest, DiscardRemove) { C_SaferCond ctx; auto req = MockObjectDiscardRequest::create_discard( &mock_image_ctx, ictx->get_object_name(0), 0, 0, - mock_image_ctx.get_object_size(), mock_image_ctx.snapc, false, true, {}, - &ctx); + mock_image_ctx.get_object_size(), mock_image_ctx.snapc, 0, {}, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } @@ -883,8 +882,9 @@ TEST_F(TestMockIoObjectRequest, DiscardRemoveTruncate) { C_SaferCond ctx; auto req = MockObjectDiscardRequest::create_discard( &mock_image_ctx, ictx->get_object_name(0), 0, 0, - mock_image_ctx.get_object_size(), mock_image_ctx.snapc, true, true, {}, - &ctx); + mock_image_ctx.get_object_size(), mock_image_ctx.snapc, + OBJECT_DISCARD_FLAG_DISABLE_CLONE_REMOVE | + OBJECT_DISCARD_FLAG_DISABLE_OBJECT_MAP_UPDATE, {}, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } @@ -916,8 +916,7 @@ TEST_F(TestMockIoObjectRequest, DiscardTruncate) { C_SaferCond ctx; auto req = MockObjectDiscardRequest::create_discard( &mock_image_ctx, ictx->get_object_name(0), 0, 1, - mock_image_ctx.get_object_size() - 1, mock_image_ctx.snapc, false, true, {}, - &ctx); + mock_image_ctx.get_object_size() - 1, mock_image_ctx.snapc, 0, {}, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } @@ -949,7 +948,7 @@ TEST_F(TestMockIoObjectRequest, DiscardZero) { C_SaferCond ctx; auto req = MockObjectDiscardRequest::create_discard( &mock_image_ctx, ictx->get_object_name(0), 0, 1, 1, mock_image_ctx.snapc, - false, true, {}, &ctx); + 0, {}, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } @@ -978,8 +977,9 @@ TEST_F(TestMockIoObjectRequest, DiscardDisableObjectMapUpdate) { C_SaferCond ctx; auto req = MockObjectDiscardRequest::create_discard( &mock_image_ctx, ictx->get_object_name(0), 0, 0, - mock_image_ctx.get_object_size(), mock_image_ctx.snapc, true, false, {}, - &ctx); + mock_image_ctx.get_object_size(), mock_image_ctx.snapc, + OBJECT_DISCARD_FLAG_DISABLE_CLONE_REMOVE | + OBJECT_DISCARD_FLAG_DISABLE_OBJECT_MAP_UPDATE, {}, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } @@ -1008,12 +1008,45 @@ TEST_F(TestMockIoObjectRequest, DiscardNoOp) { C_SaferCond ctx; auto req = MockObjectDiscardRequest::create_discard( &mock_image_ctx, ictx->get_object_name(0), 0, 0, - mock_image_ctx.get_object_size(), mock_image_ctx.snapc, true, false, {}, + mock_image_ctx.get_object_size(), mock_image_ctx.snapc, + OBJECT_DISCARD_FLAG_DISABLE_CLONE_REMOVE | + OBJECT_DISCARD_FLAG_DISABLE_OBJECT_MAP_UPDATE, {}, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } +TEST_F(TestMockIoObjectRequest, SkipPartialDiscard) { + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_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; + } + + expect_op_work_queue(mock_image_ctx); + + InSequence seq; + expect_get_parent_overlap(mock_image_ctx, CEPH_NOSNAP, 0, 0); + + C_SaferCond ctx; + auto req = MockObjectDiscardRequest::create_discard( + &mock_image_ctx, ictx->get_object_name(0), 0, 0, 1, mock_image_ctx.snapc, + OBJECT_DISCARD_FLAG_SKIP_PARTIAL, {}, &ctx); + req->send(); + ASSERT_EQ(0, ctx.wait()); +} + TEST_F(TestMockIoObjectRequest, WriteSame) { librbd::ImageCtx *ictx; ASSERT_EQ(0, open_image(m_image_name, &ictx)); -- 2.39.5