From 680efbdcc3d2e28b6de991e7ad5c18e0d71cc551 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Wed, 1 Jun 2016 23:19:20 -0400 Subject: [PATCH] librbd: refresh image after acquiring exclusive lock It's possible that the object map or journaling features have been disabled while the lock was not owned. Fixes: http://tracker.ceph.com/issues/16114 Signed-off-by: Jason Dillaman (cherry picked from commit 03c54f52d15d6283c630bac6f75427e6829f7d0a) --- src/librbd/exclusive_lock/AcquireRequest.cc | 34 ++++++++- src/librbd/exclusive_lock/AcquireRequest.h | 57 ++++++++------- .../test_mock_AcquireRequest.cc | 71 +++++++++++++++++++ 3 files changed, 135 insertions(+), 27 deletions(-) diff --git a/src/librbd/exclusive_lock/AcquireRequest.cc b/src/librbd/exclusive_lock/AcquireRequest.cc index 4a4ee6d051124..c91cced0bd1b3 100644 --- a/src/librbd/exclusive_lock/AcquireRequest.cc +++ b/src/librbd/exclusive_lock/AcquireRequest.cc @@ -10,6 +10,7 @@ #include "include/stringify.h" #include "librbd/ExclusiveLock.h" #include "librbd/ImageCtx.h" +#include "librbd/ImageState.h" #include "librbd/ImageWatcher.h" #include "librbd/Journal.h" #include "librbd/ObjectMap.h" @@ -123,7 +124,7 @@ Context *AcquireRequest::handle_lock(int *ret_val) { ldout(cct, 10) << __func__ << ": r=" << *ret_val << dendl; if (*ret_val == 0) { - return send_open_object_map(); + return send_refresh(); } else if (*ret_val != -EBUSY) { lderr(cct) << "failed to lock: " << cpp_strerror(*ret_val) << dendl; return m_on_finish; @@ -133,6 +134,37 @@ Context *AcquireRequest::handle_lock(int *ret_val) { return nullptr; } +template +Context *AcquireRequest::send_refresh() { + if (!m_image_ctx.state->is_refresh_required()) { + return send_open_object_map(); + } + + CephContext *cct = m_image_ctx.cct; + ldout(cct, 10) << __func__ << dendl; + + using klass = AcquireRequest; + Context *ctx = create_context_callback(this); + m_image_ctx.state->refresh(ctx); + return nullptr; +} + +template +Context *AcquireRequest::handle_refresh(int *ret_val) { + CephContext *cct = m_image_ctx.cct; + ldout(cct, 10) << __func__ << ": r=" << *ret_val << dendl; + + if (*ret_val < 0) { + lderr(cct) << "failed to refresh image: " << cpp_strerror(*ret_val) + << dendl; + m_error_result = *ret_val; + send_unlock(); + return nullptr; + } + + return send_open_object_map(); +} + template Context *AcquireRequest::send_open_journal() { // alert caller that we now own the exclusive lock diff --git a/src/librbd/exclusive_lock/AcquireRequest.h b/src/librbd/exclusive_lock/AcquireRequest.h index 9a16fc56e2ab5..4990abbd811c0 100644 --- a/src/librbd/exclusive_lock/AcquireRequest.h +++ b/src/librbd/exclusive_lock/AcquireRequest.h @@ -45,32 +45,34 @@ private: * | | . . . . . . . . . . . . . . . . . . . . . . | * | | . . | * | v v (EBUSY) . | - * \--> LOCK_IMAGE * * * * * * * > GET_LOCKERS . . . . | - * . | | | - * . . . . | | | - * . v v | - * . OPEN_OBJECT_MAP (skip if GET_WATCHERS . . . | - * . | disabled) | . | - * . v v . | - * . . > OPEN_JOURNAL (skip if BLACKLIST . (blacklist | - * . | * disabled) | . disabled) | - * . | * v . | - * . | * * * * * * * * BREAK_LOCK < . . . | - * . v * | | - * . ALLOCATE_JOURNAL_TAG * \-----------------------------/ - * . | * * - * . | * * - * . | v v - * . | CLOSE_JOURNAL - * . | | - * . | v - * . | CLOSE_OBJECT_MAP - * . | | - * . | v - * . | UNLOCK_IMAGE - * . | | - * . v | - * . . > <----------/ + * \--> LOCK_IMAGE * * * * * * * * > GET_LOCKERS . . . . | + * | | | + * v v | + * REFRESH (skip if not GET_WATCHERS | + * | needed) | | + * v v | + * OPEN_OBJECT_MAP (skip if BLACKLIST (skip if blacklist | + * | disabled) | disabled) | + * v v | + * OPEN_JOURNAL (skip if BREAK_LOCK | + * | * disabled) | | + * | * \-----------------------------/ + * | * * * * * * * * + * v * + * ALLOCATE_JOURNAL_TAG * + * | * * + * | * * + * | v v + * | CLOSE_JOURNAL + * | | + * | v + * | CLOSE_OBJECT_MAP + * | | + * | v + * | UNLOCK_IMAGE + * | | + * v | + * <----------/ * * @endverbatim */ @@ -104,6 +106,9 @@ private: void send_lock(); Context *handle_lock(int *ret_val); + Context *send_refresh(); + Context *handle_refresh(int *ret_val); + Context *send_open_journal(); Context *handle_open_journal(int *ret_val); diff --git a/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc b/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc index e4ba0ec81e71b..a86fbb31aca44 100644 --- a/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc +++ b/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc @@ -4,6 +4,7 @@ #include "test/librbd/test_mock_fixture.h" #include "test/librbd/test_support.h" #include "test/librbd/mock/MockImageCtx.h" +#include "test/librbd/mock/MockImageState.h" #include "test/librbd/mock/MockJournal.h" #include "test/librbd/mock/MockJournalPolicy.h" #include "test/librbd/mock/MockObjectMap.h" @@ -58,6 +59,16 @@ public: .WillOnce(Return(r)); } + void expect_is_refresh_required(MockImageCtx &mock_image_ctx, bool required) { + EXPECT_CALL(*mock_image_ctx.state, is_refresh_required()) + .WillOnce(Return(required)); + } + + void expect_refresh(MockImageCtx &mock_image_ctx, int r) { + EXPECT_CALL(*mock_image_ctx.state, refresh(_)) + .WillOnce(CompleteContext(r, mock_image_ctx.image_ctx->op_work_queue)); + } + void expect_create_object_map(MockImageCtx &mock_image_ctx, MockObjectMap *mock_object_map) { EXPECT_CALL(mock_image_ctx, create_object_map(_)) @@ -188,6 +199,7 @@ TEST_F(TestMockExclusiveLockAcquireRequest, Success) { InSequence seq; expect_flush_notifies(mock_image_ctx); expect_lock(mock_image_ctx, 0); + expect_is_refresh_required(mock_image_ctx, false); MockObjectMap mock_object_map; expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true); @@ -212,6 +224,35 @@ TEST_F(TestMockExclusiveLockAcquireRequest, Success) { ASSERT_EQ(0, ctx.wait()); } +TEST_F(TestMockExclusiveLockAcquireRequest, SuccessRefresh) { + REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK); + + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + + MockImageCtx mock_image_ctx(*ictx); + expect_op_work_queue(mock_image_ctx); + + InSequence seq; + expect_flush_notifies(mock_image_ctx); + expect_lock(mock_image_ctx, 0); + expect_is_refresh_required(mock_image_ctx, true); + expect_refresh(mock_image_ctx, 0); + + MockObjectMap mock_object_map; + expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, false); + expect_test_features(mock_image_ctx, RBD_FEATURE_JOURNALING, false); + + C_SaferCond acquire_ctx; + C_SaferCond ctx; + MockAcquireRequest *req = MockAcquireRequest::create(mock_image_ctx, + TEST_COOKIE, + &acquire_ctx, &ctx); + req->send(); + ASSERT_EQ(0, acquire_ctx.wait()); + ASSERT_EQ(0, ctx.wait()); +} + TEST_F(TestMockExclusiveLockAcquireRequest, SuccessJournalDisabled) { REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK); @@ -224,6 +265,7 @@ TEST_F(TestMockExclusiveLockAcquireRequest, SuccessJournalDisabled) { InSequence seq; expect_flush_notifies(mock_image_ctx); expect_lock(mock_image_ctx, 0); + expect_is_refresh_required(mock_image_ctx, false); MockObjectMap mock_object_map; expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true); @@ -254,6 +296,7 @@ TEST_F(TestMockExclusiveLockAcquireRequest, SuccessObjectMapDisabled) { InSequence seq; expect_flush_notifies(mock_image_ctx); expect_lock(mock_image_ctx, 0); + expect_is_refresh_required(mock_image_ctx, false); expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, false); @@ -275,6 +318,31 @@ TEST_F(TestMockExclusiveLockAcquireRequest, SuccessObjectMapDisabled) { ASSERT_EQ(0, ctx.wait()); } +TEST_F(TestMockExclusiveLockAcquireRequest, RefreshError) { + REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK); + + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + + MockImageCtx mock_image_ctx(*ictx); + expect_op_work_queue(mock_image_ctx); + + InSequence seq; + expect_flush_notifies(mock_image_ctx); + expect_lock(mock_image_ctx, 0); + expect_is_refresh_required(mock_image_ctx, true); + expect_refresh(mock_image_ctx, -EINVAL); + expect_unlock(mock_image_ctx, 0); + + C_SaferCond *acquire_ctx = new C_SaferCond(); + C_SaferCond ctx; + MockAcquireRequest *req = MockAcquireRequest::create(mock_image_ctx, + TEST_COOKIE, + acquire_ctx, &ctx); + req->send(); + ASSERT_EQ(-EINVAL, ctx.wait()); +} + TEST_F(TestMockExclusiveLockAcquireRequest, JournalError) { REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK); @@ -287,6 +355,7 @@ TEST_F(TestMockExclusiveLockAcquireRequest, JournalError) { InSequence seq; expect_flush_notifies(mock_image_ctx); expect_lock(mock_image_ctx, 0); + expect_is_refresh_required(mock_image_ctx, false); MockObjectMap *mock_object_map = new MockObjectMap(); expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true); @@ -322,6 +391,7 @@ TEST_F(TestMockExclusiveLockAcquireRequest, AllocateJournalTagError) { InSequence seq; expect_flush_notifies(mock_image_ctx); expect_lock(mock_image_ctx, 0); + expect_is_refresh_required(mock_image_ctx, false); MockObjectMap *mock_object_map = new MockObjectMap(); expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true); @@ -665,6 +735,7 @@ TEST_F(TestMockExclusiveLockAcquireRequest, OpenObjectMapError) { InSequence seq; expect_flush_notifies(mock_image_ctx); expect_lock(mock_image_ctx, 0); + expect_is_refresh_required(mock_image_ctx, false); MockObjectMap *mock_object_map = new MockObjectMap(); expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true); -- 2.39.5