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: v17.1.0~2062^2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=refs%2Fpull%2F40937%2Fhead;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 --- diff --git a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc index ce14df952e98..13f7894156ed 100644 --- a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc +++ b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc @@ -205,6 +205,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 2627e33a802c..9826bf9b0a41 100644 --- 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( &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 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 daab119fe3a4..4fe1d97b9ed3 100644 --- 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 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();