From: Ilya Dryomov Date: Mon, 20 Jun 2022 12:19:41 +0000 (+0200) Subject: rbd-mirror: generally skip replay/resync if remote image is not primary X-Git-Tag: v16.2.11~103^2~26^2~4 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=ba5364196433a54368532b0ae6ac2f58e568f65f;p=ceph.git rbd-mirror: generally skip replay/resync if remote image is not primary Replay and resync should generally be skipped if the remote image is not primary. If this is not done for replay, snapshot-based mirroring can run into a livelock if the primary image is demoted while a mirror snapshot is being synced. On the demote site, rbd-mirror would pick up the just demoted image, grab the exclusive lock on it and idle waiting for a new mirror snapshot to be created. On the (still) non-primary site, rbd-mirror would eventually finish syncing that mirror snapshot and attempt to unlink from it on the demote site. These attempts would fail with EROFS due to exclusive lock being held in the "refuse proxied maintenance operations" mode, blocking forward progress (syncing of the demotion snapshot so that the non-primary image can be orderly promoted to primary, etc). If this is not done for resync, data loss can ensue as the just demoted image would be immediately trashed, underneath the non-primary site that is still syncing. Currently this is done in PrepareReplayRequest only for journal-based mirroring. Note that it is conditional: if the local image is linked to the remote image, proceeding is desirable. Generalize this check, consolidate it with a related check in PrepareRemoteImageRequest and move the result to BootstrapRequest to cover both "local image does not exist" and "local image is unlinked" cases for both modes. Fixes: https://tracker.ceph.com/issues/54448 Signed-off-by: Ilya Dryomov (cherry picked from commit 79d28e63cb47e9bacbb2fd8a5ddc3a092377731b) --- diff --git a/src/test/rbd_mirror/image_replayer/journal/test_mock_PrepareReplayRequest.cc b/src/test/rbd_mirror/image_replayer/journal/test_mock_PrepareReplayRequest.cc index 9f75e69e609a6..c570f69ebf520 100644 --- a/src/test/rbd_mirror/image_replayer/journal/test_mock_PrepareReplayRequest.cc +++ b/src/test/rbd_mirror/image_replayer/journal/test_mock_PrepareReplayRequest.cc @@ -293,35 +293,6 @@ TEST_F(TestMockImageReplayerJournalPrepareReplayRequest, ResyncRequestedError) { ASSERT_EQ(-EINVAL, ctx.wait()); } -TEST_F(TestMockImageReplayerJournalPrepareReplayRequest, UnlinkedRemoteNonPrimary) { - InSequence seq; - - librbd::MockJournal mock_journal; - librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx); - mock_local_image_ctx.journal = &mock_journal; - - // check initial state - expect_is_resync_requested(mock_journal, false, 0); - expect_journal_get_tag_tid(mock_journal, 345); - expect_journal_get_tag_data(mock_journal, {"blah"}); - - C_SaferCond ctx; - ::journal::MockJournaler mock_remote_journaler; - librbd::journal::MirrorPeerClientMeta mirror_peer_client_meta; - mirror_peer_client_meta.state = librbd::journal::MIRROR_PEER_STATE_REPLAYING; - mirror_peer_client_meta.image_id = mock_local_image_ctx.id; - MockStateBuilder mock_state_builder(mock_local_image_ctx, - mock_remote_journaler, - mirror_peer_client_meta); - bool resync_requested; - bool syncing; - auto request = create_request( - mock_state_builder, librbd::mirror::PROMOTION_STATE_NON_PRIMARY, - "local mirror uuid", &resync_requested, &syncing, &ctx); - request->send(); - ASSERT_EQ(-EREMOTEIO, ctx.wait()); -} - TEST_F(TestMockImageReplayerJournalPrepareReplayRequest, Syncing) { InSequence seq; @@ -558,37 +529,6 @@ TEST_F(TestMockImageReplayerJournalPrepareReplayRequest, UpdateClientError) { ASSERT_FALSE(syncing); } -TEST_F(TestMockImageReplayerJournalPrepareReplayRequest, NonPrimaryRemoteNotTagOwner) { - InSequence seq; - - librbd::MockJournal mock_journal; - librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx); - mock_local_image_ctx.journal = &mock_journal; - - // check initial state - expect_is_resync_requested(mock_journal, false, 0); - expect_journal_get_tag_tid(mock_journal, 345); - expect_journal_get_tag_data(mock_journal, {librbd::Journal<>::LOCAL_MIRROR_UUID, - librbd::Journal<>::ORPHAN_MIRROR_UUID, - true, 344, 0}); - - C_SaferCond ctx; - ::journal::MockJournaler mock_remote_journaler; - librbd::journal::MirrorPeerClientMeta mirror_peer_client_meta; - mirror_peer_client_meta.state = librbd::journal::MIRROR_PEER_STATE_REPLAYING; - mirror_peer_client_meta.image_id = mock_local_image_ctx.id; - MockStateBuilder mock_state_builder(mock_local_image_ctx, - mock_remote_journaler, - mirror_peer_client_meta); - bool resync_requested; - bool syncing; - auto request = create_request( - mock_state_builder, librbd::mirror::PROMOTION_STATE_NON_PRIMARY, - "local mirror uuid", &resync_requested, &syncing, &ctx); - request->send(); - ASSERT_EQ(-EREMOTEIO, ctx.wait()); -} - TEST_F(TestMockImageReplayerJournalPrepareReplayRequest, RemoteDemotePromote) { InSequence seq; diff --git a/src/test/rbd_mirror/image_replayer/test_mock_BootstrapRequest.cc b/src/test/rbd_mirror/image_replayer/test_mock_BootstrapRequest.cc index 53a69a84eefaf..4edd1e760bf6a 100644 --- a/src/test/rbd_mirror/image_replayer/test_mock_BootstrapRequest.cc +++ b/src/test/rbd_mirror/image_replayer/test_mock_BootstrapRequest.cc @@ -232,6 +232,7 @@ struct StateBuilder { MOCK_CONST_METHOD0(is_disconnected, bool()); MOCK_CONST_METHOD0(is_local_primary, bool()); + MOCK_CONST_METHOD0(is_remote_primary, bool()); MOCK_CONST_METHOD0(is_linked, bool()); MOCK_CONST_METHOD0(replay_requires_remote_image, bool()); @@ -355,6 +356,17 @@ public: .WillOnce(Return(is_primary)); } + void expect_is_remote_primary(MockStateBuilder& mock_state_builder, + bool is_primary) { + EXPECT_CALL(mock_state_builder, is_remote_primary()) + .WillOnce(Return(is_primary)); + } + + void expect_is_linked(MockStateBuilder& mock_state_builder, bool is_linked) { + EXPECT_CALL(mock_state_builder, is_linked()) + .WillOnce(Return(is_linked)); + } + void expect_is_disconnected(MockStateBuilder& mock_state_builder, bool is_disconnected) { EXPECT_CALL(mock_state_builder, is_disconnected()) @@ -492,6 +504,107 @@ TEST_F(TestMockImageReplayerBootstrapRequest, Success) { expect_send(mock_prepare_remote_image_request, mock_state_builder, "remote mirror uuid", m_remote_image_ctx->id, 0); expect_is_local_primary(mock_state_builder, false); + expect_is_remote_primary(mock_state_builder, true); + + // open the remote image + librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); + MockOpenImageRequest mock_open_image_request; + expect_open_image(mock_open_image_request, m_remote_io_ctx, + mock_remote_image_ctx.id, mock_remote_image_ctx, 0); + + // open the local image + librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx); + MockOpenLocalImageRequest mock_open_local_image_request; + expect_open_local_image(mock_open_local_image_request, m_local_io_ctx, + mock_local_image_ctx.id, &mock_local_image_ctx, 0); + + // prepare replay + expect_prepare_replay(mock_state_builder, false, false, 0); + expect_is_disconnected(mock_state_builder, false); + + // close remote image + expect_replay_requires_remote_image(mock_state_builder, false); + expect_close_remote_image(mock_state_builder, 0); + + C_SaferCond ctx; + MockThreads mock_threads(m_threads); + MockInstanceWatcher mock_instance_watcher; + MockBootstrapRequest *request = create_request( + &mock_threads, &mock_instance_watcher, "global image id", + "local mirror uuid", &ctx); + request->send(); + ASSERT_EQ(0, ctx.wait()); +} + +TEST_F(TestMockImageReplayerBootstrapRequest, PrepareRemoteImageNotPrimaryLocalDNE) { + InSequence seq; + + // prepare local image + MockStateBuilder mock_state_builder; + MockPrepareLocalImageRequest mock_prepare_local_image_request; + expect_send(mock_prepare_local_image_request, mock_state_builder, + m_local_image_ctx->id, m_local_image_ctx->name, -ENOENT); + + // prepare remote image + MockPrepareRemoteImageRequest mock_prepare_remote_image_request; + expect_send(mock_prepare_remote_image_request, mock_state_builder, + "remote mirror uuid", m_remote_image_ctx->id, 0); + expect_is_local_primary(mock_state_builder, false); + expect_is_remote_primary(mock_state_builder, false); + + C_SaferCond ctx; + MockThreads mock_threads(m_threads); + MockInstanceWatcher mock_instance_watcher; + MockBootstrapRequest *request = create_request( + &mock_threads, &mock_instance_watcher, "global image id", + "local mirror uuid", &ctx); + request->send(); + ASSERT_EQ(-EREMOTEIO, ctx.wait()); +} + +TEST_F(TestMockImageReplayerBootstrapRequest, PrepareRemoteImageNotPrimaryLocalUnlinked) { + InSequence seq; + + // prepare local image + MockStateBuilder mock_state_builder; + MockPrepareLocalImageRequest mock_prepare_local_image_request; + expect_send(mock_prepare_local_image_request, mock_state_builder, + m_local_image_ctx->id, m_local_image_ctx->name, 0); + + // prepare remote image + MockPrepareRemoteImageRequest mock_prepare_remote_image_request; + expect_send(mock_prepare_remote_image_request, mock_state_builder, + "remote mirror uuid", m_remote_image_ctx->id, 0); + expect_is_local_primary(mock_state_builder, false); + expect_is_remote_primary(mock_state_builder, false); + expect_is_linked(mock_state_builder, false); + + C_SaferCond ctx; + MockThreads mock_threads(m_threads); + MockInstanceWatcher mock_instance_watcher; + MockBootstrapRequest *request = create_request( + &mock_threads, &mock_instance_watcher, "global image id", + "local mirror uuid", &ctx); + request->send(); + ASSERT_EQ(-EREMOTEIO, ctx.wait()); +} + +TEST_F(TestMockImageReplayerBootstrapRequest, PrepareRemoteImageNotPrimaryLocalLinked) { + InSequence seq; + + // prepare local image + MockStateBuilder mock_state_builder; + MockPrepareLocalImageRequest mock_prepare_local_image_request; + expect_send(mock_prepare_local_image_request, mock_state_builder, + m_local_image_ctx->id, m_local_image_ctx->name, 0); + + // prepare remote image + MockPrepareRemoteImageRequest mock_prepare_remote_image_request; + expect_send(mock_prepare_remote_image_request, mock_state_builder, + "remote mirror uuid", m_remote_image_ctx->id, 0); + expect_is_local_primary(mock_state_builder, false); + expect_is_remote_primary(mock_state_builder, false); + expect_is_linked(mock_state_builder, true); // open the remote image librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); @@ -537,6 +650,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, OpenLocalImageError) { expect_send(mock_prepare_remote_image_request, mock_state_builder, "remote mirror uuid", m_remote_image_ctx->id, 0); expect_is_local_primary(mock_state_builder, false); + expect_is_remote_primary(mock_state_builder, true); // open the remote image librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); @@ -579,6 +693,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, OpenLocalImageDNE) { expect_send(mock_prepare_remote_image_request, mock_state_builder, "remote mirror uuid", m_remote_image_ctx->id, 0); expect_is_local_primary(mock_state_builder, false); + expect_is_remote_primary(mock_state_builder, true); // open the remote image librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); @@ -632,6 +747,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, OpenLocalImagePrimary) { expect_send(mock_prepare_remote_image_request, mock_state_builder, "remote mirror uuid", m_remote_image_ctx->id, 0); expect_is_local_primary(mock_state_builder, false); + expect_is_remote_primary(mock_state_builder, true); // open the remote image librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); @@ -674,6 +790,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, CreateLocalImageError) { expect_send(mock_prepare_remote_image_request, mock_state_builder, "remote mirror uuid", m_remote_image_ctx->id, 0); expect_is_local_primary(mock_state_builder, false); + expect_is_remote_primary(mock_state_builder, true); // open the remote image librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); @@ -712,6 +829,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, PrepareReplayError) { expect_send(mock_prepare_remote_image_request, mock_state_builder, "remote mirror uuid", m_remote_image_ctx->id, 0); expect_is_local_primary(mock_state_builder, false); + expect_is_remote_primary(mock_state_builder, true); // open the remote image librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); @@ -756,6 +874,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, PrepareReplayResyncRequested) { expect_send(mock_prepare_remote_image_request, mock_state_builder, "remote mirror uuid", m_remote_image_ctx->id, 0); expect_is_local_primary(mock_state_builder, false); + expect_is_remote_primary(mock_state_builder, true); // open the remote image librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); @@ -801,6 +920,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, PrepareReplaySyncing) { expect_send(mock_prepare_remote_image_request, mock_state_builder, "remote mirror uuid", m_remote_image_ctx->id, 0); expect_is_local_primary(mock_state_builder, false); + expect_is_remote_primary(mock_state_builder, true); // open the remote image librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); @@ -850,6 +970,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, PrepareReplayDisconnected) { expect_send(mock_prepare_remote_image_request, mock_state_builder, "remote mirror uuid", m_remote_image_ctx->id, 0); expect_is_local_primary(mock_state_builder, false); + expect_is_remote_primary(mock_state_builder, true); // open the remote image librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); @@ -895,6 +1016,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, ImageSyncError) { expect_send(mock_prepare_remote_image_request, mock_state_builder, "remote mirror uuid", m_remote_image_ctx->id, 0); expect_is_local_primary(mock_state_builder, false); + expect_is_remote_primary(mock_state_builder, true); // open the remote image librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); @@ -944,6 +1066,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, ImageSyncCanceled) { expect_send(mock_prepare_remote_image_request, mock_state_builder, "remote mirror uuid", m_remote_image_ctx->id, 0); expect_is_local_primary(mock_state_builder, false); + expect_is_remote_primary(mock_state_builder, true); // open the remote image librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); @@ -990,6 +1113,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, CloseRemoteImageError) { expect_send(mock_prepare_remote_image_request, mock_state_builder, "remote mirror uuid", m_remote_image_ctx->id, 0); expect_is_local_primary(mock_state_builder, false); + expect_is_remote_primary(mock_state_builder, true); // open the remote image librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); @@ -1035,6 +1159,7 @@ TEST_F(TestMockImageReplayerBootstrapRequest, ReplayRequiresRemoteImage) { expect_send(mock_prepare_remote_image_request, mock_state_builder, "remote mirror uuid", m_remote_image_ctx->id, 0); expect_is_local_primary(mock_state_builder, false); + expect_is_remote_primary(mock_state_builder, true); // open the remote image librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx); diff --git a/src/tools/rbd_mirror/image_replayer/BootstrapRequest.cc b/src/tools/rbd_mirror/image_replayer/BootstrapRequest.cc index a34f39c15b34a..0cf52f8d57d64 100644 --- a/src/tools/rbd_mirror/image_replayer/BootstrapRequest.cc +++ b/src/tools/rbd_mirror/image_replayer/BootstrapRequest.cc @@ -177,10 +177,6 @@ void BootstrapRequest::handle_prepare_remote_image(int r) { dout(5) << "local image is primary" << dendl; finish(-ENOMSG); return; - } else if (r == -EREMOTEIO) { - dout(10) << "remote-image is non-primary" << cpp_strerror(r) << dendl; - finish(r); - return; } else if (r == -ENOENT || state_builder == nullptr) { dout(10) << "remote image does not exist"; if (state_builder != nullptr) { @@ -209,6 +205,23 @@ void BootstrapRequest::handle_prepare_remote_image(int r) { return; } + if (!state_builder->is_remote_primary()) { + ceph_assert(!state_builder->remote_image_id.empty()); + if (state_builder->local_image_id.empty()) { + dout(10) << "local image does not exist and remote image is not primary" + << dendl; + finish(-EREMOTEIO); + return; + } else if (!state_builder->is_linked()) { + dout(10) << "local image is unlinked and remote image is not primary" + << dendl; + finish(-EREMOTEIO); + return; + } + // if the local image is linked to the remote image, we ignore that + // the remote image is not primary so that we can replay demotion + } + open_remote_image(); } @@ -314,9 +327,7 @@ void BootstrapRequest::handle_prepare_replay(int r) { dout(10) << "r=" << r << dendl; if (r < 0) { - if (r != -EREMOTEIO) { - derr << "failed to prepare local replay: " << cpp_strerror(r) << dendl; - } + derr << "failed to prepare local replay: " << cpp_strerror(r) << dendl; m_ret_val = r; close_remote_image(); return; diff --git a/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.cc b/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.cc index b1e37ed084b80..02dfb4f1cfbff 100644 --- a/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.cc +++ b/src/tools/rbd_mirror/image_replayer/PrepareRemoteImageRequest.cc @@ -116,15 +116,6 @@ void PrepareRemoteImageRequest::handle_get_mirror_info(int r) { dout(5) << "remote image mirroring is being disabled" << dendl; finish(-ENOENT); return; - } else if (m_promotion_state != librbd::mirror::PROMOTION_STATE_PRIMARY && - (state_builder == nullptr || - state_builder->local_image_id.empty() || - state_builder->local_promotion_state == - librbd::mirror::PROMOTION_STATE_UNKNOWN)) { - // no local image and remote isn't primary -- don't sync it - dout(5) << "remote image is not primary -- not syncing" << dendl; - finish(-EREMOTEIO); - return; } switch (m_mirror_image.mode) { diff --git a/src/tools/rbd_mirror/image_replayer/StateBuilder.cc b/src/tools/rbd_mirror/image_replayer/StateBuilder.cc index c8649ab2fa83e..55fb3509d19d2 100644 --- a/src/tools/rbd_mirror/image_replayer/StateBuilder.cc +++ b/src/tools/rbd_mirror/image_replayer/StateBuilder.cc @@ -44,6 +44,15 @@ bool StateBuilder::is_local_primary() const { return false; } +template +bool StateBuilder::is_remote_primary() const { + if (remote_promotion_state == librbd::mirror::PROMOTION_STATE_PRIMARY) { + ceph_assert(!remote_image_id.empty()); + return true; + } + return false; +} + template bool StateBuilder::is_linked() const { if (local_promotion_state == librbd::mirror::PROMOTION_STATE_NON_PRIMARY) { diff --git a/src/tools/rbd_mirror/image_replayer/StateBuilder.h b/src/tools/rbd_mirror/image_replayer/StateBuilder.h index 4c839a3efcee2..51cf8668c1eff 100644 --- a/src/tools/rbd_mirror/image_replayer/StateBuilder.h +++ b/src/tools/rbd_mirror/image_replayer/StateBuilder.h @@ -44,6 +44,7 @@ public: virtual bool is_disconnected() const = 0; bool is_local_primary() const; + bool is_remote_primary() const; bool is_linked() const; virtual cls::rbd::MirrorImageMode get_mirror_image_mode() const = 0; diff --git a/src/tools/rbd_mirror/image_replayer/journal/PrepareReplayRequest.cc b/src/tools/rbd_mirror/image_replayer/journal/PrepareReplayRequest.cc index 69631f27587ed..c8a96a4ad364c 100644 --- a/src/tools/rbd_mirror/image_replayer/journal/PrepareReplayRequest.cc +++ b/src/tools/rbd_mirror/image_replayer/journal/PrepareReplayRequest.cc @@ -68,17 +68,6 @@ void PrepareReplayRequest::send() { << "local tag data=" << m_local_tag_data << dendl; image_locker.unlock(); - if (m_local_tag_data.mirror_uuid != m_state_builder->remote_mirror_uuid && - m_remote_promotion_state != librbd::mirror::PROMOTION_STATE_PRIMARY) { - // if the local mirror is not linked to the (now) non-primary image, - // stop the replay. Otherwise, we ignore that the remote is non-primary - // so that we can replay the demotion - dout(5) << "remote image is not primary -- skipping image replay" - << dendl; - finish(-EREMOTEIO); - return; - } - if (*m_resync_requested) { finish(0); return;