From 9de6523b934956cec20ae8c5093da02f31f1552f Mon Sep 17 00:00:00 2001 From: songweibin Date: Tue, 20 Aug 2019 20:25:25 +0800 Subject: [PATCH] librbd: allow remove snapshot with child from non-existent pool Signed-off-by: songweibin (cherry picked from commit 50f3cc199e5a4ace0ccf1d755ac682aeb489a454) Conflicts: src/test/librbd/test_librbd.cc - nautilus does not have test "DISABLED_TestSeqWriteAIOPP" test/librbd/operation/test_mock_SnapshotRemoveRequest.cc - nautilus uses "RWLock::RLocker", instead of "std::shared_lock", with owner_locker --- src/librbd/operation/SnapshotRemoveRequest.cc | 99 ++++++++++++++- src/librbd/operation/SnapshotRemoveRequest.h | 13 ++ .../test_mock_SnapshotRemoveRequest.cc | 118 ++++++++++++++++++ src/test/librbd/test_librbd.cc | 92 ++++++++++++++ 4 files changed, 320 insertions(+), 2 deletions(-) diff --git a/src/librbd/operation/SnapshotRemoveRequest.cc b/src/librbd/operation/SnapshotRemoveRequest.cc index fde41a5f363dc..69fbc3e735a67 100644 --- a/src/librbd/operation/SnapshotRemoveRequest.cc +++ b/src/librbd/operation/SnapshotRemoveRequest.cc @@ -120,6 +120,7 @@ void SnapshotRemoveRequest::get_snap() { auto aio_comp = create_rados_callback< SnapshotRemoveRequest, &SnapshotRemoveRequest::handle_get_snap>(this); + m_out_bl.clear(); int r = image_ctx.md_ctx.aio_operate(image_ctx.header_oid, aio_comp, &op, &m_out_bl); ceph_assert(r == 0); @@ -137,8 +138,10 @@ void SnapshotRemoveRequest::handle_get_snap(int r) { auto it = m_out_bl.cbegin(); r = cls_client::snapshot_get_finish(&it, &snap_info); - if (r == 0) { - m_child_attached = (snap_info.child_count > 0); + m_child_attached = (snap_info.child_count > 0); + if (r == 0 && m_child_attached) { + list_children(); + return; } } @@ -152,6 +155,98 @@ void SnapshotRemoveRequest::handle_get_snap(int r) { detach_child(); } +template +void SnapshotRemoveRequest::list_children() { + I &image_ctx = this->m_image_ctx; + CephContext *cct = image_ctx.cct; + ldout(cct, 5) << dendl; + + librados::ObjectReadOperation op; + cls_client::children_list_start(&op, m_snap_id); + + m_out_bl.clear(); + m_child_images.clear(); + auto aio_comp = create_rados_callback< + SnapshotRemoveRequest, + &SnapshotRemoveRequest::handle_list_children>(this); + int r = image_ctx.md_ctx.aio_operate(image_ctx.header_oid, aio_comp, &op, + &m_out_bl); + ceph_assert(r == 0); + aio_comp->release(); +} + +template +void SnapshotRemoveRequest::handle_list_children(int r) { + I &image_ctx = this->m_image_ctx; + CephContext *cct = image_ctx.cct; + ldout(cct, 5) << "r=" << r << dendl; + + if (r == 0) { + auto it = m_out_bl.cbegin(); + r = cls_client::children_list_finish(&it, &m_child_images); + } + + if (r < 0 && r != -ENOENT) { + lderr(cct) << "failed to retrieve child: " << cpp_strerror(r) + << dendl; + this->complete(r); + return; + } + + detach_stale_child(); +} + +template +void SnapshotRemoveRequest::detach_stale_child() { + I &image_ctx = this->m_image_ctx; + CephContext *cct = image_ctx.cct; + ldout(cct, 5) << dendl; + + for (auto& child_image : m_child_images) { + m_child_attached = true; + IoCtx ioctx; + int r = util::create_ioctx(image_ctx.md_ctx, "child image", + child_image.pool_id, + child_image.pool_namespace, &ioctx); + if (r == -ENOENT) { + librados::ObjectWriteOperation op; + cls_client::child_detach(&op, m_snap_id, + {child_image.pool_id, + child_image.pool_namespace, + child_image.image_id}); + auto aio_comp = create_rados_callback< + SnapshotRemoveRequest, + &SnapshotRemoveRequest::handle_detach_stale_child>(this); + r = image_ctx.md_ctx.aio_operate(image_ctx.header_oid, aio_comp, &op); + ceph_assert(r == 0); + aio_comp->release(); + return; + } else if (r < 0) { + this->async_complete(r); + return; + } + } + + detach_child(); +} + +template +void SnapshotRemoveRequest::handle_detach_stale_child(int r) { + I &image_ctx = this->m_image_ctx; + CephContext *cct = image_ctx.cct; + ldout(cct, 5) << "r=" << r << dendl; + + if (r < 0 && r != -ENOENT) { + lderr(cct) << "failed to detach stale child: " << cpp_strerror(r) + << dendl; + this->complete(r); + return; + } + + m_child_attached = false; + list_children(); +} + template void SnapshotRemoveRequest::detach_child() { I &image_ctx = this->m_image_ctx; diff --git a/src/librbd/operation/SnapshotRemoveRequest.h b/src/librbd/operation/SnapshotRemoveRequest.h index f980ba3b83f99..a47bbc0a377aa 100644 --- a/src/librbd/operation/SnapshotRemoveRequest.h +++ b/src/librbd/operation/SnapshotRemoveRequest.h @@ -32,6 +32,12 @@ public: * GET_SNAP * | * v (skip if unnecessary) + * LIST_CHILDREN <-------------\ + * | | + * v (skip if unnecessary) | (repeat as needed) + * DETACH_STALE_CHILD ---------/ + * | + * v (skip if unnecessary) * DETACH_CHILD * | * v (skip if disabled/in-use) @@ -71,6 +77,7 @@ protected: private: cls::rbd::SnapshotNamespace m_snap_namespace; + cls::rbd::ChildImageSpecs m_child_images; std::string m_snap_name; uint64_t m_snap_id; bool m_trashed_snapshot = false; @@ -84,6 +91,12 @@ private: void get_snap(); void handle_get_snap(int r); + void list_children(); + void handle_list_children(int r); + + void detach_stale_child(); + void handle_detach_stale_child(int r); + void detach_child(); void handle_detach_child(int r); diff --git a/src/test/librbd/operation/test_mock_SnapshotRemoveRequest.cc b/src/test/librbd/operation/test_mock_SnapshotRemoveRequest.cc index 87e1f28792209..53c0ca1be95d2 100644 --- a/src/test/librbd/operation/test_mock_SnapshotRemoveRequest.cc +++ b/src/test/librbd/operation/test_mock_SnapshotRemoveRequest.cc @@ -112,6 +112,36 @@ public: }))); } + void expect_children_list(MockImageCtx &mock_image_ctx, + const cls::rbd::ChildImageSpecs& child_images, int r) { + if (mock_image_ctx.old_format) { + return; + } + + using ceph::encode; + EXPECT_CALL(get_mock_io_ctx(mock_image_ctx.md_ctx), + exec(mock_image_ctx.header_oid, _, StrEq("rbd"), + StrEq("children_list"), _, _, _)) + .WillOnce(WithArg<5>(Invoke([child_images, r](bufferlist* bl) { + encode(child_images, *bl); + return r; + }))); + } + + void expect_detach_stale_child(MockImageCtx &mock_image_ctx, int r) { + auto& parent_spec = mock_image_ctx.parent_md.spec; + + bufferlist bl; + encode(parent_spec.snap_id, bl); + encode(cls::rbd::ChildImageSpec{mock_image_ctx.md_ctx.get_id(), "", + mock_image_ctx.id}, bl); + EXPECT_CALL(get_mock_io_ctx(mock_image_ctx.md_ctx), + exec(util::header_name(parent_spec.image_id), + _, StrEq("rbd"), StrEq("child_detach"), ContentsEqual(bl), + _, _)) + .WillOnce(Return(r)); + } + void expect_object_map_snap_remove(MockImageCtx &mock_image_ctx, int r) { if (mock_image_ctx.object_map != nullptr) { EXPECT_CALL(*mock_image_ctx.object_map, snapshot_remove(_, _)) @@ -241,6 +271,8 @@ TEST_F(TestMockOperationSnapshotRemoveRequest, SuccessCloneParent) { {snap_id, {cls::rbd::UserSnapshotNamespace{}}, "snap1", 123, {}, 1}, 0); + const cls::rbd::ChildImageSpecs child_images; + expect_children_list(mock_image_ctx, child_images, 0); expect_get_parent_spec(mock_image_ctx, 0); C_SaferCond cond_ctx; @@ -397,6 +429,8 @@ TEST_F(TestMockOperationSnapshotRemoveRequest, TrashCloneParent) { expect_snapshot_get(mock_image_ctx, {snap_id, {cls::rbd::TrashSnapshotNamespace{}}, "snap1", 123, {}, 1}, 0); + const cls::rbd::ChildImageSpecs child_images; + expect_children_list(mock_image_ctx, child_images, 0); expect_get_parent_spec(mock_image_ctx, 0); C_SaferCond cond_ctx; @@ -711,5 +745,89 @@ TEST_F(TestMockOperationSnapshotRemoveRequest, MissingSnap) { ASSERT_EQ(-ENOENT, cond_ctx.wait()); } +TEST_F(TestMockOperationSnapshotRemoveRequest, ListChildrenError) { + REQUIRE_FEATURE(RBD_FEATURE_LAYERING); + + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + ASSERT_EQ(0, snap_create(*ictx, "snap1")); + ASSERT_EQ(0, ictx->state->refresh_if_required()); + + MockImageCtx mock_image_ctx(*ictx); + + MockExclusiveLock mock_exclusive_lock; + if (ictx->test_features(RBD_FEATURE_EXCLUSIVE_LOCK)) { + mock_image_ctx.exclusive_lock = &mock_exclusive_lock; + } + + MockObjectMap mock_object_map; + if (ictx->test_features(RBD_FEATURE_OBJECT_MAP)) { + mock_image_ctx.object_map = &mock_object_map; + } + + expect_op_work_queue(mock_image_ctx); + + ::testing::InSequence seq; + expect_snapshot_trash_add(mock_image_ctx, 0); + + uint64_t snap_id = ictx->snap_info.rbegin()->first; + expect_snapshot_get(mock_image_ctx, + {snap_id, {cls::rbd::UserSnapshotNamespace{}}, + "snap1", 123, {}, 1}, 0); + const cls::rbd::ChildImageSpecs child_images; + expect_children_list(mock_image_ctx, child_images, -EINVAL); + + C_SaferCond cond_ctx; + MockSnapshotRemoveRequest *req = new MockSnapshotRemoveRequest( + mock_image_ctx, &cond_ctx, cls::rbd::UserSnapshotNamespace(), "snap1", + snap_id); + { + RWLock::RLocker owner_locker(mock_image_ctx.owner_lock); + req->send(); + } + ASSERT_EQ(-EINVAL, cond_ctx.wait()); +} + +TEST_F(TestMockOperationSnapshotRemoveRequest, DetachStaleChildError) { + REQUIRE_FEATURE(RBD_FEATURE_LAYERING); + + ASSERT_EQ(0, create_snapshot("snap1")); + + int order = 22; + uint64_t features; + ASSERT_TRUE(::get_features(&features)); + std::string clone_name = get_temp_image_name(); + ASSERT_EQ(0, librbd::clone(m_ioctx, m_image_name.c_str(), "snap1", m_ioctx, + clone_name.c_str(), features, &order, 0, 0)); + + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(clone_name, &ictx)); + ASSERT_EQ(0, snap_create(*ictx, "snap1")); + ASSERT_EQ(0, ictx->state->refresh_if_required()); + + MockImageCtx mock_image_ctx(*ictx); + expect_op_work_queue(mock_image_ctx); + + ::testing::InSequence seq; + expect_snapshot_trash_add(mock_image_ctx, 0); + + uint64_t snap_id = ictx->snap_info.rbegin()->first; + expect_snapshot_get(mock_image_ctx, + {snap_id, {cls::rbd::UserSnapshotNamespace{}}, + "snap1", 123, {}, 1}, 0); + const cls::rbd::ChildImageSpecs child_images; + expect_children_list(mock_image_ctx, child_images, -EINVAL); + + C_SaferCond cond_ctx; + MockSnapshotRemoveRequest *req = new MockSnapshotRemoveRequest( + mock_image_ctx, &cond_ctx, cls::rbd::UserSnapshotNamespace(), "snap1", + snap_id); + { + RWLock::RLocker owner_locker(mock_image_ctx.owner_lock); + req->send(); + } + ASSERT_EQ(-EINVAL, cond_ctx.wait()); +} + } // namespace operation } // namespace librbd diff --git a/src/test/librbd/test_librbd.cc b/src/test/librbd/test_librbd.cc index f94824aadb1df..933335bbb01cd 100644 --- a/src/test/librbd/test_librbd.cc +++ b/src/test/librbd/test_librbd.cc @@ -7890,6 +7890,98 @@ TEST_F(TestLibRBD, ImageSpec) { ASSERT_EQ(expected_children, children); } +TEST_F(TestLibRBD, SnapRemoveWithChildMissing) +{ + REQUIRE_FEATURE(RBD_FEATURE_LAYERING); + ASSERT_EQ(0, rados_conf_set(_cluster, "rbd_default_clone_format", "2")); + BOOST_SCOPE_EXIT_ALL(&) { + ASSERT_EQ(0, rados_conf_set(_cluster, "rbd_default_clone_format", "auto")); + }; + + librbd::RBD rbd; + rados_ioctx_t ioctx1, ioctx2; + string pool_name1 = create_pool(true); + rados_ioctx_create(_cluster, pool_name1.c_str(), &ioctx1); + ASSERT_EQ(0, rados_ioctx_create(_cluster, m_pool_name.c_str(), &ioctx2)); + + bool old_format; + uint64_t features; + rbd_image_t parent, child1, child2, child3; + int order = 0; + char child_id1[4096]; + char child_id2[4096]; + char child_id3[4096]; + + ASSERT_EQ(0, get_features(&old_format, &features)); + ASSERT_FALSE(old_format); + std::string parent_name = get_temp_image_name(); + std::string child_name1 = get_temp_image_name(); + std::string child_name2 = get_temp_image_name(); + std::string child_name3 = get_temp_image_name(); + ASSERT_EQ(0, create_image_full(ioctx1, parent_name.c_str(), 4<<20, &order, + false, features)); + ASSERT_EQ(0, rbd_open(ioctx1, parent_name.c_str(), &parent, NULL)); + ASSERT_EQ(0, rbd_snap_create(parent, "snap1")); + ASSERT_EQ(0, rbd_snap_create(parent, "snap2")); + + ASSERT_EQ(0, clone_image(ioctx1, parent, parent_name.c_str(), "snap1", + ioctx2, child_name1.c_str(), features, &order)); + ASSERT_EQ(0, clone_image(ioctx1, parent, parent_name.c_str(), "snap2", + ioctx1, child_name2.c_str(), features, &order)); + ASSERT_EQ(0, clone_image(ioctx1, parent, parent_name.c_str(), "snap2", + ioctx2, child_name3.c_str(), features, &order)); + + ASSERT_EQ(0, rbd_open(ioctx2, child_name1.c_str(), &child1, NULL)); + ASSERT_EQ(0, rbd_open(ioctx1, child_name2.c_str(), &child2, NULL)); + ASSERT_EQ(0, rbd_open(ioctx2, child_name3.c_str(), &child3, NULL)); + ASSERT_EQ(0, rbd_get_id(child1, child_id1, sizeof(child_id1))); + ASSERT_EQ(0, rbd_get_id(child2, child_id2, sizeof(child_id2))); + ASSERT_EQ(0, rbd_get_id(child3, child_id3, sizeof(child_id3))); + test_list_children2(parent, 3, + child_id1, m_pool_name.c_str(), child_name1.c_str(), false, + child_id2, pool_name1.c_str(), child_name2.c_str(), false, + child_id3, m_pool_name.c_str(), child_name3.c_str(), false); + + size_t max_size = 10; + rbd_linked_image_spec_t children[max_size]; + ASSERT_EQ(0, rbd_list_children3(parent, children, &max_size)); + ASSERT_EQ(3, static_cast(max_size)); + rbd_linked_image_spec_list_cleanup(children, max_size); + + ASSERT_EQ(0, rbd_close(child1)); + ASSERT_EQ(0, rbd_close(child2)); + ASSERT_EQ(0, rbd_close(child3)); + rados_ioctx_destroy(ioctx2); + ASSERT_EQ(0, rados_pool_delete(_cluster, m_pool_name.c_str())); + _pool_names.erase(std::remove(_pool_names.begin(), + _pool_names.end(), m_pool_name), + _pool_names.end()); + EXPECT_EQ(0, rados_wait_for_latest_osdmap(_cluster)); + + ASSERT_EQ(0, rbd_list_children3(parent, children, &max_size)); + ASSERT_EQ(3, static_cast(max_size)); + rbd_linked_image_spec_list_cleanup(children, max_size); + ASSERT_EQ(0, rbd_snap_remove(parent, "snap1")); + ASSERT_EQ(0, rbd_list_children3(parent, children, &max_size)); + ASSERT_EQ(2, static_cast(max_size)); + rbd_linked_image_spec_list_cleanup(children, max_size); + + ASSERT_EQ(0, rbd_remove(ioctx1, child_name2.c_str())); + ASSERT_EQ(0, rbd_list_children3(parent, children, &max_size)); + ASSERT_EQ(1, static_cast(max_size)); + rbd_linked_image_spec_list_cleanup(children, max_size); + + ASSERT_EQ(0, rbd_snap_remove(parent, "snap2")); + ASSERT_EQ(0, rbd_list_children3(parent, children, &max_size)); + ASSERT_EQ(0, static_cast(max_size)); + rbd_linked_image_spec_list_cleanup(children, max_size); + test_list_children2(parent, 0); + ASSERT_EQ(0, test_ls_snaps(parent, 0)); + + ASSERT_EQ(0, rbd_close(parent)); + rados_ioctx_destroy(ioctx1); +} + // poorman's ceph_assert() namespace ceph { void __ceph_assert_fail(const char *assertion, const char *file, int line, -- 2.39.5