From: Arthur Outhenin-Chalandre Date: Tue, 20 Apr 2021 11:51:45 +0000 (+0200) Subject: librbd/mirror/snapshot: avoid UnlinkPeerRequest with a unlinked peer X-Git-Tag: v15.2.13~3^2 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=d1869fcccd572dec9b7bd1023478fd988efba8ff;p=ceph.git 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 (cherry picked from commit c6e2953fdb9e29cfb5fb4e04fd633862160cdb13) --- diff --git a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc index 9fc865d1747d3..65a8a05b97a46 100644 --- a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc +++ b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc @@ -203,6 +203,18 @@ void CreatePrimaryRequest::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 f07f425500424..754ef9727f957 100644 --- 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( &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 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 d449be78d9fd0..918b57f555ee3 100644 --- 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 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();