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>

diff --git a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
index ce14df952e981278db69b9b5bb24a4560f23d326..13f7894156ed16cc91513668cad470a9c0b384c9 100644 (file)
--- a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
+++ b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
@@ -205,6 +205,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 2627e33a802cee32f19b3835286fd9a7e643a7bb..9826bf9b0a410215e77115a7a00cde78a542b185 100644 (file)
--- a/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc
+++ b/src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc
@@ -167,10 +167,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);
@@ -181,8 +181,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);
                            }
@@ -315,7 +315,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, 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, 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, 0U, nullptr, &ctx);

diff --git a/src/test/librbd/test_mirroring.cc b/src/test/librbd/test_mirroring.cc
index daab119fe3a4980a0a7c1a0c7ef7ac157659f462..4fe1d97b9ed32955338a8b65921db18f33f4beb0 100644 (file)
--- a/src/test/librbd/test_mirroring.cc
+++ b/src/test/librbd/test_mirroring.cc
@@ -1134,12 +1134,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();