From: Jason Dillaman Date: Thu, 19 Jul 2018 16:34:59 +0000 (-0400) Subject: librbd: deep-copy should not write to objects that cannot exist X-Git-Tag: v14.0.1~794^2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=fbd6ea1ffd4befe0b13213be9bc17808ff89a85e;p=ceph.git librbd: deep-copy should not write to objects that cannot exist If an image has a snapshot and is subsequently shrunk, ensure that discard ops are not sent to the deep-copy objects beyond the bounds of the image at a given snapshot/HEAD. Only a remove op should be expected in such cases. Fixes: http://tracker.ceph.com/issues/25000 Signed-off-by: Jason Dillaman --- diff --git a/src/librbd/deep_copy/ObjectCopyRequest.cc b/src/librbd/deep_copy/ObjectCopyRequest.cc index 440cdbed035d..19072aee7eb1 100644 --- a/src/librbd/deep_copy/ObjectCopyRequest.cc +++ b/src/librbd/deep_copy/ObjectCopyRequest.cc @@ -61,6 +61,7 @@ ObjectCopyRequest::ObjectCopyRequest(I *src_image_ctx, ldout(m_cct, 20) << "dst_oid=" << m_dst_oid << dendl; compute_src_object_extents(); + compute_dst_object_may_exist(); } template @@ -294,6 +295,7 @@ void ObjectCopyRequest::handle_read_from_parent(int r) { template void ObjectCopyRequest::send_write_object() { assert(!m_write_ops.empty()); + auto& copy_ops = m_write_ops.begin()->second; // retrieve the destination snap context for the op SnapIds dst_snap_ids; @@ -303,6 +305,15 @@ void ObjectCopyRequest::send_write_object() { auto snap_map_it = m_snap_map.find(src_snap_seq); assert(snap_map_it != m_snap_map.end()); + auto dst_snap_id = snap_map_it->second.front(); + auto dst_may_exist_it = m_dst_object_may_exist.find(dst_snap_id); + assert(dst_may_exist_it != m_dst_object_may_exist.end()); + if (!dst_may_exist_it->second && !copy_ops.empty()) { + // if the object cannot exist, the only valid op is to remove it + assert(copy_ops.size() == 1U); + assert(copy_ops.begin()->type == COPY_OP_TYPE_REMOVE); + } + // write snapshot context should be before actual snapshot if (snap_map_it != m_snap_map.begin()) { --snap_map_it; @@ -318,7 +329,7 @@ void ObjectCopyRequest::send_write_object() { librados::ObjectWriteOperation op; uint64_t buffer_offset; - for (auto ©_op : m_write_ops.begin()->second) { + for (auto ©_op : copy_ops) { switch (copy_op.type) { case COPY_OP_TYPE_WRITE: buffer_offset = 0; @@ -822,12 +833,22 @@ void ObjectCopyRequest::compute_zero_ops() { auto src_snap_seq = it.first; auto &zero_interval = it.second; + auto snap_map_it = m_snap_map.find(src_snap_seq); + assert(snap_map_it != m_snap_map.end()); + auto dst_snap_seq = snap_map_it->second.front(); + + auto dst_may_exist_it = m_dst_object_may_exist.find(dst_snap_seq); + assert(dst_may_exist_it != m_dst_object_may_exist.end()); + if (!dst_may_exist_it->second && prev_end_size > 0) { + ldout(m_cct, 5) << "object DNE for snap_id: " << dst_snap_seq << dendl; + m_write_ops[src_snap_seq].emplace_back(COPY_OP_TYPE_REMOVE, 0, 0, 0); + prev_end_size = 0; + continue; + } + if (hide_parent) { RWLock::RLocker snap_locker(m_dst_image_ctx->snap_lock); RWLock::RLocker parent_locker(m_dst_image_ctx->parent_lock); - auto snap_map_it = m_snap_map.find(src_snap_seq); - assert(snap_map_it != m_snap_map.end()); - auto dst_snap_seq = snap_map_it->second.front(); uint64_t parent_overlap = 0; int r = m_dst_image_ctx->get_parent_overlap(dst_snap_seq, &parent_overlap); if (r < 0) { @@ -920,6 +941,19 @@ void ObjectCopyRequest::finish(int r) { on_finish->complete(r); } +template +void ObjectCopyRequest::compute_dst_object_may_exist() { + RWLock::RLocker snap_locker(m_dst_image_ctx->snap_lock); + + auto snap_ids = m_dst_image_ctx->snaps; + snap_ids.push_back(CEPH_NOSNAP); + + for (auto snap_id : snap_ids) { + m_dst_object_may_exist[snap_id] = + (m_dst_object_number < m_dst_image_ctx->get_object_count(snap_id)); + } +} + } // namespace deep_copy } // namespace librbd diff --git a/src/librbd/deep_copy/ObjectCopyRequest.h b/src/librbd/deep_copy/ObjectCopyRequest.h index 0d6598b08c50..e514d539876a 100644 --- a/src/librbd/deep_copy/ObjectCopyRequest.h +++ b/src/librbd/deep_copy/ObjectCopyRequest.h @@ -161,6 +161,7 @@ private: std::map> m_zero_interval; std::map> m_dst_zero_interval; std::map m_dst_object_state; + std::map m_dst_object_may_exist; bufferlist m_read_from_parent_data; void send_list_snaps(); @@ -188,6 +189,8 @@ private: void merge_write_ops(); void compute_zero_ops(); + void compute_dst_object_may_exist(); + void finish(int r); }; diff --git a/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc b/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc index cfefb694ec94..0a1afcd1819d 100644 --- a/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc +++ b/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc @@ -147,6 +147,13 @@ public: mock_image_ctx.exclusive_lock = &mock_exclusive_lock; } + void expect_get_object_count(librbd::MockImageCtx& mock_image_ctx) { + EXPECT_CALL(mock_image_ctx, get_object_count(_)) + .WillRepeatedly(Invoke([&mock_image_ctx](librados::snap_t snap_id) { + return mock_image_ctx.image_ctx->get_object_count(snap_id); + })); + } + void expect_test_features(librbd::MockImageCtx &mock_image_ctx) { EXPECT_CALL(mock_image_ctx, test_features(_)) .WillRepeatedly(WithArg<0>(Invoke([&mock_image_ctx](uint64_t features) { @@ -198,6 +205,7 @@ public: librbd::MockTestImageCtx &mock_src_image_ctx, librbd::MockTestImageCtx &mock_dst_image_ctx, Context *on_finish) { expect_get_object_name(mock_dst_image_ctx); + expect_get_object_count(mock_dst_image_ctx); return new MockObjectCopyRequest(&mock_src_image_ctx, nullptr, &mock_dst_image_ctx, m_snap_map, 0, false, on_finish);