From a85fbb4e9a9a409738e9f031c20a8e2beb1b514f Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Mon, 13 Jun 2016 12:00:28 -0400 Subject: [PATCH] librbd: do not shut down exclusive lock while acquiring Fixes: http://tracker.ceph.com/issues/16260 Signed-off-by: Jason Dillaman (cherry picked from commit c5694fc6766fb8e213c4b65d2cd7b9d066b07ff7) --- src/librbd/ImageState.cc | 26 +++++++- src/librbd/ImageState.h | 13 ++-- src/librbd/exclusive_lock/AcquireRequest.cc | 2 +- src/librbd/image/OpenRequest.cc | 2 +- src/librbd/image/RefreshRequest.cc | 17 ++++- src/librbd/image/RefreshRequest.h | 9 ++- .../test_mock_AcquireRequest.cc | 2 +- .../librbd/image/test_mock_RefreshRequest.cc | 62 ++++++++++++++----- src/test/librbd/mock/MockImageState.h | 1 + 9 files changed, 106 insertions(+), 28 deletions(-) diff --git a/src/librbd/ImageState.cc b/src/librbd/ImageState.cc index 0bf2e45f7c15e..39f4ee610664c 100644 --- a/src/librbd/ImageState.cc +++ b/src/librbd/ImageState.cc @@ -105,6 +105,18 @@ template void ImageState::refresh(Context *on_finish) { CephContext *cct = m_image_ctx->cct; ldout(cct, 20) << __func__ << dendl; + refresh(false, on_finish); +} + +template +void ImageState::acquire_lock_refresh(Context *on_finish) { + CephContext *cct = m_image_ctx->cct; + ldout(cct, 20) << __func__ << dendl; + refresh(true, on_finish); +} + +template +void ImageState::refresh(bool acquiring_lock, Context *on_finish) { m_lock.Lock(); if (is_closed()) { @@ -115,6 +127,7 @@ void ImageState::refresh(Context *on_finish) { Action action(ACTION_TYPE_REFRESH); action.refresh_seq = m_refresh_seq; + action.refresh_acquiring_lock = acquiring_lock; execute_action_unlock(action, on_finish); } @@ -326,12 +339,15 @@ void ImageState::send_refresh_unlock() { ldout(cct, 10) << this << " " << __func__ << dendl; m_state = STATE_REFRESHING; + assert(!m_actions_contexts.empty()); + auto &action_context = m_actions_contexts.front().first; + assert(action_context.action_type == ACTION_TYPE_REFRESH); Context *ctx = create_async_context_callback( *m_image_ctx, create_context_callback< ImageState, &ImageState::handle_refresh>(this)); image::RefreshRequest *req = image::RefreshRequest::create( - *m_image_ctx, ctx); + *m_image_ctx, action_context.refresh_acquiring_lock, ctx); m_lock.Unlock(); req->send(); @@ -348,7 +364,13 @@ void ImageState::handle_refresh(int r) { ActionContexts &action_contexts(m_actions_contexts.front()); assert(action_contexts.first.action_type == ACTION_TYPE_REFRESH); assert(m_last_refresh <= action_contexts.first.refresh_seq); - m_last_refresh = action_contexts.first.refresh_seq; + + if (r == -ERESTART) { + ldout(cct, 5) << "incomplete refresh: not updating sequence" << dendl; + r = 0; + } else { + m_last_refresh = action_contexts.first.refresh_seq; + } complete_action_unlock(STATE_OPEN, r); } diff --git a/src/librbd/ImageState.h b/src/librbd/ImageState.h index 2834116ad98dd..b60172f0b0a77 100644 --- a/src/librbd/ImageState.h +++ b/src/librbd/ImageState.h @@ -34,8 +34,9 @@ public: bool is_refresh_required() const; int refresh(); - void refresh(Context *on_finish); int refresh_if_required(); + void refresh(Context *on_finish); + void acquire_lock_refresh(Context *on_finish); void snap_set(const std::string &snap_name, Context *on_finish); @@ -59,10 +60,11 @@ private: struct Action { ActionType action_type; - uint64_t refresh_seq; + uint64_t refresh_seq = 0; + bool refresh_acquiring_lock = false; std::string snap_name; - Action(ActionType action_type) : action_type(action_type), refresh_seq(0) { + Action(ActionType action_type) : action_type(action_type) { } inline bool operator==(const Action &action) const { if (action_type != action.action_type) { @@ -70,7 +72,8 @@ private: } switch (action_type) { case ACTION_TYPE_REFRESH: - return refresh_seq == action.refresh_seq; + return (refresh_seq == action.refresh_seq && + refresh_acquiring_lock == action.refresh_acquiring_lock); case ACTION_TYPE_SET_SNAP: return snap_name == action.snap_name; default: @@ -95,6 +98,8 @@ private: bool is_transition_state() const; bool is_closed() const; + void refresh(bool acquiring_lock, Context *on_finish); + void append_context(const Action &action, Context *context); void execute_next_action_unlock(); void execute_action_unlock(const Action &action, Context *context); diff --git a/src/librbd/exclusive_lock/AcquireRequest.cc b/src/librbd/exclusive_lock/AcquireRequest.cc index c91cced0bd1b3..f030b0ea3bce1 100644 --- a/src/librbd/exclusive_lock/AcquireRequest.cc +++ b/src/librbd/exclusive_lock/AcquireRequest.cc @@ -145,7 +145,7 @@ Context *AcquireRequest::send_refresh() { using klass = AcquireRequest; Context *ctx = create_context_callback(this); - m_image_ctx.state->refresh(ctx); + m_image_ctx.state->acquire_lock_refresh(ctx); return nullptr; } diff --git a/src/librbd/image/OpenRequest.cc b/src/librbd/image/OpenRequest.cc index 41f753e1c378b..7714722cb4e95 100644 --- a/src/librbd/image/OpenRequest.cc +++ b/src/librbd/image/OpenRequest.cc @@ -374,7 +374,7 @@ void OpenRequest::send_refresh() { using klass = OpenRequest; RefreshRequest *ctx = RefreshRequest::create( - *m_image_ctx, + *m_image_ctx, false, create_context_callback(this)); ctx->send(); } diff --git a/src/librbd/image/RefreshRequest.cc b/src/librbd/image/RefreshRequest.cc index dedf3073832a6..ebd86efe7e773 100644 --- a/src/librbd/image/RefreshRequest.cc +++ b/src/librbd/image/RefreshRequest.cc @@ -28,8 +28,9 @@ using util::create_async_context_callback; using util::create_context_callback; template -RefreshRequest::RefreshRequest(I &image_ctx, Context *on_finish) - : m_image_ctx(image_ctx), +RefreshRequest::RefreshRequest(I &image_ctx, bool acquiring_lock, + Context *on_finish) + : m_image_ctx(image_ctx), m_acquiring_lock(acquiring_lock), m_on_finish(create_async_context_callback(m_image_ctx, on_finish)), m_error_result(0), m_flush_aio(false), m_exclusive_lock(nullptr), m_object_map(nullptr), m_journal(nullptr), m_refresh_parent(nullptr) { @@ -271,6 +272,12 @@ Context *RefreshRequest::handle_v2_get_mutable_metadata(int *result) { return m_on_finish; } + if (m_acquiring_lock && (m_features & RBD_FEATURE_EXCLUSIVE_LOCK) == 0) { + ldout(cct, 5) << "ignoring dynamically disabled exclusive lock" << dendl; + m_features |= RBD_FEATURE_EXCLUSIVE_LOCK; + m_incomplete_update = true; + } + send_v2_get_flags(); return nullptr; } @@ -636,6 +643,7 @@ Context *RefreshRequest::handle_v2_apply(int *result) { ldout(cct, 10) << this << " " << __func__ << dendl; apply(); + return send_v2_finalize_refresh_parent(); } @@ -779,6 +787,11 @@ Context *RefreshRequest::handle_v2_close_object_map(int *result) { template Context *RefreshRequest::send_flush_aio() { + if (m_incomplete_update && m_error_result == 0) { + // if this was a partial refresh, notify ImageState + m_error_result = -ERESTART; + } + if (m_flush_aio) { CephContext *cct = m_image_ctx.cct; ldout(cct, 10) << this << " " << __func__ << dendl; diff --git a/src/librbd/image/RefreshRequest.h b/src/librbd/image/RefreshRequest.h index 63c858b9141f9..58b6487e1e767 100644 --- a/src/librbd/image/RefreshRequest.h +++ b/src/librbd/image/RefreshRequest.h @@ -27,11 +27,12 @@ template class RefreshParentRequest; template class RefreshRequest { public: - static RefreshRequest *create(ImageCtxT &image_ctx, Context *on_finish) { - return new RefreshRequest(image_ctx, on_finish); + static RefreshRequest *create(ImageCtxT &image_ctx, bool acquiring_lock, + Context *on_finish) { + return new RefreshRequest(image_ctx, acquiring_lock, on_finish); } - RefreshRequest(ImageCtxT &image_ctx, Context *on_finish); + RefreshRequest(ImageCtxT &image_ctx, bool acquiring_lock, Context *on_finish); ~RefreshRequest(); void send(); @@ -97,6 +98,7 @@ private: */ ImageCtxT &m_image_ctx; + bool m_acquiring_lock; Context *m_on_finish; int m_error_result; @@ -129,6 +131,7 @@ private: bool m_exclusive_locked; bool m_blocked_writes = false; + bool m_incomplete_update = false; void send_v1_read_header(); Context *handle_v1_read_header(int *result); diff --git a/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc b/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc index a86fbb31aca44..c98d69463e9ba 100644 --- a/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc +++ b/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc @@ -65,7 +65,7 @@ public: } void expect_refresh(MockImageCtx &mock_image_ctx, int r) { - EXPECT_CALL(*mock_image_ctx.state, refresh(_)) + EXPECT_CALL(*mock_image_ctx.state, acquire_lock_refresh(_)) .WillOnce(CompleteContext(r, mock_image_ctx.image_ctx->op_work_queue)); } diff --git a/src/test/librbd/image/test_mock_RefreshRequest.cc b/src/test/librbd/image/test_mock_RefreshRequest.cc index 01a7381064628..05034919b149f 100644 --- a/src/test/librbd/image/test_mock_RefreshRequest.cc +++ b/src/test/librbd/image/test_mock_RefreshRequest.cc @@ -308,7 +308,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessV1) { expect_init_layout(mock_image_ctx); C_SaferCond ctx; - MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx); + MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); @@ -334,7 +334,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessSnapshotV1) { expect_add_snap(mock_image_ctx, "snap", ictx->snap_ids.begin()->second); C_SaferCond ctx; - MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx); + MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); @@ -361,7 +361,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessV2) { } C_SaferCond ctx; - MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx); + MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); @@ -392,7 +392,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessSnapshotV2) { expect_add_snap(mock_image_ctx, "snap", ictx->snap_ids.begin()->second); C_SaferCond ctx; - MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx); + MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); @@ -425,7 +425,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessSetSnapshotV2) { expect_get_snap_id(mock_image_ctx, "snap", ictx->snap_ids.begin()->second); C_SaferCond ctx; - MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx); + MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); @@ -475,7 +475,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessChild) { expect_refresh_parent_finalize(mock_image_ctx, *mock_refresh_parent_request, 0); C_SaferCond ctx; - MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx); + MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); @@ -521,12 +521,46 @@ TEST_F(TestMockImageRefreshRequest, DisableExclusiveLock) { expect_shut_down_exclusive_lock(mock_image_ctx, *mock_exclusive_lock, 0); C_SaferCond ctx; - MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx); + MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); } +TEST_F(TestMockImageRefreshRequest, DisableExclusiveLockWhileAcquiringLock) { + REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK); + + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + + MockRefreshImageCtx mock_image_ctx(*ictx); + MockRefreshParentRequest mock_refresh_parent_request; + + MockExclusiveLock mock_exclusive_lock; + mock_image_ctx.exclusive_lock = &mock_exclusive_lock; + + ASSERT_EQ(0, update_features(ictx, + RBD_FEATURE_EXCLUSIVE_LOCK | + RBD_FEATURE_OBJECT_MAP | + RBD_FEATURE_FAST_DIFF | + RBD_FEATURE_JOURNALING, false)); + + expect_op_work_queue(mock_image_ctx); + expect_test_features(mock_image_ctx); + + // verify that exclusive lock is properly handled when object map + // and journaling were never enabled (or active) + InSequence seq; + expect_get_mutable_metadata(mock_image_ctx, 0); + expect_get_flags(mock_image_ctx, 0); + expect_refresh_parent_is_required(mock_refresh_parent_request, false); + + C_SaferCond ctx; + MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, true, &ctx); + req->send(); + + ASSERT_EQ(-ERESTART, ctx.wait()); +} TEST_F(TestMockImageRefreshRequest, EnableJournalWithExclusiveLock) { REQUIRE_FEATURE(RBD_FEATURE_JOURNALING); @@ -557,7 +591,7 @@ TEST_F(TestMockImageRefreshRequest, EnableJournalWithExclusiveLock) { expect_open_journal(mock_image_ctx, mock_journal, 0); C_SaferCond ctx; - MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx); + MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); @@ -591,7 +625,7 @@ TEST_F(TestMockImageRefreshRequest, EnableJournalWithoutExclusiveLock) { expect_set_require_lock_on_read(mock_image_ctx); C_SaferCond ctx; - MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx); + MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); @@ -633,7 +667,7 @@ TEST_F(TestMockImageRefreshRequest, DisableJournal) { expect_unblock_writes(mock_image_ctx); C_SaferCond ctx; - MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx); + MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); @@ -667,7 +701,7 @@ TEST_F(TestMockImageRefreshRequest, EnableObjectMapWithExclusiveLock) { expect_open_object_map(mock_image_ctx, &mock_object_map, 0); C_SaferCond ctx; - MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx); + MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); @@ -698,7 +732,7 @@ TEST_F(TestMockImageRefreshRequest, EnableObjectMapWithoutExclusiveLock) { expect_refresh_parent_is_required(mock_refresh_parent_request, false); C_SaferCond ctx; - MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx); + MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); @@ -739,7 +773,7 @@ TEST_F(TestMockImageRefreshRequest, DisableObjectMap) { expect_close_object_map(mock_image_ctx, *mock_object_map, 0); C_SaferCond ctx; - MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx); + MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); @@ -773,7 +807,7 @@ TEST_F(TestMockImageRefreshRequest, OpenObjectMapError) { expect_open_object_map(mock_image_ctx, mock_object_map, -EFBIG); C_SaferCond ctx; - MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx); + MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, &ctx); req->send(); ASSERT_EQ(0, ctx.wait()); diff --git a/src/test/librbd/mock/MockImageState.h b/src/test/librbd/mock/MockImageState.h index bf341f58033a9..3ba1ee5d7dd0e 100644 --- a/src/test/librbd/mock/MockImageState.h +++ b/src/test/librbd/mock/MockImageState.h @@ -13,6 +13,7 @@ namespace librbd { struct MockImageState { MOCK_CONST_METHOD0(is_refresh_required, bool()); MOCK_METHOD1(refresh, void(Context*)); + MOCK_METHOD1(acquire_lock_refresh, void(Context*)); MOCK_METHOD1(open, void(Context*)); -- 2.39.5