From: Jason Dillaman <dillaman@redhat.com>
Date: Thu, 10 Dec 2020 22:32:16 +0000 (-0500)
Subject: librbd/mirror: tweak which snapshot is unlinked when at capacity
X-Git-Tag: v16.1.0~247^2~1
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=a888bff8d00e3e496ec80e4273e01a47b67da5dc;p=ceph.git

librbd/mirror: tweak which snapshot is unlinked when at capacity

The rbd-mirror daemon will attempt to sync from the last synced
snapshot to the next mirror snapshot. When the limit is at 3, this
currently can result in a situation where an in-use sync snapshot is
deleted. Instead of unlinking the second oldest snapshot, always
unlink the third oldest.

Fixes: https://tracker.ceph.com/issues/48553
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
---

diff --git a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
index 70962cc6efb..ce14df952e9 100644
--- a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
+++ b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
@@ -192,7 +192,7 @@ void CreatePrimaryRequest<I>::unlink_peer() {
   for (auto &peer : m_mirror_peer_uuids) {
     std::shared_lock image_locker{m_image_ctx->image_lock};
     size_t count = 0;
-    uint64_t prev_snap_id = 0;
+    uint64_t unlink_snap_id = 0;
     for (auto &snap_it : m_image_ctx->snap_info) {
       auto info = boost::get<cls::rbd::MirrorSnapshotNamespace>(
         &snap_it.second.snap_namespace);
@@ -202,15 +202,16 @@ void CreatePrimaryRequest<I>::unlink_peer() {
       if (info->state != cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY) {
         // reset counters -- we count primary snapshots after the last promotion
         count = 0;
-        prev_snap_id = 0;
+        unlink_snap_id = 0;
         continue;
       }
       count++;
-      if (count == max_snapshots - 1) {
-        prev_snap_id = snap_it.first;
-      } else if (count > max_snapshots) {
+      if (count == 3) {
+        unlink_snap_id = snap_it.first;
+      }
+      if (count > max_snapshots) {
         peer_uuid = peer;
-        snap_id = prev_snap_id;
+        snap_id = unlink_snap_id;
         break;
       }
     }
diff --git a/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc b/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc
index f38e58d8e87..2627e33a802 100644
--- a/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc
+++ b/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc
@@ -313,7 +313,7 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkPeer) {
   expect_create_snapshot(mock_image_ctx, 0);
   MockUnlinkPeerRequest mock_unlink_peer_request;
   auto it = mock_image_ctx.snap_info.rbegin();
-  auto snap_id = (++it)->first;
+  auto snap_id = it->first;
   expect_unlink_peer(mock_image_ctx, mock_unlink_peer_request, snap_id, "uuid",
                      0);
   C_SaferCond ctx;
diff --git a/src/test/librbd/test_mirroring.cc b/src/test/librbd/test_mirroring.cc
index 75d18208cc7..daab119fe3a 100644
--- a/src/test/librbd/test_mirroring.cc
+++ b/src/test/librbd/test_mirroring.cc
@@ -1153,7 +1153,7 @@ TEST_F(TestMirroring, Snapshot)
   ASSERT_EQ(0, image.snap_list(snaps1));
   ASSERT_EQ(3U, snaps1.size());
   ASSERT_EQ(snaps1[0].id, snaps[0].id);
-  ASSERT_EQ(snaps1[1].id, snaps[2].id);
+  ASSERT_EQ(snaps1[1].id, snaps[1].id);
   ASSERT_EQ(snaps1[2].id, snap_id);
 
   librbd::snap_namespace_type_t snap_ns_type;