librbd/mirror/snapshot: avoid UnlinkPeerRequest with a unlinked peer

CreatePrimaryRequest could create some UnlinkPeerRequest with an already
unlinked peer in a scenario where you have multiple peers. This request
will not remove the peer (as it's already not linked to the requested
peer) and will skip deletion of the mirror snapshot if another peer
remains. Eventually the code will go through an infinite recursive loop
between CreatePrimaryRequest and UnlinkPeerRequest and segfault.

This commit adds an extra condition to make sure to not submit a
UnlinkPeerRequest if the peer is not linked to the current snapshot. If
there is already no peer in the list it will submit a UnlinkPeerRequest
to remove the snapshot.

Fixes: https://tracker.ceph.com/issues/50439
Signed-off-by: Arthur Outhenin-Chalandre <arthur.outhenin-chalandre@cern.ch>
(cherry picked from commit c6e2953fdb9e29cfb5fb4e04fd633862160cdb13)

diff --git a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
index 9fc865d1747d31eb0a981b36c6879687102e3fb8..65a8a05b97a462facd483cdcdb110df84d5e55ca 100644 (file)
--- a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
+++ b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
@@ -203,6 +203,18 @@ void CreatePrimaryRequest<I>::unlink_peer() {
         unlink_snap_id = 0;
         continue;
       }
+      // call UnlinkPeerRequest only if the snapshot is linked with this peer
+      // or if it's not linked with any peer (happens if mirroring is enabled
+      // on a pool with no peers configured or if UnlinkPeerRequest gets
+      // interrupted)
+      if (info->mirror_peer_uuids.size() == 0) {
+        peer_uuid = peer;
+        snap_id = snap_it.first;
+        break;
+      }
+      if (info->mirror_peer_uuids.count(peer) == 0) {
+        continue;
+      }
       count++;
       if (count == 3) {
         unlink_snap_id = snap_it.first;

diff --git a/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc b/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc
index f07f425500424cfbfb8bc635c3c43804a25eb413..754ef9727f95755582d1f04655b3ee8e75a5d4e5 100644 (file)
--- a/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc
+++ b/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc
@@ -165,10 +165,10 @@ public:
   void expect_unlink_peer(MockTestImageCtx &mock_image_ctx,
                           MockUnlinkPeerRequest &mock_unlink_peer_request,
                           uint64_t snap_id, const std::string &peer_uuid,
-                          int r) {
+                          bool is_linked, int r) {
     EXPECT_CALL(mock_unlink_peer_request, send())
-      .WillOnce(Invoke([&mock_image_ctx, &mock_unlink_peer_request, snap_id,
-                        peer_uuid, r]() {
+      .WillOnce(Invoke([&mock_image_ctx, &mock_unlink_peer_request,
+                        snap_id, peer_uuid, is_linked, r]() {
                          ASSERT_EQ(mock_unlink_peer_request.mirror_peer_uuid,
                                    peer_uuid);
                          ASSERT_EQ(mock_unlink_peer_request.snap_id, snap_id);
@@ -179,8 +179,8 @@ public:
                              boost::get<cls::rbd::MirrorSnapshotNamespace>(
                                &it->second.snap_namespace);
                            ASSERT_NE(nullptr, info);
-                           ASSERT_NE(0, info->mirror_peer_uuids.erase(
-                                       peer_uuid));
+                           ASSERT_EQ(is_linked, info->mirror_peer_uuids.erase(
+                                     peer_uuid));
                            if (info->mirror_peer_uuids.empty()) {
                              mock_image_ctx.snap_info.erase(it);
                            }
@@ -313,7 +313,82 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkPeer) {
   auto it = mock_image_ctx.snap_info.rbegin();
   auto snap_id = it->first;
   expect_unlink_peer(mock_image_ctx, mock_unlink_peer_request, snap_id, "uuid",
-                     0);
+                     true, 0);
+  C_SaferCond ctx;
+  auto req = new MockCreatePrimaryRequest(&mock_image_ctx, "gid", CEPH_NOSNAP,
+                                          0U, nullptr, &ctx);
+  req->send();
+  ASSERT_EQ(0, ctx.wait());
+}
+
+TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkNoPeer) {
+  REQUIRE_FORMAT_V2();
+
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(m_image_name, &ictx));
+  ictx->config.set_val("conf_rbd_mirroring_max_mirroring_snapshots", "3");
+
+  MockTestImageCtx mock_image_ctx(*ictx);
+  cls::rbd::MirrorSnapshotNamespace ns{
+    cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, {}, "", CEPH_NOSNAP};
+  snap_create(mock_image_ctx, ns, "mirror_snap");
+
+  InSequence seq;
+
+  expect_clone_md_ctx(mock_image_ctx);
+  MockUtils mock_utils;
+  expect_can_create_primary_snapshot(mock_utils, false, false, true);
+  expect_get_mirror_peers(mock_image_ctx,
+                          {{"uuid", cls::rbd::MIRROR_PEER_DIRECTION_TX, "ceph",
+                            "mirror", "mirror uuid"}}, 0);
+  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;
+  std::list<string> peer_uuids = {"uuid"};
+  expect_unlink_peer(mock_image_ctx, mock_unlink_peer_request, snap_id, "uuid",
+                     false, 0);
+
+  C_SaferCond ctx;
+  auto req = new MockCreatePrimaryRequest(&mock_image_ctx, "gid", CEPH_NOSNAP,
+                                          0U, nullptr, &ctx);
+  req->send();
+  ASSERT_EQ(0, ctx.wait());
+}
+
+TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkMultiplePeers) {
+  REQUIRE_FORMAT_V2();
+
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(m_image_name, &ictx));
+  ictx->config.set_val("conf_rbd_mirroring_max_mirroring_snapshots", "3");
+
+  MockTestImageCtx mock_image_ctx(*ictx);
+  for (int i = 0; i < 3; i++) {
+    cls::rbd::MirrorSnapshotNamespace ns{
+      cls::rbd::MIRROR_SNAPSHOT_STATE_PRIMARY, {"uuid1", "uuid2"}, "",
+      CEPH_NOSNAP};
+    snap_create(mock_image_ctx, ns, "mirror_snap");
+  }
+
+  InSequence seq;
+
+  expect_clone_md_ctx(mock_image_ctx);
+  MockUtils mock_utils;
+  expect_can_create_primary_snapshot(mock_utils, false, false, true);
+  expect_get_mirror_peers(mock_image_ctx,
+                          {{"uuid1", cls::rbd::MIRROR_PEER_DIRECTION_TX, "ceph",
+                            "mirror", "mirror uuid"},
+                           {"uuid2", cls::rbd::MIRROR_PEER_DIRECTION_TX, "ceph",
+                            "mirror", "mirror uuid"}}, 0);
+  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;
+  expect_unlink_peer(mock_image_ctx, mock_unlink_peer_request, snap_id, "uuid1",
+                     true, 0);
+  expect_unlink_peer(mock_image_ctx, mock_unlink_peer_request, snap_id, "uuid2",
+                     true, 0);
   C_SaferCond ctx;
   auto req = new MockCreatePrimaryRequest(&mock_image_ctx, "gid", CEPH_NOSNAP,
                                           0U, nullptr, &ctx);

diff --git a/src/test/librbd/test_mirroring.cc b/src/test/librbd/test_mirroring.cc
index d449be78d9fd02a0449f7d17bca212b3a379583b..918b57f555ee345f4d87d499a3321dff0f38f69a 100644 (file)
--- a/src/test/librbd/test_mirroring.cc
+++ b/src/test/librbd/test_mirroring.cc
@@ -1135,12 +1135,16 @@ TEST_F(TestMirroring, Snapshot)
   ASSERT_EQ(0, m_rbd.mirror_peer_site_add(m_ioctx, &peer_uuid,
                                           RBD_MIRROR_PEER_DIRECTION_RX_TX,
                                           "cluster", "client"));
+  // The mirroring was enabled when no peer was configured. Therefore, the
+  // initial snapshot has no peers linked and will be removed after the
+  // creation of a new mirror snapshot.
   ASSERT_EQ(0, image.mirror_image_create_snapshot(&snap_id));
   vector<librbd::snap_info_t> snaps;
   ASSERT_EQ(0, image.snap_list(snaps));
-  ASSERT_EQ(2U, snaps.size());
-  ASSERT_EQ(snaps[1].id, snap_id);
+  ASSERT_EQ(1U, snaps.size());
+  ASSERT_EQ(snaps[0].id, snap_id);
 
+  ASSERT_EQ(0, image.mirror_image_create_snapshot(&snap_id));
   ASSERT_EQ(0, image.mirror_image_create_snapshot(&snap_id));
   ASSERT_EQ(0, image.mirror_image_create_snapshot(&snap_id));
   snaps.clear();