From 2f042666004930c702aaafef1740c41f84b4e765 Mon Sep 17 00:00:00 2001
From: Jason Dillaman <dillaman@redhat.com>
Date: Thu, 10 Dec 2020 17:32:16 -0500
Subject: [PATCH] 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>
(cherry picked from commit a888bff8d00e3e496ec80e4273e01a47b67da5dc)
---
 src/librbd/mirror/snapshot/CreatePrimaryRequest.cc  | 13 +++++++------
 .../snapshot/test_mock_CreatePrimaryRequest.cc      |  2 +-
 src/test/librbd/test_mirroring.cc                   |  2 +-
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
index a6ce53ff16150..9fc865d1747d3 100644
--- a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
+++ b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
@@ -190,7 +190,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);
@@ -200,15 +200,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 a79bd0cc45ff6..f07f425500424 100644
--- a/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc
+++ b/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc
@@ -311,7 +311,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 c72c34c32d4b2..d449be78d9fd0 100644
--- a/src/test/librbd/test_mirroring.cc
+++ b/src/test/librbd/test_mirroring.cc
@@ -1154,7 +1154,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;
-- 
2.39.5