From 8a8582a3e0f4c4c69e1b715b235bd2e2fdcb68fb Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Thu, 5 Jan 2017 13:31:57 -0500 Subject: [PATCH] librbd: prevent assertion failure when journal IO is blacklisted Fixes: http://tracker.ceph.com/issues/18429 Signed-off-by: Jason Dillaman (cherry picked from commit c720f6e3704ed7e8cf41dffdb931dbb05d59a003) --- src/librbd/ImageCtx.cc | 13 +++-- src/librbd/ImageCtx.h | 5 +- src/librbd/LibrbdWriteback.cc | 7 ++- src/librbd/exclusive_lock/ReleaseRequest.cc | 47 ++++++++++++++++++- src/librbd/exclusive_lock/ReleaseRequest.h | 6 +++ src/librbd/internal.cc | 2 +- src/librbd/operation/ResizeRequest.cc | 2 +- .../operation/SnapshotRollbackRequest.cc | 2 +- .../test_mock_ReleaseRequest.cc | 24 ++++++++++ src/test/librbd/mock/MockImageCtx.h | 3 +- .../operation/test_mock_ResizeRequest.cc | 4 +- .../test_mock_SnapshotRollbackRequest.cc | 4 +- 12 files changed, 103 insertions(+), 16 deletions(-) diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc index d972edf757b5d..2e867ab65cb4a 100644 --- a/src/librbd/ImageCtx.cc +++ b/src/librbd/ImageCtx.cc @@ -138,7 +138,9 @@ struct C_InvalidateCache : public Context { } else { lderr(cct) << "could not release all objects from cache: " << unclean << " bytes remain" << dendl; - r = -EBUSY; + if (r == 0) { + r = -EBUSY; + } } if (reentrant_safe) { @@ -789,7 +791,7 @@ struct C_InvalidateCache : public Context { return result; } - void ImageCtx::invalidate_cache(Context *on_finish) { + void ImageCtx::invalidate_cache(bool purge_on_error, Context *on_finish) { if (object_cacher == NULL) { op_work_queue->queue(on_finish, 0); return; @@ -799,7 +801,7 @@ struct C_InvalidateCache : public Context { object_cacher->release_set(object_set); cache_lock.Unlock(); - flush_cache(new C_InvalidateCache(this, false, false, on_finish)); + flush_cache(new C_InvalidateCache(this, purge_on_error, false, on_finish)); } void ImageCtx::clear_nonexistence_cache() { @@ -809,6 +811,11 @@ struct C_InvalidateCache : public Context { object_cacher->clear_nonexistence(object_set); } + bool ImageCtx::is_cache_empty() { + Mutex::Locker locker(cache_lock); + return object_cacher->set_is_empty(object_set); + } + void ImageCtx::register_watch(Context *on_finish) { assert(image_watcher == NULL); image_watcher = new ImageWatcher<>(*this); diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h index 52cf2847a5691..de8232ddc25f6 100644 --- a/src/librbd/ImageCtx.h +++ b/src/librbd/ImageCtx.h @@ -277,9 +277,10 @@ namespace librbd { void user_flushed(); void flush_cache(Context *onfinish); void shut_down_cache(Context *on_finish); - int invalidate_cache(bool purge_on_error=false); - void invalidate_cache(Context *on_finish); + int invalidate_cache(bool purge_on_error); + void invalidate_cache(bool purge_on_error, Context *on_finish); void clear_nonexistence_cache(); + bool is_cache_empty(); void register_watch(Context *on_finish); uint64_t prune_parent_extents(vector >& objectx, uint64_t overlap); diff --git a/src/librbd/LibrbdWriteback.cc b/src/librbd/LibrbdWriteback.cc index 3b56ed282424e..8a6244d79f545 100644 --- a/src/librbd/LibrbdWriteback.cc +++ b/src/librbd/LibrbdWriteback.cc @@ -118,7 +118,12 @@ namespace librbd { virtual void complete(int r) { if (request_sent || r < 0) { - commit_io_event_extent(r); + if (request_sent && r == 0) { + // only commit IO events that are safely recorded to the backing image + // since the cache will retry all IOs that fail + commit_io_event_extent(0); + } + req_comp->complete(r); delete this; } else { diff --git a/src/librbd/exclusive_lock/ReleaseRequest.cc b/src/librbd/exclusive_lock/ReleaseRequest.cc index dc496536cd782..c37cbc86456c8 100644 --- a/src/librbd/exclusive_lock/ReleaseRequest.cc +++ b/src/librbd/exclusive_lock/ReleaseRequest.cc @@ -129,9 +129,52 @@ Context *ReleaseRequest::handle_block_writes(int *ret_val) { if (*ret_val == -EBLACKLISTED) { // allow clean shut down if blacklisted - lderr(cct) << "failed to block writes: " << cpp_strerror(*ret_val) << dendl; - *ret_val = 0; + lderr(cct) << "failed to block writes because client is blacklisted" + << dendl; } else if (*ret_val < 0) { + lderr(cct) << "failed to block writes: " << cpp_strerror(*ret_val) << dendl; + m_image_ctx.aio_work_queue->unblock_writes(); + return m_on_finish; + } + + send_invalidate_cache(false); + return nullptr; +} + +template +void ReleaseRequest::send_invalidate_cache(bool purge_on_error) { + if (m_image_ctx.object_cacher == nullptr) { + send_flush_notifies(); + return; + } + + CephContext *cct = m_image_ctx.cct; + ldout(cct, 10) << __func__ << ": purge_on_error=" << purge_on_error << dendl; + + RWLock::RLocker owner_lock(m_image_ctx.owner_lock); + Context *ctx = create_async_context_callback( + m_image_ctx, create_context_callback< + ReleaseRequest, + &ReleaseRequest::handle_invalidate_cache>(this)); + m_image_ctx.invalidate_cache(purge_on_error, ctx); +} + +template +Context *ReleaseRequest::handle_invalidate_cache(int *ret_val) { + CephContext *cct = m_image_ctx.cct; + ldout(cct, 10) << __func__ << ": r=" << *ret_val << dendl; + + if (*ret_val == -EBLACKLISTED) { + lderr(cct) << "failed to invalidate cache because client is blacklisted" + << dendl; + if (!m_image_ctx.is_cache_empty()) { + // force purge the cache after after being blacklisted + send_invalidate_cache(true); + return nullptr; + } + } else if (*ret_val < 0 && *ret_val != -EBUSY) { + lderr(cct) << "failed to invalidate cache: " << cpp_strerror(*ret_val) + << dendl; m_image_ctx.aio_work_queue->unblock_writes(); return m_on_finish; } diff --git a/src/librbd/exclusive_lock/ReleaseRequest.h b/src/librbd/exclusive_lock/ReleaseRequest.h index 7c070ef690352..17d5b93dfa471 100644 --- a/src/librbd/exclusive_lock/ReleaseRequest.h +++ b/src/librbd/exclusive_lock/ReleaseRequest.h @@ -42,6 +42,9 @@ private: * BLOCK_WRITES * | * v + * INVALIDATE_CACHE + * | + * v * FLUSH_NOTIFIES . . . . . . . . . . . . . . * | . * v . @@ -81,6 +84,9 @@ private: void send_block_writes(); Context *handle_block_writes(int *ret_val); + void send_invalidate_cache(bool purge_on_error); + Context *handle_invalidate_cache(int *ret_val); + void send_flush_notifies(); Context *handle_flush_notifies(int *ret_val); diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc index eb9f3f2cf9de2..8512e6f4999cc 100644 --- a/src/librbd/internal.cc +++ b/src/librbd/internal.cc @@ -2381,7 +2381,7 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force, RWLock::RLocker owner_locker(ictx->owner_lock); RWLock::WLocker md_locker(ictx->md_lock); - r = ictx->invalidate_cache(); + r = ictx->invalidate_cache(false); ictx->perfcounter->inc(l_librbd_invalidate_cache); return r; } diff --git a/src/librbd/operation/ResizeRequest.cc b/src/librbd/operation/ResizeRequest.cc index edd1ca36f08c4..078c268defaa3 100644 --- a/src/librbd/operation/ResizeRequest.cc +++ b/src/librbd/operation/ResizeRequest.cc @@ -220,7 +220,7 @@ void ResizeRequest::send_invalidate_cache() { // need to invalidate since we're deleting objects, and // ObjectCacher doesn't track non-existent objects RWLock::RLocker owner_locker(image_ctx.owner_lock); - image_ctx.invalidate_cache(create_async_context_callback( + image_ctx.invalidate_cache(false, create_async_context_callback( image_ctx, create_context_callback< ResizeRequest, &ResizeRequest::handle_invalidate_cache>(this))); } diff --git a/src/librbd/operation/SnapshotRollbackRequest.cc b/src/librbd/operation/SnapshotRollbackRequest.cc index 8edf5b763164c..4129544103dc7 100644 --- a/src/librbd/operation/SnapshotRollbackRequest.cc +++ b/src/librbd/operation/SnapshotRollbackRequest.cc @@ -286,7 +286,7 @@ Context *SnapshotRollbackRequest::send_invalidate_cache() { Context *ctx = create_context_callback< SnapshotRollbackRequest, &SnapshotRollbackRequest::handle_invalidate_cache>(this); - image_ctx.invalidate_cache(ctx); + image_ctx.invalidate_cache(true, ctx); return nullptr; } diff --git a/src/test/librbd/exclusive_lock/test_mock_ReleaseRequest.cc b/src/test/librbd/exclusive_lock/test_mock_ReleaseRequest.cc index 801099872f6e8..f15c2ff29b138 100644 --- a/src/test/librbd/exclusive_lock/test_mock_ReleaseRequest.cc +++ b/src/test/librbd/exclusive_lock/test_mock_ReleaseRequest.cc @@ -33,6 +33,7 @@ using ::testing::InSequence; using ::testing::Invoke; using ::testing::Return; using ::testing::StrEq; +using ::testing::WithArg; static const std::string TEST_COOKIE("auto 123"); @@ -91,6 +92,21 @@ public: .WillOnce(CompleteContext(0, mock_image_ctx.image_ctx->op_work_queue)); } + void expect_invalidate_cache(MockImageCtx &mock_image_ctx, bool purge, + int r) { + if (mock_image_ctx.object_cacher != nullptr) { + EXPECT_CALL(mock_image_ctx, invalidate_cache(purge, _)) + .WillOnce(WithArg<1>(CompleteContext(r, NULL))); + } + } + + void expect_is_cache_empty(MockImageCtx &mock_image_ctx, bool empty) { + if (mock_image_ctx.object_cacher != nullptr) { + EXPECT_CALL(mock_image_ctx, is_cache_empty()) + .WillOnce(Return(empty)); + } + } + void expect_flush_notifies(MockImageCtx &mock_image_ctx) { EXPECT_CALL(*mock_image_ctx.image_watcher, flush(_)) .WillOnce(CompleteContext(0, mock_image_ctx.image_ctx->op_work_queue)); @@ -122,6 +138,7 @@ TEST_F(TestMockExclusiveLockReleaseRequest, Success) { expect_prepare_lock(mock_image_ctx); expect_cancel_op_requests(mock_image_ctx, 0); expect_block_writes(mock_image_ctx, 0); + expect_invalidate_cache(mock_image_ctx, false, 0); expect_flush_notifies(mock_image_ctx); MockJournal *mock_journal = new MockJournal(); @@ -159,6 +176,7 @@ TEST_F(TestMockExclusiveLockReleaseRequest, SuccessJournalDisabled) { InSequence seq; expect_prepare_lock(mock_image_ctx); expect_cancel_op_requests(mock_image_ctx, 0); + expect_invalidate_cache(mock_image_ctx, false, 0); expect_flush_notifies(mock_image_ctx); MockObjectMap *mock_object_map = new MockObjectMap(); @@ -191,6 +209,7 @@ TEST_F(TestMockExclusiveLockReleaseRequest, SuccessObjectMapDisabled) { InSequence seq; expect_cancel_op_requests(mock_image_ctx, 0); + expect_invalidate_cache(mock_image_ctx, false, 0); expect_flush_notifies(mock_image_ctx); expect_unlock(mock_image_ctx, 0); @@ -219,6 +238,10 @@ TEST_F(TestMockExclusiveLockReleaseRequest, Blacklisted) { expect_prepare_lock(mock_image_ctx); expect_cancel_op_requests(mock_image_ctx, 0); expect_block_writes(mock_image_ctx, -EBLACKLISTED); + expect_invalidate_cache(mock_image_ctx, false, -EBLACKLISTED); + expect_is_cache_empty(mock_image_ctx, false); + expect_invalidate_cache(mock_image_ctx, true, -EBLACKLISTED); + expect_is_cache_empty(mock_image_ctx, true); expect_flush_notifies(mock_image_ctx); MockJournal *mock_journal = new MockJournal(); @@ -278,6 +301,7 @@ TEST_F(TestMockExclusiveLockReleaseRequest, UnlockError) { InSequence seq; expect_cancel_op_requests(mock_image_ctx, 0); expect_block_writes(mock_image_ctx, 0); + expect_invalidate_cache(mock_image_ctx, false, 0); expect_flush_notifies(mock_image_ctx); expect_unlock(mock_image_ctx, -EINVAL); diff --git a/src/test/librbd/mock/MockImageCtx.h b/src/test/librbd/mock/MockImageCtx.h index 8fb4c6489bcf4..943f0e874882a 100644 --- a/src/test/librbd/mock/MockImageCtx.h +++ b/src/test/librbd/mock/MockImageCtx.h @@ -163,8 +163,9 @@ struct MockImageCtx { MOCK_METHOD1(flush_copyup, void(Context *)); MOCK_METHOD1(flush_cache, void(Context *)); - MOCK_METHOD1(invalidate_cache, void(Context *)); + MOCK_METHOD2(invalidate_cache, void(bool, Context *)); MOCK_METHOD1(shut_down_cache, void(Context *)); + MOCK_METHOD0(is_cache_empty, bool()); MOCK_CONST_METHOD1(test_features, bool(uint64_t test_features)); MOCK_CONST_METHOD2(test_features, bool(uint64_t test_features, diff --git a/src/test/librbd/operation/test_mock_ResizeRequest.cc b/src/test/librbd/operation/test_mock_ResizeRequest.cc index 5a996599262fa..a2145fcedc237 100644 --- a/src/test/librbd/operation/test_mock_ResizeRequest.cc +++ b/src/test/librbd/operation/test_mock_ResizeRequest.cc @@ -119,8 +119,8 @@ public: } void expect_invalidate_cache(MockImageCtx &mock_image_ctx, int r) { - EXPECT_CALL(mock_image_ctx, invalidate_cache(_)) - .WillOnce(CompleteContext(r, NULL)); + EXPECT_CALL(mock_image_ctx, invalidate_cache(false, _)) + .WillOnce(WithArg<1>(CompleteContext(r, NULL))); expect_op_work_queue(mock_image_ctx); } diff --git a/src/test/librbd/operation/test_mock_SnapshotRollbackRequest.cc b/src/test/librbd/operation/test_mock_SnapshotRollbackRequest.cc index addb32b279b26..4000eb9b05203 100644 --- a/src/test/librbd/operation/test_mock_SnapshotRollbackRequest.cc +++ b/src/test/librbd/operation/test_mock_SnapshotRollbackRequest.cc @@ -165,8 +165,8 @@ public: void expect_invalidate_cache(MockOperationImageCtx &mock_image_ctx, int r) { if (mock_image_ctx.object_cacher != nullptr) { - EXPECT_CALL(mock_image_ctx, invalidate_cache(_)) - .WillOnce(CompleteContext(r, NULL)); + EXPECT_CALL(mock_image_ctx, invalidate_cache(true, _)) + .WillOnce(WithArg<1>(CompleteContext(r, NULL))); } } -- 2.39.5