From 164fd6643cc4593b09b057a80780aeadec417159 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Wed, 4 Apr 2018 11:48:56 -0400 Subject: [PATCH] librbd: discard should wait for in-flight cache writeback to complete Signed-off-by: Jason Dillaman (derived from commit 0e04de4bb9c08755bd0915776bd538ce8f7c2b5b) --- src/librbd/io/ImageRequest.cc | 98 +++++++++++++++----- src/librbd/io/ImageRequest.h | 5 +- src/test/librbd/io/test_mock_ImageRequest.cc | 4 + 3 files changed, 80 insertions(+), 27 deletions(-) diff --git a/src/librbd/io/ImageRequest.cc b/src/librbd/io/ImageRequest.cc index 883bdd758dd..b75f5aa06c1 100644 --- a/src/librbd/io/ImageRequest.cc +++ b/src/librbd/io/ImageRequest.cc @@ -28,32 +28,75 @@ using util::get_image_ctx; namespace { template -struct C_DiscardJournalCommit : public Context { +struct C_DiscardRequest { typedef std::vector ObjectExtents; ImageCtxT &image_ctx; - AioCompletion *aio_comp; + AioCompletion *aio_completion; ObjectExtents object_extents; - C_DiscardJournalCommit(ImageCtxT &_image_ctx, AioCompletion *_aio_comp, - const ObjectExtents &_object_extents, uint64_t tid) - : image_ctx(_image_ctx), aio_comp(_aio_comp), - object_extents(_object_extents) { + C_DiscardRequest(ImageCtxT &image_ctx, AioCompletion *aio_completion, + const ObjectExtents &object_extents) + : image_ctx(image_ctx), aio_completion(aio_completion), + object_extents(object_extents) { + aio_completion->add_request(); + } + + void send() { + discard_writeback(); + } + + void discard_writeback() { + CephContext *cct = image_ctx.cct; + ldout(cct, 20) << "C_DiscardRequest: delaying cache discard until " + << "writeback flushed" << dendl; + + // ensure we aren't holding the cache lock post-write + auto ctx = util::create_async_context_callback( + image_ctx, util::create_context_callback< + C_DiscardRequest, + &C_DiscardRequest::handle_discard_writeback>(this)); + + // ensure any in-flight writeback is complete before advancing + // the discard request + Mutex::Locker cache_locker(image_ctx.cache_lock); + image_ctx.object_cacher->discard_writeback(image_ctx.object_set, + object_extents, ctx); + } + + void handle_discard_writeback(int r) { + CephContext *cct = image_ctx.cct; + ldout(cct, 20) << "C_DiscardRequest: writeback flushed: " << r << dendl; + + { + Mutex::Locker cache_locker(image_ctx.cache_lock); + image_ctx.object_cacher->discard_set(image_ctx.object_set, + object_extents); + } + aio_completion->complete_request(r); + delete this; + } +}; + +template +struct C_DiscardJournalCommit : public Context { + ImageCtxT &image_ctx; + C_DiscardRequest *discard_request; + + C_DiscardJournalCommit(ImageCtxT &_image_ctx, + C_DiscardRequest *discard_request, + uint64_t tid) + : image_ctx(_image_ctx), discard_request(discard_request) { CephContext *cct = image_ctx.cct; ldout(cct, 20) << "delaying cache discard until journal tid " << tid << " " << "safe" << dendl; - - aio_comp->add_request(); } void finish(int r) override { CephContext *cct = image_ctx.cct; ldout(cct, 20) << "C_DiscardJournalCommit: " << "journal committed: discarding from cache" << dendl; - - Mutex::Locker cache_locker(image_ctx.cache_lock); - image_ctx.object_cacher->discard_set(image_ctx.object_set, object_extents); - aio_comp->complete_request(r); + discard_request->send(); } }; @@ -402,7 +445,7 @@ void AbstractImageWriteRequest::send_request() { if (!object_extents.empty()) { uint64_t journal_tid = 0; aio_comp->set_request_count( - object_extents.size() + get_object_cache_request_count(journaling)); + object_extents.size() + get_object_cache_request_count()); ObjectRequests requests; send_object_requests(object_extents, snapc, @@ -598,10 +641,10 @@ int ImageDiscardRequest::prune_object_extents(ObjectExtents &object_extents) } template -uint32_t ImageDiscardRequest::get_object_cache_request_count(bool journaling) const { - // extra completion request is required for tracking journal commit +uint32_t ImageDiscardRequest::get_object_cache_request_count() const { + // extra completion request is required for tracking discard I &image_ctx = this->m_image_ctx; - return (image_ctx.object_cacher != nullptr && journaling ? 1 : 0); + return (image_ctx.object_cacher != nullptr ? 1 : 0); } template @@ -622,17 +665,15 @@ template void ImageDiscardRequest::send_object_cache_requests( const ObjectExtents &object_extents, uint64_t journal_tid) { I &image_ctx = this->m_image_ctx; + auto aio_comp = this->m_aio_comp; + auto req = new C_DiscardRequest(image_ctx, aio_comp, object_extents); if (journal_tid == 0) { - Mutex::Locker cache_locker(image_ctx.cache_lock); - image_ctx.object_cacher->discard_set(image_ctx.object_set, - object_extents); + req->send(); } else { // cannot discard from cache until journal has committed assert(image_ctx.journal != NULL); - AioCompletion *aio_comp = this->m_aio_comp; image_ctx.journal->wait_event( - journal_tid, new C_DiscardJournalCommit(image_ctx, aio_comp, - object_extents, journal_tid)); + journal_tid, new C_DiscardJournalCommit(image_ctx, req, journal_tid)); } } @@ -881,15 +922,22 @@ uint64_t ImageCompareAndWriteRequest::append_journal_event( return tid; } +template +uint32_t ImageCompareAndWriteRequest::get_object_cache_request_count() const { + // extra completion request is required for tracking discard + I &image_ctx = this->m_image_ctx; + return (image_ctx.object_cacher != nullptr ? 1 : 0); +} + template void ImageCompareAndWriteRequest::send_object_cache_requests( const ObjectExtents &object_extents, uint64_t journal_tid) { I &image_ctx = this->m_image_ctx; if (image_ctx.object_cacher != NULL) { - Mutex::Locker cache_locker(image_ctx.cache_lock); - image_ctx.object_cacher->discard_set(image_ctx.object_set, - object_extents); + auto aio_comp = this->m_aio_comp; + auto req = new C_DiscardRequest(image_ctx, aio_comp, object_extents); + req->send(); } } diff --git a/src/librbd/io/ImageRequest.h b/src/librbd/io/ImageRequest.h index 9d707a3f8f5..24239d3e5be 100644 --- a/src/librbd/io/ImageRequest.h +++ b/src/librbd/io/ImageRequest.h @@ -183,7 +183,7 @@ protected: virtual int prune_object_extents(ObjectExtents &object_extents) { return 0; } - virtual uint32_t get_object_cache_request_count(bool journaling) const { + virtual uint32_t get_object_cache_request_count() const { return 0; } virtual void send_object_cache_requests(const ObjectExtents &object_extents, @@ -278,7 +278,7 @@ protected: void send_image_cache_request() override; - uint32_t get_object_cache_request_count(bool journaling) const override; + uint32_t get_object_cache_request_count() const override; void send_object_cache_requests(const ObjectExtents &object_extents, uint64_t journal_tid) override; @@ -386,6 +386,7 @@ public: protected: void send_image_cache_request() override; + uint32_t get_object_cache_request_count() const override; void send_object_cache_requests(const ObjectExtents &object_extents, uint64_t journal_tid) override; diff --git a/src/test/librbd/io/test_mock_ImageRequest.cc b/src/test/librbd/io/test_mock_ImageRequest.cc index e85ae259cd0..8460926d195 100644 --- a/src/test/librbd/io/test_mock_ImageRequest.cc +++ b/src/test/librbd/io/test_mock_ImageRequest.cc @@ -248,6 +248,8 @@ TEST_F(TestMockIoImageRequest, AioDiscardJournalAppendDisabled) { MockJournal mock_journal; mock_image_ctx.journal = &mock_journal; + expect_op_work_queue(mock_image_ctx); + InSequence seq; expect_is_journal_appending(mock_journal, false); if (!ictx->skip_partial_discard) { @@ -342,6 +344,8 @@ TEST_F(TestMockIoImageRequest, AioCompareAndWriteJournalAppendDisabled) { MockJournal mock_journal; mock_image_ctx.journal = &mock_journal; + expect_op_work_queue(mock_image_ctx); + InSequence seq; expect_is_journal_appending(mock_journal, false); expect_object_request_send(mock_image_ctx, mock_aio_object_request, 0); -- 2.47.3