From 21d19325f35c6c2ab546618efd6f52cdceea925b Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Wed, 7 Jun 2017 10:52:24 -0400 Subject: [PATCH] rbd-mirror: narrowed scope of exclusive lock op tracking Signed-off-by: Jason Dillaman --- .../image_sync/test_mock_ObjectCopyRequest.cc | 53 ++++++++++++++++++- .../image_sync/ObjectCopyRequest.cc | 51 ++++++++++++++++-- .../rbd_mirror/image_sync/ObjectCopyRequest.h | 3 ++ 3 files changed, 101 insertions(+), 6 deletions(-) diff --git a/src/test/rbd_mirror/image_sync/test_mock_ObjectCopyRequest.cc b/src/test/rbd_mirror/image_sync/test_mock_ObjectCopyRequest.cc index 370c25fb5555c..f70df5c0582f5 100644 --- a/src/test/rbd_mirror/image_sync/test_mock_ObjectCopyRequest.cc +++ b/src/test/rbd_mirror/image_sync/test_mock_ObjectCopyRequest.cc @@ -46,6 +46,7 @@ using ::testing::DoDefault; using ::testing::InSequence; using ::testing::Invoke; using ::testing::Return; +using ::testing::ReturnNew; using ::testing::WithArg; namespace { @@ -88,6 +89,11 @@ public: ASSERT_EQ(0, open_image(m_local_io_ctx, m_image_name, &m_local_image_ctx)); } + void expect_start_op(librbd::MockExclusiveLock &mock_exclusive_lock) { + EXPECT_CALL(mock_exclusive_lock, start_op()).WillOnce( + ReturnNew([](int) {})); + } + void expect_list_snaps(librbd::MockTestImageCtx &mock_image_ctx, librados::MockTestMemIoCtxImpl &mock_io_ctx, const librados::snap_set_t &snap_set) { @@ -346,9 +352,11 @@ TEST_F(TestMockImageSyncObjectCopyRequest, DNE) { librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx); + librbd::MockExclusiveLock mock_exclusive_lock; + mock_local_image_ctx.exclusive_lock = &mock_exclusive_lock; + librbd::MockObjectMap mock_object_map; mock_local_image_ctx.object_map = &mock_object_map; - expect_test_features(mock_local_image_ctx); C_SaferCond ctx; @@ -374,6 +382,9 @@ TEST_F(TestMockImageSyncObjectCopyRequest, Write) { librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx); + librbd::MockExclusiveLock mock_exclusive_lock; + mock_local_image_ctx.exclusive_lock = &mock_exclusive_lock; + librbd::MockObjectMap mock_object_map; mock_local_image_ctx.object_map = &mock_object_map; @@ -392,7 +403,9 @@ TEST_F(TestMockImageSyncObjectCopyRequest, Write) { expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0); expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[0]); expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0); + expect_start_op(mock_exclusive_lock); expect_write(mock_local_io_ctx, 0, one.range_end(), {0, {}}, 0); + expect_start_op(mock_exclusive_lock); expect_update_object_map(mock_local_image_ctx, mock_object_map, m_local_snap_ids[0], OBJECT_EXISTS, 0); @@ -414,6 +427,9 @@ TEST_F(TestMockImageSyncObjectCopyRequest, ReadMissingStaleSnapSet) { librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx); + librbd::MockExclusiveLock mock_exclusive_lock; + mock_local_image_ctx.exclusive_lock = &mock_exclusive_lock; + librbd::MockObjectMap mock_object_map; mock_local_image_ctx.object_map = &mock_object_map; @@ -449,12 +465,15 @@ TEST_F(TestMockImageSyncObjectCopyRequest, ReadMissingStaleSnapSet) { expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0); expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[3]); expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0); + expect_start_op(mock_exclusive_lock); expect_write(mock_local_io_ctx, 0, one.range_end(), {m_local_snap_ids[1], {m_local_snap_ids[1], m_local_snap_ids[0]}}, 0); + expect_start_op(mock_exclusive_lock); expect_update_object_map(mock_local_image_ctx, mock_object_map, m_local_snap_ids[2], OBJECT_EXISTS, 0); + expect_start_op(mock_exclusive_lock); expect_update_object_map(mock_local_image_ctx, mock_object_map, m_local_snap_ids[3], OBJECT_EXISTS_CLEAN, 0); @@ -472,6 +491,9 @@ TEST_F(TestMockImageSyncObjectCopyRequest, ReadMissingUpToDateSnapMap) { librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx); + librbd::MockExclusiveLock mock_exclusive_lock; + mock_local_image_ctx.exclusive_lock = &mock_exclusive_lock; + librbd::MockObjectMap mock_object_map; mock_local_image_ctx.object_map = &mock_object_map; @@ -503,6 +525,9 @@ TEST_F(TestMockImageSyncObjectCopyRequest, ReadError) { librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx); + librbd::MockExclusiveLock mock_exclusive_lock; + mock_local_image_ctx.exclusive_lock = &mock_exclusive_lock; + librbd::MockObjectMap mock_object_map; mock_local_image_ctx.object_map = &mock_object_map; @@ -533,6 +558,9 @@ TEST_F(TestMockImageSyncObjectCopyRequest, WriteError) { librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx); + librbd::MockExclusiveLock mock_exclusive_lock; + mock_local_image_ctx.exclusive_lock = &mock_exclusive_lock; + librbd::MockObjectMap mock_object_map; mock_local_image_ctx.object_map = &mock_object_map; @@ -551,6 +579,7 @@ TEST_F(TestMockImageSyncObjectCopyRequest, WriteError) { expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0); expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[0]); expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0); + expect_start_op(mock_exclusive_lock); expect_write(mock_local_io_ctx, 0, one.range_end(), {0, {}}, -EINVAL); request->send(); @@ -577,6 +606,9 @@ TEST_F(TestMockImageSyncObjectCopyRequest, WriteSnaps) { librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx); + librbd::MockExclusiveLock mock_exclusive_lock; + mock_local_image_ctx.exclusive_lock = &mock_exclusive_lock; + librbd::MockObjectMap mock_object_map; mock_local_image_ctx.object_map = &mock_object_map; @@ -595,15 +627,20 @@ TEST_F(TestMockImageSyncObjectCopyRequest, WriteSnaps) { expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0); expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[0]); expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0); + expect_start_op(mock_exclusive_lock); expect_write(mock_local_io_ctx, 0, one.range_end(), {0, {}}, 0); expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[2]); expect_sparse_read(mock_remote_io_ctx, two, 0); + expect_start_op(mock_exclusive_lock); expect_write(mock_local_io_ctx, two, {m_local_snap_ids[0], {m_local_snap_ids[0]}}, 0); + expect_start_op(mock_exclusive_lock); expect_update_object_map(mock_local_image_ctx, mock_object_map, m_local_snap_ids[0], OBJECT_EXISTS, 0); + expect_start_op(mock_exclusive_lock); expect_update_object_map(mock_local_image_ctx, mock_object_map, m_local_snap_ids[1], OBJECT_EXISTS, 0); + expect_start_op(mock_exclusive_lock); expect_update_object_map(mock_local_image_ctx, mock_object_map, m_local_snap_ids[2], OBJECT_EXISTS_CLEAN, 0); @@ -629,6 +666,9 @@ TEST_F(TestMockImageSyncObjectCopyRequest, Trim) { librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx); + librbd::MockExclusiveLock mock_exclusive_lock; + mock_local_image_ctx.exclusive_lock = &mock_exclusive_lock; + librbd::MockObjectMap mock_object_map; mock_local_image_ctx.object_map = &mock_object_map; @@ -647,10 +687,14 @@ TEST_F(TestMockImageSyncObjectCopyRequest, Trim) { expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0); expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[0]); expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0); + expect_start_op(mock_exclusive_lock); expect_write(mock_local_io_ctx, 0, one.range_end(), {0, {}}, 0); + expect_start_op(mock_exclusive_lock); expect_truncate(mock_local_io_ctx, trim_offset, 0); + expect_start_op(mock_exclusive_lock); expect_update_object_map(mock_local_image_ctx, mock_object_map, m_local_snap_ids[0], OBJECT_EXISTS, 0); + expect_start_op(mock_exclusive_lock); expect_update_object_map(mock_local_image_ctx, mock_object_map, m_local_snap_ids[1], OBJECT_EXISTS, 0); @@ -673,6 +717,9 @@ TEST_F(TestMockImageSyncObjectCopyRequest, Remove) { librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx); + librbd::MockExclusiveLock mock_exclusive_lock; + mock_local_image_ctx.exclusive_lock = &mock_exclusive_lock; + librbd::MockObjectMap mock_object_map; mock_local_image_ctx.object_map = &mock_object_map; @@ -691,10 +738,14 @@ TEST_F(TestMockImageSyncObjectCopyRequest, Remove) { expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0); expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[1]); expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0); + expect_start_op(mock_exclusive_lock); expect_write(mock_local_io_ctx, 0, one.range_end(), {0, {}}, 0); + expect_start_op(mock_exclusive_lock); expect_remove(mock_local_io_ctx, 0); + expect_start_op(mock_exclusive_lock); expect_update_object_map(mock_local_image_ctx, mock_object_map, m_local_snap_ids[0], OBJECT_EXISTS, 0); + expect_start_op(mock_exclusive_lock); expect_update_object_map(mock_local_image_ctx, mock_object_map, m_local_snap_ids[1], OBJECT_EXISTS_CLEAN, 0); diff --git a/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.cc b/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.cc index 92817b7a78850..cc59030878ba3 100644 --- a/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.cc +++ b/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.cc @@ -3,6 +3,7 @@ #include "ObjectCopyRequest.h" #include "librados/snap_set_diff.h" +#include "librbd/ExclusiveLock.h" #include "librbd/ObjectMap.h" #include "librbd/Utils.h" #include "common/errno.h" @@ -210,6 +211,17 @@ void ObjectCopyRequest::send_write_object() { } } + Context *finish_op_ctx; + { + RWLock::RLocker owner_locker(m_local_image_ctx->owner_lock); + finish_op_ctx = start_local_op(m_local_image_ctx->owner_lock); + } + if (finish_op_ctx == nullptr) { + derr << ": lost exclusive lock" << dendl; + finish(-EROFS); + return; + } + dout(20) << ": " << "local_snap_seq=" << local_snap_seq << ", " << "local_snaps=" << local_snap_ids << dendl; @@ -270,8 +282,11 @@ void ObjectCopyRequest::send_write_object() { } } - librados::AioCompletion *comp = create_rados_callback< - ObjectCopyRequest, &ObjectCopyRequest::handle_write_object>(this); + auto ctx = new FunctionContext([this, finish_op_ctx](int r) { + handle_write_object(r); + finish_op_ctx->complete(0); + }); + librados::AioCompletion *comp = create_rados_callback(ctx); int r = m_local_io_ctx.aio_operate(m_local_oid, comp, &op, local_snap_seq, local_snap_ids); assert(r == 0); @@ -303,12 +318,13 @@ void ObjectCopyRequest::handle_write_object(int r) { template void ObjectCopyRequest::send_update_object_map() { - + m_local_image_ctx->owner_lock.get_read(); m_local_image_ctx->snap_lock.get_read(); if (!m_local_image_ctx->test_features(RBD_FEATURE_OBJECT_MAP, m_local_image_ctx->snap_lock) || m_snap_object_states.empty()) { m_local_image_ctx->snap_lock.put_read(); + m_local_image_ctx->owner_lock.put_read(); finish(0); return; } else if (m_local_image_ctx->object_map == nullptr) { @@ -316,6 +332,7 @@ void ObjectCopyRequest::send_update_object_map() { derr << ": object map is not initialized" << dendl; m_local_image_ctx->snap_lock.put_read(); + m_local_image_ctx->owner_lock.put_read(); finish(-EINVAL); return; } @@ -330,13 +347,28 @@ void ObjectCopyRequest::send_update_object_map() { << "object_state=" << static_cast(snap_object_state.second) << dendl; + auto finish_op_ctx = start_local_op(m_local_image_ctx->owner_lock); + if (finish_op_ctx == nullptr) { + derr << ": lost exclusive lock" << dendl; + m_local_image_ctx->snap_lock.put_read(); + m_local_image_ctx->owner_lock.put_read(); + finish(-EROFS); + return; + } + + auto ctx = new FunctionContext([this, finish_op_ctx](int r) { + handle_update_object_map(r); + finish_op_ctx->complete(0); + }); + RWLock::WLocker object_map_locker(m_local_image_ctx->object_map_lock); bool sent = m_local_image_ctx->object_map->template aio_update< - ObjectCopyRequest, &ObjectCopyRequest::handle_update_object_map>( + Context, &Context::complete>( snap_object_state.first, m_object_number, snap_object_state.second, {}, - {}, this); + {}, ctx); assert(sent); m_local_image_ctx->snap_lock.put_read(); + m_local_image_ctx->owner_lock.put_read(); } template @@ -351,6 +383,15 @@ void ObjectCopyRequest::handle_update_object_map(int r) { finish(0); } +template +Context *ObjectCopyRequest::start_local_op(RWLock &owner_lock) { + assert(m_local_image_ctx->owner_lock.is_locked()); + if (m_local_image_ctx->exclusive_lock == nullptr) { + return nullptr; + } + return m_local_image_ctx->exclusive_lock->start_op(); +} + template void ObjectCopyRequest::compute_diffs() { CephContext *cct = m_local_image_ctx->cct; diff --git a/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.h b/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.h index 78068861a5c0c..8430e7030c439 100644 --- a/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.h +++ b/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.h @@ -14,6 +14,7 @@ #include class Context; +class RWLock; namespace rbd { namespace mirror { @@ -136,6 +137,8 @@ private: void send_update_object_map(); void handle_update_object_map(int r); + Context *start_local_op(RWLock &owner_lock); + void compute_diffs(); void finish(int r); -- 2.39.5