From 94b7d231a6281ad2be962fbd55236ebea04868b9 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Tue, 12 Jan 2016 11:56:24 -0500 Subject: [PATCH] librbd: initialize object map before replaying journal The journal might replay write events which will need to update the object map. Signed-off-by: Jason Dillaman --- src/librbd/exclusive_lock/AcquireRequest.cc | 65 +++++++++++++++---- src/librbd/exclusive_lock/AcquireRequest.h | 31 +++++---- .../test_mock_AcquireRequest.cc | 56 +++++++++++++--- 3 files changed, 120 insertions(+), 32 deletions(-) diff --git a/src/librbd/exclusive_lock/AcquireRequest.cc b/src/librbd/exclusive_lock/AcquireRequest.cc index 8c19282165c11..f0826b1252c2e 100644 --- a/src/librbd/exclusive_lock/AcquireRequest.cc +++ b/src/librbd/exclusive_lock/AcquireRequest.cc @@ -62,7 +62,7 @@ AcquireRequest::AcquireRequest(I &image_ctx, const std::string &cookie, Context *on_finish) : m_image_ctx(image_ctx), m_cookie(cookie), m_on_finish(create_async_context_callback(image_ctx, on_finish)), - m_object_map(nullptr), m_journal(nullptr) { + m_object_map(nullptr), m_journal(nullptr), m_error_result(0) { } template @@ -94,7 +94,7 @@ Context *AcquireRequest::handle_lock(int *ret_val) { ldout(cct, 10) << __func__ << ": r=" << *ret_val << dendl; if (*ret_val == 0) { - return send_open_journal(); + return send_open_object_map(); } else if (*ret_val != -EBUSY) { lderr(cct) << "failed to lock: " << cpp_strerror(*ret_val) << dendl; return m_on_finish; @@ -107,7 +107,8 @@ Context *AcquireRequest::handle_lock(int *ret_val) { template Context *AcquireRequest::send_open_journal() { if (!m_image_ctx.test_features(RBD_FEATURE_JOURNALING)) { - return send_open_object_map(); + apply(); + return m_on_finish; } CephContext *cct = m_image_ctx.cct; @@ -117,6 +118,10 @@ Context *AcquireRequest::send_open_journal() { Context *ctx = create_context_callback( this); m_journal = m_image_ctx.create_journal(); + + // journal playback required object map (if enabled) and itself + apply(); + m_journal->open(ctx); return nullptr; } @@ -128,20 +133,17 @@ Context *AcquireRequest::handle_open_journal(int *ret_val) { if (*ret_val < 0) { lderr(cct) << "failed to open journal: " << cpp_strerror(*ret_val) << dendl; - - delete m_journal; - return m_on_finish; + m_error_result = *ret_val; + return send_unlock_object_map(); } - assert(m_image_ctx.journal == nullptr); - return send_open_object_map(); + return m_on_finish; } template Context *AcquireRequest::send_open_object_map() { if (!m_image_ctx.test_features(RBD_FEATURE_OBJECT_MAP)) { - apply(); - return m_on_finish; + return send_open_journal(); } CephContext *cct = m_image_ctx.cct; @@ -150,6 +152,7 @@ Context *AcquireRequest::send_open_object_map() { using klass = AcquireRequest; Context *ctx = create_context_callback( this); + m_object_map = m_image_ctx.create_object_map(CEPH_NOSNAP); m_object_map->open(ctx); return nullptr; @@ -186,8 +189,38 @@ Context *AcquireRequest::handle_lock_object_map(int *ret_val) { // object map should never result in an error assert(*ret_val == 0); + return send_open_journal(); +} - apply(); +template +Context *AcquireRequest::send_unlock_object_map() { + if (m_object_map == nullptr) { + revert(); + return m_on_finish; + } + + CephContext *cct = m_image_ctx.cct; + ldout(cct, 10) << __func__ << dendl; + + using klass = AcquireRequest; + Context *ctx = create_context_callback< + klass, &klass::handle_unlock_object_map>(this); + m_object_map->unlock(ctx); + return nullptr; +} + +template +Context *AcquireRequest::handle_unlock_object_map(int *ret_val) { + CephContext *cct = m_image_ctx.cct; + ldout(cct, 10) << __func__ << ": r=" << *ret_val << dendl; + + // object map should never result in an error + assert(*ret_val == 0); + + assert(m_error_result < 0); + *ret_val = m_error_result; + + revert(); return m_on_finish; } @@ -397,6 +430,16 @@ void AcquireRequest::apply() { m_image_ctx.journal = m_journal; } +template +void AcquireRequest::revert() { + RWLock::WLocker snap_locker(m_image_ctx.snap_lock); + m_image_ctx.object_map = nullptr; + m_image_ctx.journal = nullptr; + + delete m_object_map; + delete m_journal; +} + } // namespace exclusive_lock } // namespace librbd diff --git a/src/librbd/exclusive_lock/AcquireRequest.h b/src/librbd/exclusive_lock/AcquireRequest.h index 71d4a9dea25c8..e3062156e59f0 100644 --- a/src/librbd/exclusive_lock/AcquireRequest.h +++ b/src/librbd/exclusive_lock/AcquireRequest.h @@ -44,18 +44,19 @@ private: * . | | | * . . . . | | | * . v v | - * . OPEN_JOURNAL . . . GET_WATCHERS . . . | - * . | . | . | - * . v . v . | - * . . > OPEN_OBJECT_MAP . BLACKLIST . (blacklist | - * . | . | . disabled) | - * . v . v . | - * . LOCK_OBJECT_MAP . BREAK_LOCK < . . . | - * . | . | | - * . v . | | - * . . > < . . . . | | - * \-----------------------------/ - * + * . OPEN_OBJECT_MAP GET_WATCHERS . . . | + * . | | . | + * . v v . | + * . LOCK_OBJECT_MAP BLACKLIST . (blacklist | + * . | | . disabled) | + * . v v . | + * . . > OPEN_JOURNAL * * BREAK_LOCK < . . . | + * . | * | | + * . | v | | + * . | UNLOCK_OBJECT_MAP | | + * . | | \-----------------------------/ + * . v | + * . . > <-----/ * @endverbatim */ @@ -79,6 +80,8 @@ private: std::string m_locker_address; uint64_t m_locker_handle; + int m_error_result; + void send_lock(); Context *handle_lock(int *ret_val); @@ -91,6 +94,9 @@ private: Context *send_lock_object_map(); Context *handle_lock_object_map(int *ret_val); + Context *send_unlock_object_map(); + Context *handle_unlock_object_map(int *ret_val); + void send_get_lockers(); Context *handle_get_lockers(int *ret_val); @@ -104,6 +110,7 @@ private: Context *handle_break_lock(int *ret_val); void apply(); + void revert(); }; } // namespace exclusive_lock diff --git a/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc b/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc index 31e17a0385c95..7cf7c9dd9ef5e 100644 --- a/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc +++ b/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc @@ -67,6 +67,12 @@ public: .WillOnce(CompleteContext(0, mock_image_ctx.image_ctx->op_work_queue)); } + void expect_unlock_object_map(MockImageCtx &mock_image_ctx, + MockObjectMap &mock_object_map) { + EXPECT_CALL(mock_object_map, unlock(_)) + .WillOnce(CompleteContext(0, mock_image_ctx.image_ctx->op_work_queue)); + } + void expect_create_journal(MockImageCtx &mock_image_ctx, MockJournal *mock_journal) { EXPECT_CALL(mock_image_ctx, create_journal()) @@ -155,17 +161,17 @@ TEST_F(TestMockExclusiveLockAcquireRequest, Success) { InSequence seq; expect_lock(mock_image_ctx, 0); - MockJournal mock_journal; - expect_test_features(mock_image_ctx, RBD_FEATURE_JOURNALING, true); - expect_create_journal(mock_image_ctx, &mock_journal); - expect_open_journal(mock_image_ctx, mock_journal, 0); - MockObjectMap mock_object_map; expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true); expect_create_object_map(mock_image_ctx, &mock_object_map); expect_open_object_map(mock_image_ctx, mock_object_map); expect_lock_object_map(mock_image_ctx, mock_object_map); + MockJournal mock_journal; + expect_test_features(mock_image_ctx, RBD_FEATURE_JOURNALING, true); + expect_create_journal(mock_image_ctx, &mock_journal); + expect_open_journal(mock_image_ctx, mock_journal, 0); + C_SaferCond ctx; MockAcquireRequest *req = MockAcquireRequest::create(mock_image_ctx, TEST_COOKIE, @@ -186,14 +192,14 @@ TEST_F(TestMockExclusiveLockAcquireRequest, SuccessJournalDisabled) { InSequence seq; expect_lock(mock_image_ctx, 0); - expect_test_features(mock_image_ctx, RBD_FEATURE_JOURNALING, false); - MockObjectMap mock_object_map; expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true); expect_create_object_map(mock_image_ctx, &mock_object_map); expect_open_object_map(mock_image_ctx, mock_object_map); expect_lock_object_map(mock_image_ctx, mock_object_map); + expect_test_features(mock_image_ctx, RBD_FEATURE_JOURNALING, false); + C_SaferCond ctx; MockAcquireRequest *req = MockAcquireRequest::create(mock_image_ctx, TEST_COOKIE, @@ -214,13 +220,13 @@ TEST_F(TestMockExclusiveLockAcquireRequest, SuccessObjectMapDisabled) { InSequence seq; expect_lock(mock_image_ctx, 0); + expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, false); + MockJournal mock_journal; expect_test_features(mock_image_ctx, RBD_FEATURE_JOURNALING, true); expect_create_journal(mock_image_ctx, &mock_journal); expect_open_journal(mock_image_ctx, mock_journal, 0); - expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, false); - C_SaferCond ctx; MockAcquireRequest *req = MockAcquireRequest::create(mock_image_ctx, TEST_COOKIE, @@ -229,6 +235,38 @@ TEST_F(TestMockExclusiveLockAcquireRequest, SuccessObjectMapDisabled) { ASSERT_EQ(0, ctx.wait()); } +TEST_F(TestMockExclusiveLockAcquireRequest, JournalError) { + 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_lock(mock_image_ctx, 0); + + MockObjectMap *mock_object_map = new MockObjectMap(); + expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true); + expect_create_object_map(mock_image_ctx, mock_object_map); + expect_open_object_map(mock_image_ctx, *mock_object_map); + expect_lock_object_map(mock_image_ctx, *mock_object_map); + + MockJournal *mock_journal = new MockJournal(); + expect_test_features(mock_image_ctx, RBD_FEATURE_JOURNALING, true); + expect_create_journal(mock_image_ctx, mock_journal); + expect_open_journal(mock_image_ctx, *mock_journal, -EINVAL); + expect_unlock_object_map(mock_image_ctx, *mock_object_map); + + C_SaferCond ctx; + MockAcquireRequest *req = MockAcquireRequest::create(mock_image_ctx, + TEST_COOKIE, + &ctx); + req->send(); + ASSERT_EQ(-EINVAL, ctx.wait()); +} + TEST_F(TestMockExclusiveLockAcquireRequest, LockBusy) { REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK); -- 2.39.5