From 02afa2a5b41074c651e62a5475ed4828df982ed4 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Tue, 15 May 2018 14:07:35 -0400 Subject: [PATCH] rbd-mirror: image create should lookup local parent snapshot id A recent code change associated with a librbd cleanup incorrectly started using the remote parent image's snapshot id. Signed-off-by: Jason Dillaman --- .../test_mock_CreateImageRequest.cc | 29 +++++++------ .../image_replayer/CreateImageRequest.cc | 43 +++++++++++++++++-- 2 files changed, 54 insertions(+), 18 deletions(-) diff --git a/src/test/rbd_mirror/image_replayer/test_mock_CreateImageRequest.cc b/src/test/rbd_mirror/image_replayer/test_mock_CreateImageRequest.cc index 0c3bf6613ff..3d2152ab931 100644 --- a/src/test/rbd_mirror/image_replayer/test_mock_CreateImageRequest.cc +++ b/src/test/rbd_mirror/image_replayer/test_mock_CreateImageRequest.cc @@ -206,21 +206,17 @@ public: ASSERT_EQ(0, open_image(m_remote_io_ctx, m_image_name, &m_remote_image_ctx)); } + void snap_create(librbd::ImageCtx *image_ctx, const std::string &snap_name) { + ASSERT_EQ(0, image_ctx->operations->snap_create(cls::rbd::UserSnapshotNamespace(), + snap_name.c_str())); + ASSERT_EQ(0, image_ctx->operations->snap_protect(cls::rbd::UserSnapshotNamespace(), + snap_name.c_str())); + ASSERT_EQ(0, image_ctx->state->refresh()); + } + int clone_image(librbd::ImageCtx *parent_image_ctx, const std::string &snap_name, const std::string &clone_name) { - { - librbd::ImageCtx *ictx = new librbd::ImageCtx(parent_image_ctx->name, - "", "", m_remote_io_ctx, - false); - ictx->state->open(false); - EXPECT_EQ(0, ictx->operations->snap_create(cls::rbd::UserSnapshotNamespace(), - snap_name.c_str())); - EXPECT_EQ(0, ictx->operations->snap_protect(cls::rbd::UserSnapshotNamespace(), - snap_name.c_str())); - ictx->state->close(); - } - - EXPECT_EQ(0, parent_image_ctx->state->refresh()); + snap_create(parent_image_ctx, snap_name); int order = 0; return librbd::clone(m_remote_io_ctx, parent_image_ctx->name.c_str(), @@ -292,7 +288,7 @@ public: RWLock::RLocker snap_locker(mock_image_ctx.snap_lock); auto id = mock_image_ctx.image_ctx->get_snap_id( cls::rbd::UserSnapshotNamespace{}, snap_name); - ASSERT_NE(snap_id, id); + ASSERT_EQ(snap_id, id); m_threads->work_queue->queue(on_finish, r); })); } @@ -366,6 +362,7 @@ TEST_F(TestMockImageReplayerCreateImageRequest, Clone) { librbd::ImageCtx *local_image_ctx; ASSERT_EQ(0, create_image(rbd, m_local_io_ctx, m_image_name, m_image_size)); ASSERT_EQ(0, open_image(m_local_io_ctx, m_image_name, &local_image_ctx)); + snap_create(local_image_ctx, "snap"); std::string clone_image_name = get_temp_image_name(); ASSERT_EQ(0, clone_image(m_remote_image_ctx, "snap", clone_image_name)); @@ -533,6 +530,7 @@ TEST_F(TestMockImageReplayerCreateImageRequest, CloneSnapSetError) { librbd::ImageCtx *local_image_ctx; ASSERT_EQ(0, create_image(rbd, m_local_io_ctx, m_image_name, m_image_size)); ASSERT_EQ(0, open_image(m_local_io_ctx, m_image_name, &local_image_ctx)); + snap_create(local_image_ctx, "snap"); std::string clone_image_name = get_temp_image_name(); ASSERT_EQ(0, clone_image(m_remote_image_ctx, "snap", clone_image_name)); @@ -576,6 +574,7 @@ TEST_F(TestMockImageReplayerCreateImageRequest, CloneError) { librbd::ImageCtx *local_image_ctx; ASSERT_EQ(0, create_image(rbd, m_local_io_ctx, m_image_name, m_image_size)); ASSERT_EQ(0, open_image(m_local_io_ctx, m_image_name, &local_image_ctx)); + snap_create(local_image_ctx, "snap"); std::string clone_image_name = get_temp_image_name(); ASSERT_EQ(0, clone_image(m_remote_image_ctx, "snap", clone_image_name)); @@ -620,6 +619,7 @@ TEST_F(TestMockImageReplayerCreateImageRequest, CloneLocalParentCloseError) { librbd::ImageCtx *local_image_ctx; ASSERT_EQ(0, create_image(rbd, m_local_io_ctx, m_image_name, m_image_size)); ASSERT_EQ(0, open_image(m_local_io_ctx, m_image_name, &local_image_ctx)); + snap_create(local_image_ctx, "snap"); std::string clone_image_name = get_temp_image_name(); ASSERT_EQ(0, clone_image(m_remote_image_ctx, "snap", clone_image_name)); @@ -664,6 +664,7 @@ TEST_F(TestMockImageReplayerCreateImageRequest, CloneRemoteParentCloseError) { librbd::ImageCtx *local_image_ctx; ASSERT_EQ(0, create_image(rbd, m_local_io_ctx, m_image_name, m_image_size)); ASSERT_EQ(0, open_image(m_local_io_ctx, m_image_name, &local_image_ctx)); + snap_create(local_image_ctx, "snap"); std::string clone_image_name = get_temp_image_name(); ASSERT_EQ(0, clone_image(m_remote_image_ctx, "snap", clone_image_name)); diff --git a/src/tools/rbd_mirror/image_replayer/CreateImageRequest.cc b/src/tools/rbd_mirror/image_replayer/CreateImageRequest.cc index 351bf1e50ce..7a2bf903164 100644 --- a/src/tools/rbd_mirror/image_replayer/CreateImageRequest.cc +++ b/src/tools/rbd_mirror/image_replayer/CreateImageRequest.cc @@ -262,20 +262,55 @@ void CreateImageRequest::handle_open_local_parent_image(int r) { template void CreateImageRequest::set_local_parent_snap() { - dout(20) << ": parent_snap_id=" << m_remote_parent_spec.snap_id << dendl; + std::string snap_name; + cls::rbd::SnapshotNamespace snap_namespace; + { + RWLock::RLocker remote_snap_locker(m_remote_parent_image_ctx->snap_lock); + auto it = m_remote_parent_image_ctx->snap_info.find( + m_remote_parent_spec.snap_id); + if (it != m_remote_parent_image_ctx->snap_info.end()) { + snap_name = it->second.name; + snap_namespace = it->second.snap_namespace; + } + } + + if (snap_name.empty()) { + dout(15) << ": failed to locate remote parent snapshot" << dendl; + m_ret_val = -ENOENT; + close_local_parent_image(); + return; + } + + m_local_parent_spec.snap_id = CEPH_NOSNAP; + { + RWLock::RLocker local_snap_locker(m_local_parent_image_ctx->snap_lock); + auto it = m_local_parent_image_ctx->snap_ids.find( + {snap_namespace, snap_name}); + if (it != m_local_parent_image_ctx->snap_ids.end()) { + m_local_parent_spec.snap_id = it->second; + } + } + + if (m_local_parent_spec.snap_id == CEPH_NOSNAP) { + dout(15) << ": failed to locate local parent snapshot" << dendl; + m_ret_val = -ENOENT; + close_local_parent_image(); + return; + } + + dout(15) << ": local_snap_id=" << m_local_parent_spec.snap_id << dendl; Context *ctx = create_context_callback< CreateImageRequest, &CreateImageRequest::handle_set_local_parent_snap>(this); - m_local_parent_image_ctx->state->snap_set(m_remote_parent_spec.snap_id, - ctx); + m_local_parent_image_ctx->state->snap_set(m_local_parent_spec.snap_id, ctx); } template void CreateImageRequest::handle_set_local_parent_snap(int r) { dout(20) << ": r=" << r << dendl; if (r < 0) { - derr << ": failed to set parent snapshot " << m_remote_parent_spec.snap_id + derr << ": failed to set parent snapshot " << m_local_parent_spec.snap_id << ": " << cpp_strerror(r) << dendl; m_ret_val = r; close_local_parent_image(); -- 2.39.5