From: Jason Dillaman Date: Fri, 25 Sep 2020 14:40:32 +0000 (-0400) Subject: librbd: deep-copy should update object-map before writing to object X-Git-Tag: v17.0.0~865^2~8 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=e782b85bfda8ae6487c637af0059ab94fba332d6;p=ceph.git 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 --- diff --git a/src/librbd/deep_copy/ObjectCopyRequest.cc b/src/librbd/deep_copy/ObjectCopyRequest.cc index 569bb7e75a8bc..e423ccab1bace 100644 --- a/src/librbd/deep_copy/ObjectCopyRequest.cc +++ b/src/librbd/deep_copy/ObjectCopyRequest.cc @@ -124,7 +124,7 @@ void ObjectCopyRequest::send_read() { return; } - send_write_object(); + send_update_object_map(); return; } @@ -190,6 +190,83 @@ void ObjectCopyRequest::handle_read(int r) { send_read(); } +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()); @@ -317,82 +394,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 e45685e796e5d..e126481326027 100644 --- a/src/librbd/deep_copy/ObjectCopyRequest.h +++ b/src/librbd/deep_copy/ObjectCopyRequest.h @@ -74,13 +74,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 * * @@ -159,12 +159,12 @@ private: void send_read(); void handle_read(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); void compute_read_ops(); diff --git a/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc b/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc index 72c194f87bb4c..e6d569718d839 100644 --- a/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc +++ b/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc @@ -516,10 +516,10 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Write) { expect_list_snaps(mock_src_image_ctx, 0); expect_read(mock_src_image_ctx, m_src_snap_ids[0], 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()); @@ -587,6 +587,11 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteError) { InSequence seq; expect_list_snaps(mock_src_image_ctx, 0); expect_read(mock_src_image_ctx, m_src_snap_ids[0], 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); @@ -636,11 +641,6 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnaps) { expect_read(mock_src_image_ctx, m_src_snap_ids[0], 0, one.range_end(), 0); expect_read(mock_src_image_ctx, m_src_snap_ids[2], 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); expect_start_op(mock_exclusive_lock); @@ -650,6 +650,11 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnaps) { expect_update_object_map(mock_dst_image_ctx, mock_object_map, 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()); @@ -697,15 +702,15 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Trim) { expect_list_snaps(mock_src_image_ctx, 0); expect_read(mock_src_image_ctx, m_src_snap_ids[0], 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); 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); + 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()); @@ -748,10 +753,7 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, Remove) { InSequence seq; expect_list_snaps(mock_src_image_ctx, 0); expect_read(mock_src_image_ctx, m_src_snap_ids[1], 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, @@ -761,6 +763,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()); @@ -791,15 +798,10 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, ObjectMapUpdateError) { mock_dst_image_ctx, 0, 0, &ctx); - librados::MockTestMemIoCtxImpl &mock_dst_io_ctx(get_mock_io_ctx( - request->get_dst_io_ctx())); - InSequence seq; expect_list_snaps(mock_src_image_ctx, 0); expect_read(mock_src_image_ctx, m_src_snap_ids[0], 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, -EBLOCKLISTED); @@ -873,14 +875,6 @@ TEST_F(TestMockDeepCopyObjectCopyRequest, WriteSnapsStart) { expect_read(mock_src_image_ctx, m_src_snap_ids[1], two, 0); expect_read(mock_src_image_ctx, m_src_snap_ids[2], 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); @@ -889,6 +883,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());