From 2d17abb8f2bc337d99df600051e2b47d4c23b3ed Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Fri, 25 Sep 2020 10:40:32 -0400 Subject: [PATCH] librbd: deep-copy should update object-map before writing to object For the original use-case of RBD mirroring it was (maybe) more acceptable to write to the object before updating the object map because an interrupted sync will be retried. However, when using the deep-copy object copy state machine as part of copyup, it's more likely that the object-map has the potential to become out-of-sync with reality if it's updated after the object is written. Signed-off-by: Jason Dillaman (cherry picked from commit e782b85bfda8ae6487c637af0059ab94fba332d6) Conflicts: src/librbd/deep_copy/ObjectCopyRequest.cc: trivial resolution src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc: trivial resolution --- src/librbd/deep_copy/ObjectCopyRequest.cc | 155 +++++++++--------- src/librbd/deep_copy/ObjectCopyRequest.h | 14 +- .../deep_copy/test_mock_ObjectCopyRequest.cc | 83 ++++++---- 3 files changed, 138 insertions(+), 114 deletions(-) diff --git a/src/librbd/deep_copy/ObjectCopyRequest.cc b/src/librbd/deep_copy/ObjectCopyRequest.cc index 4e8d23d53fb6a..dd5da5f31a16d 100644 --- a/src/librbd/deep_copy/ObjectCopyRequest.cc +++ b/src/librbd/deep_copy/ObjectCopyRequest.cc @@ -307,10 +307,87 @@ void ObjectCopyRequest::handle_read_from_parent(int r) { return; } - send_write_object(); + send_update_object_map(); return; } +template +void ObjectCopyRequest::send_update_object_map() { + if (!m_dst_image_ctx->test_features(RBD_FEATURE_OBJECT_MAP) || + m_dst_object_state.empty()) { + send_write_object(); + return; + } + + m_dst_image_ctx->owner_lock.lock_shared(); + m_dst_image_ctx->image_lock.lock_shared(); + if (m_dst_image_ctx->object_map == nullptr) { + // possible that exclusive lock was lost in background + lderr(m_cct) << "object map is not initialized" << dendl; + + m_dst_image_ctx->image_lock.unlock_shared(); + m_dst_image_ctx->owner_lock.unlock_shared(); + finish(-EINVAL); + return; + } + + auto &dst_object_state = *m_dst_object_state.begin(); + auto it = m_snap_map.find(dst_object_state.first); + ceph_assert(it != m_snap_map.end()); + auto dst_snap_id = it->second.front(); + auto object_state = dst_object_state.second; + m_dst_object_state.erase(m_dst_object_state.begin()); + + ldout(m_cct, 20) << "dst_snap_id=" << dst_snap_id << ", object_state=" + << static_cast(object_state) << dendl; + + int r; + auto finish_op_ctx = start_lock_op(m_dst_image_ctx->owner_lock, &r); + if (finish_op_ctx == nullptr) { + lderr(m_cct) << "lost exclusive lock" << dendl; + m_dst_image_ctx->image_lock.unlock_shared(); + m_dst_image_ctx->owner_lock.unlock_shared(); + finish(r); + return; + } + + auto ctx = new LambdaContext([this, finish_op_ctx](int r) { + handle_update_object_map(r); + finish_op_ctx->complete(0); + }); + + auto dst_image_ctx = m_dst_image_ctx; + bool sent = dst_image_ctx->object_map->template aio_update< + Context, &Context::complete>(dst_snap_id, m_dst_object_number, object_state, + {}, {}, false, ctx); + + // NOTE: state machine might complete before we reach here + dst_image_ctx->image_lock.unlock_shared(); + dst_image_ctx->owner_lock.unlock_shared(); + if (!sent) { + ceph_assert(dst_snap_id == CEPH_NOSNAP); + ctx->complete(0); + } +} + +template +void ObjectCopyRequest::handle_update_object_map(int r) { + ldout(m_cct, 20) << "r=" << r << dendl; + + if (r < 0) { + lderr(m_cct) << "failed to update object map: " << cpp_strerror(r) << dendl; + finish(r); + return; + } + + if (!m_dst_object_state.empty()) { + send_update_object_map(); + return; + } + + send_write_object(); +} + template void ObjectCopyRequest::send_write_object() { ceph_assert(!m_write_ops.empty()); @@ -444,82 +521,6 @@ void ObjectCopyRequest::handle_write_object(int r) { return; } - send_update_object_map(); -} - -template -void ObjectCopyRequest::send_update_object_map() { - if (!m_dst_image_ctx->test_features(RBD_FEATURE_OBJECT_MAP) || - m_dst_object_state.empty()) { - finish(0); - return; - } - - m_dst_image_ctx->owner_lock.lock_shared(); - m_dst_image_ctx->image_lock.lock_shared(); - if (m_dst_image_ctx->object_map == nullptr) { - // possible that exclusive lock was lost in background - lderr(m_cct) << "object map is not initialized" << dendl; - - m_dst_image_ctx->image_lock.unlock_shared(); - m_dst_image_ctx->owner_lock.unlock_shared(); - finish(-EINVAL); - return; - } - - auto &dst_object_state = *m_dst_object_state.begin(); - auto it = m_snap_map.find(dst_object_state.first); - ceph_assert(it != m_snap_map.end()); - auto dst_snap_id = it->second.front(); - auto object_state = dst_object_state.second; - m_dst_object_state.erase(m_dst_object_state.begin()); - - ldout(m_cct, 20) << "dst_snap_id=" << dst_snap_id << ", object_state=" - << static_cast(object_state) << dendl; - - int r; - auto finish_op_ctx = start_lock_op(m_dst_image_ctx->owner_lock, &r); - if (finish_op_ctx == nullptr) { - lderr(m_cct) << "lost exclusive lock" << dendl; - m_dst_image_ctx->image_lock.unlock_shared(); - m_dst_image_ctx->owner_lock.unlock_shared(); - finish(r); - return; - } - - auto ctx = new LambdaContext([this, finish_op_ctx](int r) { - handle_update_object_map(r); - finish_op_ctx->complete(0); - }); - - auto dst_image_ctx = m_dst_image_ctx; - bool sent = dst_image_ctx->object_map->template aio_update< - Context, &Context::complete>(dst_snap_id, m_dst_object_number, object_state, - {}, {}, false, ctx); - - // NOTE: state machine might complete before we reach here - dst_image_ctx->image_lock.unlock_shared(); - dst_image_ctx->owner_lock.unlock_shared(); - if (!sent) { - ceph_assert(dst_snap_id == CEPH_NOSNAP); - ctx->complete(0); - } -} - -template -void ObjectCopyRequest::handle_update_object_map(int r) { - ldout(m_cct, 20) << "r=" << r << dendl; - - if (r < 0) { - lderr(m_cct) << "failed to update object map: " << cpp_strerror(r) << dendl; - finish(r); - return; - } - - if (!m_dst_object_state.empty()) { - send_update_object_map(); - return; - } finish(0); } diff --git a/src/librbd/deep_copy/ObjectCopyRequest.h b/src/librbd/deep_copy/ObjectCopyRequest.h index 5d57a8b32c222..c39c33b9b6326 100644 --- a/src/librbd/deep_copy/ObjectCopyRequest.h +++ b/src/librbd/deep_copy/ObjectCopyRequest.h @@ -80,13 +80,13 @@ private: * | /-----------\ * | | | (repeat for each snapshot) * v v | - * WRITE_OBJECT --------/ - * | + * UPDATE_OBJECT_MAP ---/ (skip if object + * | map disabled) * | /-----------\ * | | | (repeat for each snapshot) * v v | - * UPDATE_OBJECT_MAP ---/ (skip if object - * | map disabled) + * WRITE_OBJECT --------/ + * | * v * * @@ -185,12 +185,12 @@ private: void send_read_from_parent(); void handle_read_from_parent(int r); - void send_write_object(); - void handle_write_object(int r); - void send_update_object_map(); void handle_update_object_map(int r); + void send_write_object(); + void handle_write_object(int r); + Context *start_lock_op(ceph::shared_mutex &owner_lock, int* r); uint64_t src_to_dst_object_offset(uint64_t objectno, uint64_t offset); diff --git a/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc b/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc index c2e03db37b715..dc1c4f323c1af 100644 --- a/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc +++ b/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc @@ -526,10 +526,10 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Write) { expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[0]); expect_sparse_read(mock_src_io_ctx, 0, one.range_end(), 0); expect_start_op(mock_exclusive_lock); - expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0); - expect_start_op(mock_exclusive_lock); expect_update_object_map(mock_dst_image_ctx, mock_object_map, m_dst_snap_ids[0], OBJECT_EXISTS, 0); + expect_start_op(mock_exclusive_lock); + expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0); request->send(); ASSERT_EQ(0, ctx.wait()); @@ -581,19 +581,20 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, ReadMissingStaleSnapSet) { InSequence seq; expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, dummy_snap_set1); + expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[3]); expect_sparse_read(mock_src_io_ctx, 0, 123, -ENOENT); + expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, dummy_snap_set2); + expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[3]); expect_sparse_read(mock_src_io_ctx, 0, 234, -ENOENT); + expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, 0); + expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[3]); expect_sparse_read(mock_src_io_ctx, 0, one.range_end(), 0); - expect_start_op(mock_exclusive_lock); - expect_write(mock_dst_io_ctx, 0, one.range_end(), - {m_dst_snap_ids[1], {m_dst_snap_ids[1], - m_dst_snap_ids[0]}}, - 0); + expect_start_op(mock_exclusive_lock); expect_update_object_map(mock_dst_image_ctx, mock_object_map, m_dst_snap_ids[2], OBJECT_EXISTS, 0); @@ -602,6 +603,12 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, ReadMissingStaleSnapSet) { m_dst_snap_ids[3], is_fast_diff(mock_dst_image_ctx) ? OBJECT_EXISTS_CLEAN : OBJECT_EXISTS, 0); + expect_start_op(mock_exclusive_lock); + expect_write(mock_dst_io_ctx, 0, one.range_end(), + {m_dst_snap_ids[1], {m_dst_snap_ids[1], + m_dst_snap_ids[0]}}, + 0); + request->send(); ASSERT_EQ(0, ctx.wait()); ASSERT_EQ(0, compare_objects()); @@ -635,6 +642,7 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, ReadMissingUpToDateSnapMap) { InSequence seq; expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, 0); expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[0]); + expect_sparse_read(mock_src_io_ctx, 0, one.range_end(), -ENOENT); expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, 0); @@ -708,6 +716,11 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteError) { expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, 0); expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[0]); expect_sparse_read(mock_src_io_ctx, 0, one.range_end(), 0); + + expect_start_op(mock_exclusive_lock); + expect_update_object_map(mock_dst_image_ctx, mock_object_map, + m_dst_snap_ids[0], OBJECT_EXISTS, 0); + expect_start_op(mock_exclusive_lock); expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, -EINVAL); @@ -756,15 +769,13 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnaps) { InSequence seq; expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, 0); + expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[0]); expect_sparse_read(mock_src_io_ctx, 0, one.range_end(), 0); + expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[2]); expect_sparse_read(mock_src_io_ctx, two, 0); - expect_start_op(mock_exclusive_lock); - expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0); - expect_start_op(mock_exclusive_lock); - expect_write(mock_dst_io_ctx, two, - {m_dst_snap_ids[0], {m_dst_snap_ids[0]}}, 0); + expect_start_op(mock_exclusive_lock); expect_update_object_map(mock_dst_image_ctx, mock_object_map, m_dst_snap_ids[0], OBJECT_EXISTS, 0); @@ -776,6 +787,12 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnaps) { m_dst_snap_ids[2], is_fast_diff(mock_dst_image_ctx) ? OBJECT_EXISTS_CLEAN : OBJECT_EXISTS, 0); + expect_start_op(mock_exclusive_lock); + expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0); + expect_start_op(mock_exclusive_lock); + expect_write(mock_dst_io_ctx, two, + {m_dst_snap_ids[0], {m_dst_snap_ids[0]}}, 0); + request->send(); ASSERT_EQ(0, ctx.wait()); ASSERT_EQ(0, compare_objects()); @@ -822,12 +839,10 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Trim) { InSequence seq; expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, 0); + expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[0]); expect_sparse_read(mock_src_io_ctx, 0, one.range_end(), 0); - expect_start_op(mock_exclusive_lock); - expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0); - expect_start_op(mock_exclusive_lock); - expect_truncate(mock_dst_io_ctx, trim_offset, 0); + expect_start_op(mock_exclusive_lock); expect_update_object_map(mock_dst_image_ctx, mock_object_map, m_dst_snap_ids[0], OBJECT_EXISTS, 0); @@ -835,6 +850,11 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Trim) { expect_update_object_map(mock_dst_image_ctx, mock_object_map, m_dst_snap_ids[1], OBJECT_EXISTS, 0); + expect_start_op(mock_exclusive_lock); + expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0); + expect_start_op(mock_exclusive_lock); + expect_truncate(mock_dst_io_ctx, trim_offset, 0); + request->send(); ASSERT_EQ(0, ctx.wait()); ASSERT_EQ(0, compare_objects()); @@ -877,11 +897,9 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Remove) { InSequence seq; expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, 0); expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[1]); + expect_sparse_read(mock_src_io_ctx, 0, one.range_end(), 0); - expect_start_op(mock_exclusive_lock); - expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0); - expect_start_op(mock_exclusive_lock); - expect_remove(mock_dst_io_ctx, 0); + expect_start_op(mock_exclusive_lock); uint8_t state = OBJECT_EXISTS; expect_update_object_map(mock_dst_image_ctx, mock_object_map, @@ -891,6 +909,11 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Remove) { m_dst_snap_ids[1], is_fast_diff(mock_dst_image_ctx) ? OBJECT_EXISTS_CLEAN : OBJECT_EXISTS, 0); + expect_start_op(mock_exclusive_lock); + expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0); + expect_start_op(mock_exclusive_lock); + expect_remove(mock_dst_io_ctx, 0); + request->send(); ASSERT_EQ(0, ctx.wait()); ASSERT_EQ(0, compare_objects()); @@ -928,10 +951,10 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, ObjectMapUpdateError) { InSequence seq; expect_list_snaps(mock_src_image_ctx, mock_src_io_ctx, 0); + expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[0]); expect_sparse_read(mock_src_io_ctx, 0, one.range_end(), 0); - expect_start_op(mock_exclusive_lock); - expect_write(mock_dst_io_ctx, 0, one.range_end(), {0, {}}, 0); + expect_start_op(mock_exclusive_lock); expect_update_object_map(mock_dst_image_ctx, mock_object_map, m_dst_snap_ids[0], OBJECT_EXISTS, -EBLACKLISTED); @@ -1013,14 +1036,6 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnapsStart) { expect_set_snap_read(mock_src_io_ctx, m_src_snap_ids[2]); expect_sparse_read(mock_src_io_ctx, three, 0); - expect_start_op(mock_exclusive_lock); - expect_write(mock_dst_io_ctx, two, - {m_dst_snap_ids[0], {m_dst_snap_ids[0]}}, 0); - - expect_start_op(mock_exclusive_lock); - expect_write(mock_dst_io_ctx, three, - {m_dst_snap_ids[1], {m_dst_snap_ids[1], m_dst_snap_ids[0]}}, 0); - expect_start_op(mock_exclusive_lock); expect_update_object_map(mock_dst_image_ctx, mock_object_map, m_dst_snap_ids[1], OBJECT_EXISTS, 0); @@ -1029,6 +1044,14 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnapsStart) { expect_update_object_map(mock_dst_image_ctx, mock_object_map, CEPH_NOSNAP, OBJECT_EXISTS, 0); + expect_start_op(mock_exclusive_lock); + expect_write(mock_dst_io_ctx, two, + {m_dst_snap_ids[0], {m_dst_snap_ids[0]}}, 0); + + expect_start_op(mock_exclusive_lock); + expect_write(mock_dst_io_ctx, three, + {m_dst_snap_ids[1], {m_dst_snap_ids[1], m_dst_snap_ids[0]}}, 0); + request->send(); ASSERT_EQ(0, ctx.wait()); ASSERT_EQ(0, compare_objects()); -- 2.39.5