From 46ba56025c283a70b0f384ef38f47542f0885c5b Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Tue, 21 Feb 2017 13:09:39 -0500 Subject: [PATCH] rbd-mirror: object copy should always reference valid snapshots If a remote snapshot is deleted while an image sync is in-progress, associate the read request against the most recent, valid remote snapshot for a given snapshot object clone. Signed-off-by: Jason Dillaman (cherry picked from commit 9a91efc3047963364944f8be91cee8e8f6afc49a) --- src/librados/snap_set_diff.cc | 4 +- src/librados/snap_set_diff.h | 2 +- src/librbd/DiffIterate.cc | 3 +- .../image_sync/ObjectCopyRequest.cc | 38 +++++++++++++------ .../rbd_mirror/image_sync/ObjectCopyRequest.h | 3 +- 5 files changed, 35 insertions(+), 15 deletions(-) diff --git a/src/librados/snap_set_diff.cc b/src/librados/snap_set_diff.cc index 9fb166f5817c5..fcf42d080dabc 100644 --- a/src/librados/snap_set_diff.cc +++ b/src/librados/snap_set_diff.cc @@ -17,7 +17,7 @@ void calc_snap_set_diff(CephContext *cct, const librados::snap_set_t& snap_set, librados::snap_t start, librados::snap_t end, interval_set *diff, uint64_t *end_size, - bool *end_exists) + bool *end_exists, librados::snap_t *clone_end_snap_id) { ldout(cct, 10) << "calc_snap_set_diff start " << start << " end " << end << ", snap_set seq " << snap_set.seq << dendl; @@ -26,6 +26,7 @@ void calc_snap_set_diff(CephContext *cct, const librados::snap_set_t& snap_set, diff->clear(); *end_size = 0; *end_exists = false; + *clone_end_snap_id = 0; for (vector::const_iterator r = snap_set.clones.begin(); r != snap_set.clones.end(); @@ -79,6 +80,7 @@ void calc_snap_set_diff(CephContext *cct, const librados::snap_set_t& snap_set, if (end <= b) { ldout(cct, 20) << " end" << dendl; *end_exists = true; + *clone_end_snap_id = b; break; } diff --git a/src/librados/snap_set_diff.h b/src/librados/snap_set_diff.h index acb71965b52cb..aba03ee317ec4 100644 --- a/src/librados/snap_set_diff.h +++ b/src/librados/snap_set_diff.h @@ -12,6 +12,6 @@ void calc_snap_set_diff(CephContext *cct, const librados::snap_set_t& snap_set, librados::snap_t start, librados::snap_t end, interval_set *diff, uint64_t *end_size, - bool *end_exists); + bool *end_exists, librados::snap_t *clone_end_snap_id); #endif diff --git a/src/librbd/DiffIterate.cc b/src/librbd/DiffIterate.cc index c3b9511330aac..0ef400a3ef7ec 100644 --- a/src/librbd/DiffIterate.cc +++ b/src/librbd/DiffIterate.cc @@ -125,9 +125,10 @@ private: interval_set diff; uint64_t end_size; bool end_exists; + librados::snap_t clone_end_snap_id; calc_snap_set_diff(cct, m_snap_set, m_diff_context.from_snap_id, m_diff_context.end_snap_id, &diff, &end_size, - &end_exists); + &end_exists, &clone_end_snap_id); ldout(cct, 20) << " diff " << diff << " end_exists=" << end_exists << dendl; if (diff.empty()) { diff --git a/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.cc b/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.cc index 900a5f7162400..5ebfa5c38609b 100644 --- a/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.cc +++ b/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.cc @@ -98,7 +98,7 @@ void ObjectCopyRequest::send_read_object() { assert(!sync_ops.empty()); // map the sync op start snap id back to the necessary read snap id - librados::snap_t remote_snap_seq = m_snap_sync_ops.begin()->first; + librados::snap_t remote_snap_seq = m_snap_sync_ops.begin()->first.second; m_remote_io_ctx.snap_set_read(remote_snap_seq); bool read_required = false; @@ -154,7 +154,7 @@ void ObjectCopyRequest::send_write_object() { // retrieve the local snap context for the op SnapIds local_snap_ids; librados::snap_t local_snap_seq = 0; - librados::snap_t remote_snap_seq = m_snap_sync_ops.begin()->first; + librados::snap_t remote_snap_seq = m_snap_sync_ops.begin()->first.first; if (remote_snap_seq != 0) { auto snap_map_it = m_snap_map->find(remote_snap_seq); assert(snap_map_it != m_snap_map->end()); @@ -313,6 +313,7 @@ template void ObjectCopyRequest::compute_diffs() { CephContext *cct = m_local_image_ctx->cct; + librados::snap_t remote_sync_pont_snap_id = m_snap_map->rbegin()->first; uint64_t prev_end_size = 0; bool prev_exists = false; librados::snap_t start_remote_snap_id = 0; @@ -324,12 +325,15 @@ void ObjectCopyRequest::compute_diffs() { interval_set diff; uint64_t end_size; bool exists; + librados::snap_t clone_end_snap_id; calc_snap_set_diff(cct, m_snap_set, start_remote_snap_id, - end_remote_snap_id, &diff, &end_size, &exists); + end_remote_snap_id, &diff, &end_size, &exists, + &clone_end_snap_id); dout(20) << ": " << "start_remote_snap=" << start_remote_snap_id << ", " << "end_remote_snap_id=" << end_remote_snap_id << ", " + << "clone_end_snap_id=" << clone_end_snap_id << ", " << "end_local_snap_id=" << end_local_snap_id << ", " << "diff=" << diff << ", " << "end_size=" << end_size << ", " @@ -350,32 +354,44 @@ void ObjectCopyRequest::compute_diffs() { uint8_t object_state = OBJECT_EXISTS; if (m_local_image_ctx->test_features(RBD_FEATURE_FAST_DIFF, m_local_image_ctx->snap_lock) && - diff.empty() && end_size == prev_end_size) { + prev_exists && diff.empty() && end_size == prev_end_size) { object_state = OBJECT_EXISTS_CLEAN; } m_snap_object_states[end_local_snap_id] = object_state; } + // reads should be issued against the newest (existing) snapshot within + // the associated snapshot object clone. writes should be issued + // against the oldest snapshot in the snap_map. + assert(clone_end_snap_id >= end_remote_snap_id); + if (clone_end_snap_id > remote_sync_pont_snap_id) { + // do not read past the sync point snapshot + clone_end_snap_id = remote_sync_pont_snap_id; + } + // object write/zero, or truncate + // NOTE: a single snapshot clone might represent multiple snapshots, but + // the write/zero and truncate ops will only be associated with the first + // snapshot encountered within the clone since the diff will be empty for + // subsequent snapshots and the size will remain constant for a clone. for (auto it = diff.begin(); it != diff.end(); ++it) { dout(20) << ": read/write op: " << it.get_start() << "~" << it.get_len() << dendl; - m_snap_sync_ops[end_remote_snap_id].emplace_back(SYNC_OP_TYPE_WRITE, - it.get_start(), - it.get_len()); + m_snap_sync_ops[{end_remote_snap_id, clone_end_snap_id}].emplace_back( + SYNC_OP_TYPE_WRITE, it.get_start(), it.get_len()); } if (end_size < prev_end_size) { dout(20) << ": trunc op: " << end_size << dendl; - m_snap_sync_ops[end_remote_snap_id].emplace_back(SYNC_OP_TYPE_TRUNC, - end_size, 0U); + m_snap_sync_ops[{end_remote_snap_id, clone_end_snap_id}].emplace_back( + SYNC_OP_TYPE_TRUNC, end_size, 0U); } m_snap_object_sizes[end_remote_snap_id] = end_size; } else { if (prev_exists) { // object remove dout(20) << ": remove op" << dendl; - m_snap_sync_ops[end_remote_snap_id].emplace_back(SYNC_OP_TYPE_REMOVE, - 0U, 0U); + m_snap_sync_ops[{end_remote_snap_id, end_remote_snap_id}].emplace_back( + SYNC_OP_TYPE_REMOVE, 0U, 0U); } } diff --git a/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.h b/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.h index 2ade599787f11..17545d301987b 100644 --- a/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.h +++ b/src/tools/rbd_mirror/image_sync/ObjectCopyRequest.h @@ -96,7 +96,8 @@ private: }; typedef std::list SyncOps; - typedef std::map SnapSyncOps; + typedef std::pair WriteReadSnapIds; + typedef std::map SnapSyncOps; typedef std::map SnapObjectStates; typedef std::map SnapObjectSizes; -- 2.39.5