rbd-mirror: snapshot replayer should only unlink peers for unneeded snaps

Once the snapshot will no longer be needed by the peer for delta sync,
it should be unlinked. This will ensure the primary image will not
remove snapshots that will be needed for the next sync point.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>

diff --git a/src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc b/src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc
index 25835d97ec7836c56f6dc80d2b69b84f805bc525..e1e83ca454354e90bb4d3228ba2708e2350bb2a4 100644 (file)
--- a/src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc
+++ b/src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc
@@ -648,9 +648,6 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, SyncSnapshot) {
   MockImageCopyRequest mock_image_copy_request;
   expect_image_copy(mock_image_copy_request, 0, 1, 0, {},
                     {{1, CEPH_NOSNAP}}, 0);
-  MockUnlinkPeerRequest mock_unlink_peer_request;
-  expect_unlink_peer(mock_unlink_peer_request, 1, "remote mirror peer uuid",
-                     0);
   expect_mirror_image_snapshot_set_copy_progress(
     mock_local_image_ctx, 11, true, 0, 0);
   expect_notify_update(mock_local_image_ctx);
@@ -673,11 +670,12 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, SyncSnapshot) {
                                     {{1, 11}, {4, CEPH_NOSNAP}}, 14, 0);
   expect_image_copy(mock_image_copy_request, 1, 4, 11, {},
                     {{1, 11}, {4, CEPH_NOSNAP}}, 0);
-  expect_unlink_peer(mock_unlink_peer_request, 4, "remote mirror peer uuid",
-                     0);
+  MockUnlinkPeerRequest mock_unlink_peer_request;
   expect_mirror_image_snapshot_set_copy_progress(
     mock_local_image_ctx, 14, true, 0, 0);
   expect_notify_update(mock_local_image_ctx);
+  expect_unlink_peer(mock_unlink_peer_request, 1, "remote mirror peer uuid",
+                     0);
 
   // idle
   expect_is_refresh_required(mock_local_image_ctx, true);
@@ -754,9 +752,6 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, InterruptedSync) {
   expect_image_copy(mock_image_copy_request, 0, 1, 0,
                     librbd::deep_copy::ObjectNumber{123U},
                     {{1, CEPH_NOSNAP}}, 0);
-  MockUnlinkPeerRequest mock_unlink_peer_request;
-  expect_unlink_peer(mock_unlink_peer_request, 1, "remote mirror peer uuid",
-                     0);
   expect_mirror_image_snapshot_set_copy_progress(
     mock_local_image_ctx, 11, true, 123, 0);
   expect_notify_update(mock_local_image_ctx);
@@ -832,9 +827,6 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, RemoteImageDemoted) {
   MockImageCopyRequest mock_image_copy_request;
   expect_image_copy(mock_image_copy_request, 0, 1, 0, {},
                     {{1, CEPH_NOSNAP}}, 0);
-  MockUnlinkPeerRequest mock_unlink_peer_request;
-  expect_unlink_peer(mock_unlink_peer_request, 1, "remote mirror peer uuid",
-                     0);
   expect_mirror_image_snapshot_set_copy_progress(
     mock_local_image_ctx, 11, true, 0, 0);
   expect_notify_update(mock_local_image_ctx);
@@ -1300,7 +1292,7 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, CopyImageError) {
                                         mock_remote_image_ctx));
 }
 
-TEST_F(TestMockImageReplayerSnapshotReplayer, UnlinkPeerError) {
+TEST_F(TestMockImageReplayerSnapshotReplayer, UpdateNonPrimarySnapshotError) {
   librbd::MockTestImageCtx mock_local_image_ctx{*m_local_image_ctx};
   librbd::MockTestImageCtx mock_remote_image_ctx{*m_remote_image_ctx};
 
@@ -1350,9 +1342,6 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, UnlinkPeerError) {
   MockImageCopyRequest mock_image_copy_request;
   expect_image_copy(mock_image_copy_request, 0, 1, 0, {},
                     {{1, CEPH_NOSNAP}}, 0);
-  MockUnlinkPeerRequest mock_unlink_peer_request;
-  expect_unlink_peer(mock_unlink_peer_request, 1, "remote mirror peer uuid",
-                     0);
   expect_mirror_image_snapshot_set_copy_progress(
     mock_local_image_ctx, 11, true, 0, -EINVAL);
 
@@ -1368,7 +1357,7 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, UnlinkPeerError) {
                                         mock_remote_image_ctx));
 }
 
-TEST_F(TestMockImageReplayerSnapshotReplayer, UpdateNonPrimarySnapshotError) {
+TEST_F(TestMockImageReplayerSnapshotReplayer, UnlinkPeerError) {
   librbd::MockTestImageCtx mock_local_image_ctx{*m_local_image_ctx};
   librbd::MockTestImageCtx mock_remote_image_ctx{*m_remote_image_ctx};
 
@@ -1401,28 +1390,38 @@ TEST_F(TestMockImageReplayerSnapshotReplayer, UpdateNonPrimarySnapshotError) {
     {1U, librbd::SnapInfo{"snap1", cls::rbd::MirrorSnapshotNamespace{
        cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, {"remote mirror peer uuid"}, "",
        0U},
+     0, {}, 0, 0, {}}},
+    {2U, librbd::SnapInfo{"snap2", cls::rbd::MirrorSnapshotNamespace{
+       cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, {"remote mirror peer uuid"},
+       "", 0U},
+     0, {}, 0, 0, {}}}};
+  mock_local_image_ctx.snap_info = {
+    {11U, librbd::SnapInfo{"snap1", cls::rbd::MirrorSnapshotNamespace{
+       cls::rbd::MIRROR_SNAPSHOT_STATE_NON_PRIMARY, {}, "remote mirror uuid",
+       1, true, 0, {}},
      0, {}, 0, 0, {}}}};
 
-  // sync snap1
+  // sync snap2
   expect_is_refresh_required(mock_local_image_ctx, false);
   expect_is_refresh_required(mock_remote_image_ctx, false);
   MockSnapshotCopyRequest mock_snapshot_copy_request;
-  expect_snapshot_copy(mock_snapshot_copy_request, 0, 1, 0, {{1, CEPH_NOSNAP}},
+  expect_snapshot_copy(mock_snapshot_copy_request, 1, 2, 11, {{2, CEPH_NOSNAP}},
                        0);
   MockGetImageStateRequest mock_get_image_state_request;
-  expect_get_image_state(mock_get_image_state_request, 1, 0);
+  expect_get_image_state(mock_get_image_state_request, 2, 0);
   MockCreateNonPrimaryRequest mock_create_non_primary_request;
   expect_create_non_primary_request(mock_create_non_primary_request,
-                                    false, "remote mirror uuid", 1,
-                                    {{1, CEPH_NOSNAP}}, 11, 0);
+                                    false, "remote mirror uuid", 2,
+                                    {{2, CEPH_NOSNAP}}, 12, 0);
   MockImageCopyRequest mock_image_copy_request;
-  expect_image_copy(mock_image_copy_request, 0, 1, 0, {},
-                    {{1, CEPH_NOSNAP}}, 0);
+  expect_image_copy(mock_image_copy_request, 1, 2, 11, {},
+                    {{2, CEPH_NOSNAP}}, 0);
+  expect_mirror_image_snapshot_set_copy_progress(
+    mock_local_image_ctx, 12, true, 0, 0);
+  expect_notify_update(mock_local_image_ctx);
   MockUnlinkPeerRequest mock_unlink_peer_request;
   expect_unlink_peer(mock_unlink_peer_request, 1, "remote mirror peer uuid",
-                     0);
-  expect_mirror_image_snapshot_set_copy_progress(
-    mock_local_image_ctx, 11, true, 0, -EINVAL);
+                     -EINVAL);
 
   // wake-up replayer
   update_watch_ctx->handle_notify();

diff --git a/src/tools/rbd_mirror/image_replayer/snapshot/Replayer.cc b/src/tools/rbd_mirror/image_replayer/snapshot/Replayer.cc
index e2fcc6836c346eef3a9497e84fce5e42f61d9eac..0a926429975ce0e9b86a3ddb08742d00897107d1 100644 (file)
--- a/src/tools/rbd_mirror/image_replayer/snapshot/Replayer.cc
+++ b/src/tools/rbd_mirror/image_replayer/snapshot/Replayer.cc
@@ -585,7 +585,7 @@ void Replayer<I>::handle_copy_image(int r) {
     return;
   }
 
-  unlink_peer();
+  update_non_primary_snapshot(true);
 }
 
 template <typename I>
@@ -595,32 +595,6 @@ void Replayer<I>::handle_copy_image_progress(uint64_t offset, uint64_t total) {
   // TODO
 }
 
-template <typename I>
-void Replayer<I>::unlink_peer() {
-  dout(10) << dendl;
-
-  auto ctx = create_context_callback<
-    Replayer<I>, &Replayer<I>::handle_unlink_peer>(this);
-  auto req = librbd::mirror::snapshot::UnlinkPeerRequest<I>::create(
-    m_state_builder->remote_image_ctx, m_remote_snap_id_end,
-    m_remote_mirror_peer_uuid, ctx);
-  req->send();
-}
-
-template <typename I>
-void Replayer<I>::handle_unlink_peer(int r) {
-  dout(10) << "r=" << r << dendl;
-
-  if (r < 0 && r != -ENOENT) {
-    derr << "failed to unlink local peer from remote image: " << cpp_strerror(r)
-         << dendl;
-    handle_replay_complete(r, "failed to unlink local peer from remote image");
-    return;
-  }
-
-  update_non_primary_snapshot(true);
-}
-
 template <typename I>
 void Replayer<I>::update_non_primary_snapshot(bool complete) {
   dout(10) << dendl;
@@ -655,11 +629,6 @@ void Replayer<I>::handle_update_non_primary_snapshot(bool complete, int r) {
     return;
   }
 
-  {
-    std::unique_lock locker{m_lock};
-    notify_status_updated();
-  }
-
   notify_image_update();
 }
 
@@ -684,6 +653,49 @@ void Replayer<I>::handle_notify_image_update(int r) {
     return;
   }
 
+  unlink_peer();
+}
+
+template <typename I>
+void Replayer<I>::unlink_peer() {
+  if (m_remote_snap_id_start == 0) {
+    {
+      std::unique_lock locker{m_lock};
+      notify_status_updated();
+    }
+
+    refresh_local_image();
+    return;
+  }
+
+  // local snapshot fully synced -- we no longer depend on the sync
+  // start snapshot in the remote image
+  dout(10) << "remote_snap_id=" << m_remote_snap_id_start << dendl;
+
+  auto ctx = create_context_callback<
+    Replayer<I>, &Replayer<I>::handle_unlink_peer>(this);
+  auto req = librbd::mirror::snapshot::UnlinkPeerRequest<I>::create(
+    m_state_builder->remote_image_ctx, m_remote_snap_id_start,
+    m_remote_mirror_peer_uuid, ctx);
+  req->send();
+}
+
+template <typename I>
+void Replayer<I>::handle_unlink_peer(int r) {
+  dout(10) << "r=" << r << dendl;
+
+  if (r < 0 && r != -ENOENT) {
+    derr << "failed to unlink local peer from remote image: " << cpp_strerror(r)
+         << dendl;
+    handle_replay_complete(r, "failed to unlink local peer from remote image");
+    return;
+  }
+
+  {
+    std::unique_lock locker{m_lock};
+    notify_status_updated();
+  }
+
   refresh_local_image();
 }
 
@@ -699,7 +711,6 @@ void Replayer<I>::register_update_watcher() {
   m_threads->work_queue->queue(ctx, r);
 }
 
-
 template <typename I>
 void Replayer<I>::handle_register_update_watcher(int r) {
   dout(10) << "r=" << r << dendl;

diff --git a/src/tools/rbd_mirror/image_replayer/snapshot/Replayer.h b/src/tools/rbd_mirror/image_replayer/snapshot/Replayer.h
index fd30bbdcce334cecf6e199c8cd37ee45c3f85b43..b28b69c073b3598362a96bac31788bbf96e35cba 100644 (file)
--- a/src/tools/rbd_mirror/image_replayer/snapshot/Replayer.h
+++ b/src/tools/rbd_mirror/image_replayer/snapshot/Replayer.h
@@ -122,15 +122,15 @@ private:
    *    |                 COPY_IMAGE                    |
    *    |                     |                         |
    *    |                     v                         |
-   *    |                 UNLINK_PEER                   |
-   *    |                     |                         |
-   *    |                     v                         |
    *    |                 UPDATE_NON_PRIMARY_SNAPSHOT   |
    *    |                     |                         |
    *    |                     v                         |
    *    |                 NOTIFY_IMAGE_UPDATE           |
    *    |                     |                         |
    *    |                     v                         |
+   *    |                 UNLINK_PEER                   |
+   *    |                     |                         |
+   *    |                     v                         |
    *    |                 NOTIFY_LISTENER               |
    *    |                     |                         |
    *    |                     \------------------------/|
@@ -225,15 +225,15 @@ private:
   void handle_copy_image(int r);
   void handle_copy_image_progress(uint64_t offset, uint64_t total);
 
-  void unlink_peer();
-  void handle_unlink_peer(int r);
-
   void update_non_primary_snapshot(bool complete);
   void handle_update_non_primary_snapshot(bool complete, int r);
 
   void notify_image_update();
   void handle_notify_image_update(int r);
 
+  void unlink_peer();
+  void handle_unlink_peer(int r);
+
   void register_update_watcher();
   void handle_register_update_watcher(int r);