From: Jason Dillaman Date: Mon, 23 Jul 2018 14:57:44 +0000 (-0400) Subject: librbd: clone detach should attempt to remove trashed snapshots X-Git-Tag: v14.0.1~767^2~1 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=59676fee0b4e330fe5d74c0cc5d5b3e79cad9b15;p=ceph.git librbd: clone detach should attempt to remove trashed snapshots Fixes: http://tracker.ceph.com/issues/23398 Signed-off-by: Jason Dillaman --- diff --git a/src/librbd/image/DetachChildRequest.cc b/src/librbd/image/DetachChildRequest.cc index db88f1a35b52..5902e1a6b321 100644 --- a/src/librbd/image/DetachChildRequest.cc +++ b/src/librbd/image/DetachChildRequest.cc @@ -7,6 +7,8 @@ #include "common/WorkQueue.h" #include "cls/rbd/cls_rbd_client.h" #include "librbd/ImageCtx.h" +#include "librbd/ImageState.h" +#include "librbd/Operations.h" #include "librbd/Utils.h" #include @@ -21,6 +23,11 @@ namespace image { using util::create_context_callback; using util::create_rados_callback; +template +DetachChildRequest::~DetachChildRequest() { + assert(m_parent_image_ctx == nullptr); +} + template void DetachChildRequest::send() { { @@ -65,11 +72,12 @@ void DetachChildRequest::clone_v2_child_detach() { // TODO support clone v2 parent namespaces m_parent_io_ctx.set_namespace(m_image_ctx.md_ctx.get_namespace()); + m_parent_header_name = util::header_name(m_parent_spec.image_id); + auto aio_comp = create_rados_callback< DetachChildRequest, &DetachChildRequest::handle_clone_v2_child_detach>(this); - r = m_parent_io_ctx.aio_operate(util::header_name(m_parent_spec.image_id), - aio_comp, &op); + r = m_parent_io_ctx.aio_operate(m_parent_header_name, aio_comp, &op); assert(r == 0); aio_comp->release(); } @@ -79,15 +87,149 @@ void DetachChildRequest::handle_clone_v2_child_detach(int r) { auto cct = m_image_ctx.cct; ldout(cct, 5) << "r=" << r << dendl; - if (r == -ENOENT) { - r = 0; - } else if (r < 0) { + if (r < 0 && r != -ENOENT) { lderr(cct) << "error detaching child from parent: " << cpp_strerror(r) << dendl; finish(r); return; } + clone_v2_get_snapshot(); +} + +template +void DetachChildRequest::clone_v2_get_snapshot() { + auto cct = m_image_ctx.cct; + ldout(cct, 5) << dendl; + + librados::ObjectReadOperation op; + cls_client::snapshot_info_get_start(&op, m_parent_spec.snap_id); + + m_out_bl.clear(); + auto aio_comp = create_rados_callback< + DetachChildRequest, + &DetachChildRequest::handle_clone_v2_get_snapshot>(this); + int r = m_parent_io_ctx.aio_operate(m_parent_header_name, aio_comp, &op, + &m_out_bl); + assert(r == 0); + aio_comp->release(); +} + +template +void DetachChildRequest::handle_clone_v2_get_snapshot(int r) { + auto cct = m_image_ctx.cct; + ldout(cct, 5) << "r=" << r << dendl; + + bool remove_snapshot = false; + if (r == 0) { + cls::rbd::SnapshotInfo snap_info; + auto it = m_out_bl.cbegin(); + r = cls_client::snapshot_info_get_finish(&it, &snap_info); + if (r == 0) { + m_parent_snap_namespace = snap_info.snapshot_namespace; + m_parent_snap_name = snap_info.name; + + if (cls::rbd::get_snap_namespace_type(m_parent_snap_namespace) == + cls::rbd::SNAPSHOT_NAMESPACE_TYPE_TRASH && + snap_info.child_count == 0) { + // snapshot is in trash w/ zero children, so remove it + remove_snapshot = true; + } + } + } + + if (r < 0) { + ldout(cct, 5) << "failed to retrieve snapshot: " << cpp_strerror(r) + << dendl; + } + + if (!remove_snapshot) { + finish(0); + return; + } + + clone_v2_open_parent(); +} + +template +void DetachChildRequest::clone_v2_open_parent() { + auto cct = m_image_ctx.cct; + ldout(cct, 5) << dendl; + + m_parent_image_ctx = I::create("", m_parent_spec.image_id, nullptr, + m_parent_io_ctx, false); + + auto ctx = create_context_callback< + DetachChildRequest, + &DetachChildRequest::handle_clone_v2_open_parent>(this); + m_parent_image_ctx->state->open(true, ctx); +} + +template +void DetachChildRequest::handle_clone_v2_open_parent(int r) { + auto cct = m_image_ctx.cct; + ldout(cct, 5) << "r=" << r << dendl; + + if (r < 0) { + ldout(cct, 5) << "failed to open parent for read/write: " + << cpp_strerror(r) << dendl; + m_parent_image_ctx->destroy(); + m_parent_image_ctx = nullptr; + finish(0); + return; + } + + clone_v2_remove_snapshot(); +} + +template +void DetachChildRequest::clone_v2_remove_snapshot() { + auto cct = m_image_ctx.cct; + ldout(cct, 5) << dendl; + + auto ctx = create_context_callback< + DetachChildRequest, + &DetachChildRequest::handle_clone_v2_remove_snapshot>(this); + m_parent_image_ctx->operations->snap_remove(m_parent_snap_namespace, + m_parent_snap_name, ctx); +} + +template +void DetachChildRequest::handle_clone_v2_remove_snapshot(int r) { + auto cct = m_image_ctx.cct; + ldout(cct, 5) << "r=" << r << dendl; + + if (r < 0 && r != -ENOENT) { + ldout(cct, 5) << "failed to remove trashed clone snapshot: " + << cpp_strerror(r) << dendl; + } + + clone_v2_close_parent(); +} + +template +void DetachChildRequest::clone_v2_close_parent() { + auto cct = m_image_ctx.cct; + ldout(cct, 5) << dendl; + + auto ctx = create_context_callback< + DetachChildRequest, + &DetachChildRequest::handle_clone_v2_close_parent>(this); + m_parent_image_ctx->state->close(ctx); +} + +template +void DetachChildRequest::handle_clone_v2_close_parent(int r) { + auto cct = m_image_ctx.cct; + ldout(cct, 5) << "r=" << r << dendl; + + if (r < 0) { + ldout(cct, 5) << "failed to close parent image:" << cpp_strerror(r) + << dendl; + } + + m_parent_image_ctx->destroy(); + m_parent_image_ctx = nullptr; finish(0); } diff --git a/src/librbd/image/DetachChildRequest.h b/src/librbd/image/DetachChildRequest.h index 472167902767..d9bfdf107ba6 100644 --- a/src/librbd/image/DetachChildRequest.h +++ b/src/librbd/image/DetachChildRequest.h @@ -27,6 +27,7 @@ public: DetachChildRequest(ImageCtxT& image_ctx, Context* on_finish) : m_image_ctx(image_ctx), m_on_finish(on_finish) { } + ~DetachChildRequest(); void send(); @@ -34,13 +35,28 @@ private: /** * @verbatim * - * - * | - * v (skip if v1) - * CHILD_DETACH - * | - * v (skip if v2) - * REMOVE_CHILD + * + * | + * (v1) | (v2) + * /--------------/ \--------------\ + * | | + * v v + * REMOVE_CHILD CHILD_DETACH + * | | + * | v + * | GET_SNAPSHOT + * | (snapshot in-use) . | + * |/. . . . . . . . . . . . . . . | + * | v + * | OPEN_PARENT + * | | + * | v + * | REMOVE_SNAPSHOT + * | | + * | v + * | CLOSE_PARENT + * | | + * |/------------------------------/ * | * v * @@ -53,12 +69,30 @@ private: librados::IoCtx m_parent_io_ctx; ParentSpec m_parent_spec; + std::string m_parent_header_name; + + cls::rbd::SnapshotNamespace m_parent_snap_namespace; + std::string m_parent_snap_name; + + ImageCtxT* m_parent_image_ctx = nullptr; ceph::bufferlist m_out_bl; void clone_v2_child_detach(); void handle_clone_v2_child_detach(int r); + void clone_v2_get_snapshot(); + void handle_clone_v2_get_snapshot(int r); + + void clone_v2_open_parent(); + void handle_clone_v2_open_parent(int r); + + void clone_v2_remove_snapshot(); + void handle_clone_v2_remove_snapshot(int r); + + void clone_v2_close_parent(); + void handle_clone_v2_close_parent(int r); + void clone_v1_remove_child(); void handle_clone_v1_remove_child(int r); diff --git a/src/test/librbd/image/test_mock_DetachChildRequest.cc b/src/test/librbd/image/test_mock_DetachChildRequest.cc index 2b73929cc403..4167598c85c7 100644 --- a/src/test/librbd/image/test_mock_DetachChildRequest.cc +++ b/src/test/librbd/image/test_mock_DetachChildRequest.cc @@ -15,10 +15,22 @@ namespace librbd { namespace { struct MockTestImageCtx : public MockImageCtx { + static MockTestImageCtx* s_instance; + static MockTestImageCtx* create(const std::string &image_name, + const std::string &image_id, + const char *snap, librados::IoCtx& p, + bool read_only) { + assert(s_instance != nullptr); + return s_instance; + } + MockTestImageCtx(ImageCtx &image_ctx) : MockImageCtx(image_ctx) { + s_instance = this; } }; +MockTestImageCtx* MockTestImageCtx::s_instance = nullptr; + } // anonymous namespace } // namespace librbd @@ -30,9 +42,12 @@ namespace image { using ::testing::_; using ::testing::DoAll; +using ::testing::DoDefault; using ::testing::InSequence; +using ::testing::Invoke; using ::testing::Return; using ::testing::StrEq; +using ::testing::WithArg; class TestMockImageDetachChildRequest : public TestMockFixture { public: @@ -50,7 +65,18 @@ public: .WillOnce(Return(enabled)); } - void expect_child_detach(MockImageCtx &mock_image_ctx, int r) { + void expect_create_ioctx(MockImageCtx &mock_image_ctx, + librados::MockTestMemIoCtxImpl **io_ctx_impl) { + *io_ctx_impl = &get_mock_io_ctx(mock_image_ctx.md_ctx); + auto rados_client = (*io_ctx_impl)->get_mock_rados_client(); + + EXPECT_CALL(*rados_client, create_ioctx(_, _)) + .WillOnce(Return(*io_ctx_impl)); + } + + void expect_child_detach(MockImageCtx &mock_image_ctx, + librados::MockTestMemIoCtxImpl &mock_io_ctx_impl, + int r) { auto& parent_spec = mock_image_ctx.parent_md.spec; bufferlist bl; @@ -58,14 +84,7 @@ public: encode(cls::rbd::ChildImageSpec{mock_image_ctx.md_ctx.get_id(), mock_image_ctx.id}, bl); - auto& io_ctx_impl = get_mock_io_ctx(mock_image_ctx.md_ctx); - auto rados_client = io_ctx_impl.get_mock_rados_client(); - - EXPECT_CALL(*rados_client, create_ioctx(_, _)) - .WillOnce(DoAll(GetReference(&io_ctx_impl), - Return(&io_ctx_impl))); - - EXPECT_CALL(io_ctx_impl, + EXPECT_CALL(mock_io_ctx_impl, exec(util::header_name(parent_spec.image_id), _, StrEq("rbd"), StrEq("child_detach"), ContentsEqual(bl), _, _)) @@ -79,6 +98,46 @@ public: .WillOnce(Return(r)); } + void expect_snapshot_get(MockImageCtx &mock_image_ctx, + librados::MockTestMemIoCtxImpl &mock_io_ctx_impl, + const std::string& parent_header_name, + const cls::rbd::SnapshotInfo& snap_info, int r) { + + using ceph::encode; + EXPECT_CALL(mock_io_ctx_impl, + exec(parent_header_name, _, StrEq("rbd"), + StrEq("snapshot_get"), _, _, _)) + .WillOnce(WithArg<5>(Invoke([snap_info, r](bufferlist* bl) { + encode(snap_info, *bl); + return r; + }))); + } + + void expect_open(MockImageCtx &mock_image_ctx, int r) { + EXPECT_CALL(*mock_image_ctx.state, open(true, _)) + .WillOnce(WithArg<1>(Invoke([this, r](Context* ctx) { + image_ctx->op_work_queue->queue(ctx, r); + }))); + } + + void expect_close(MockImageCtx &mock_image_ctx, int r) { + EXPECT_CALL(*mock_image_ctx.state, close(_)) + .WillOnce(Invoke([this, r](Context* ctx) { + image_ctx->op_work_queue->queue(ctx, r); + })); + EXPECT_CALL(mock_image_ctx, destroy()); + } + + void expect_snap_remove(MockImageCtx &mock_image_ctx, + const std::string &snap_name, int r) { + EXPECT_CALL(*mock_image_ctx.operations, + snap_remove({cls::rbd::TrashSnapshotNamespace{}}, + StrEq(snap_name), _)) + .WillOnce(WithArg<2>(Invoke([this, r](Context *ctx) { + image_ctx->op_work_queue->queue(ctx, r); + }))); + } + librbd::ImageCtx *image_ctx; }; @@ -106,7 +165,137 @@ TEST_F(TestMockImageDetachChildRequest, SuccessV2) { InSequence seq; expect_test_op_features(mock_image_ctx, true); - expect_child_detach(mock_image_ctx, 0); + + librados::MockTestMemIoCtxImpl *mock_io_ctx_impl; + expect_create_ioctx(mock_image_ctx, &mock_io_ctx_impl); + expect_child_detach(mock_image_ctx, *mock_io_ctx_impl, 0); + expect_snapshot_get(mock_image_ctx, *mock_io_ctx_impl, + "rbd_header.parent id", + {234, {cls::rbd::UserSnapshotNamespace{}}, + "snap1", 123, {}, 0}, 0); + + C_SaferCond ctx; + auto req = MockDetachChildRequest::create(mock_image_ctx, &ctx); + req->send(); + ASSERT_EQ(0, ctx.wait()); +} + +TEST_F(TestMockImageDetachChildRequest, TrashedSnapshotSuccess) { + REQUIRE_FEATURE(RBD_FEATURE_LAYERING); + + MockTestImageCtx mock_image_ctx(*image_ctx); + mock_image_ctx.parent_md.spec = {m_ioctx.get_id(), "parent id", 234}; + + InSequence seq; + expect_test_op_features(mock_image_ctx, true); + + librados::MockTestMemIoCtxImpl *mock_io_ctx_impl; + expect_create_ioctx(mock_image_ctx, &mock_io_ctx_impl); + expect_child_detach(mock_image_ctx, *mock_io_ctx_impl, 0); + expect_snapshot_get(mock_image_ctx, *mock_io_ctx_impl, + "rbd_header.parent id", + {234, {cls::rbd::TrashSnapshotNamespace{}}, + "snap1", 123, {}, 0}, 0); + expect_open(mock_image_ctx, 0); + expect_snap_remove(mock_image_ctx, "snap1", 0); + expect_close(mock_image_ctx, 0); + + C_SaferCond ctx; + auto req = MockDetachChildRequest::create(mock_image_ctx, &ctx); + req->send(); + ASSERT_EQ(0, ctx.wait()); +} + +TEST_F(TestMockImageDetachChildRequest, TrashedSnapshotInUse) { + REQUIRE_FEATURE(RBD_FEATURE_LAYERING); + + MockTestImageCtx mock_image_ctx(*image_ctx); + mock_image_ctx.parent_md.spec = {m_ioctx.get_id(), "parent id", 234}; + + InSequence seq; + expect_test_op_features(mock_image_ctx, true); + + librados::MockTestMemIoCtxImpl *mock_io_ctx_impl; + expect_create_ioctx(mock_image_ctx, &mock_io_ctx_impl); + expect_child_detach(mock_image_ctx, *mock_io_ctx_impl, 0); + expect_snapshot_get(mock_image_ctx, *mock_io_ctx_impl, + "rbd_header.parent id", + {234, {cls::rbd::TrashSnapshotNamespace{}}, + "snap1", 123, {}, 1}, 0); + + C_SaferCond ctx; + auto req = MockDetachChildRequest::create(mock_image_ctx, &ctx); + req->send(); + ASSERT_EQ(0, ctx.wait()); +} + +TEST_F(TestMockImageDetachChildRequest, TrashedSnapshotSnapshotGetError) { + REQUIRE_FEATURE(RBD_FEATURE_LAYERING); + + MockTestImageCtx mock_image_ctx(*image_ctx); + mock_image_ctx.parent_md.spec = {m_ioctx.get_id(), "parent id", 234}; + + InSequence seq; + expect_test_op_features(mock_image_ctx, true); + + librados::MockTestMemIoCtxImpl *mock_io_ctx_impl; + expect_create_ioctx(mock_image_ctx, &mock_io_ctx_impl); + expect_child_detach(mock_image_ctx, *mock_io_ctx_impl, 0); + expect_snapshot_get(mock_image_ctx, *mock_io_ctx_impl, + "rbd_header.parent id", + {234, {cls::rbd::TrashSnapshotNamespace{}}, + "snap1", 123, {}, 0}, -EINVAL); + + C_SaferCond ctx; + auto req = MockDetachChildRequest::create(mock_image_ctx, &ctx); + req->send(); + ASSERT_EQ(0, ctx.wait()); +} + +TEST_F(TestMockImageDetachChildRequest, TrashedSnapshotOpenParentError) { + REQUIRE_FEATURE(RBD_FEATURE_LAYERING); + + MockTestImageCtx mock_image_ctx(*image_ctx); + mock_image_ctx.parent_md.spec = {m_ioctx.get_id(), "parent id", 234}; + + InSequence seq; + expect_test_op_features(mock_image_ctx, true); + + librados::MockTestMemIoCtxImpl *mock_io_ctx_impl; + expect_create_ioctx(mock_image_ctx, &mock_io_ctx_impl); + expect_child_detach(mock_image_ctx, *mock_io_ctx_impl, 0); + expect_snapshot_get(mock_image_ctx, *mock_io_ctx_impl, + "rbd_header.parent id", + {234, {cls::rbd::TrashSnapshotNamespace{}}, + "snap1", 123, {}, 0}, 0); + expect_open(mock_image_ctx, -EPERM); + EXPECT_CALL(mock_image_ctx, destroy()); + + C_SaferCond ctx; + auto req = MockDetachChildRequest::create(mock_image_ctx, &ctx); + req->send(); + ASSERT_EQ(0, ctx.wait()); +} + +TEST_F(TestMockImageDetachChildRequest, TrashedSnapshotRemoveError) { + REQUIRE_FEATURE(RBD_FEATURE_LAYERING); + + MockTestImageCtx mock_image_ctx(*image_ctx); + mock_image_ctx.parent_md.spec = {m_ioctx.get_id(), "parent id", 234}; + + InSequence seq; + expect_test_op_features(mock_image_ctx, true); + + librados::MockTestMemIoCtxImpl *mock_io_ctx_impl; + expect_create_ioctx(mock_image_ctx, &mock_io_ctx_impl); + expect_child_detach(mock_image_ctx, *mock_io_ctx_impl, 0); + expect_snapshot_get(mock_image_ctx, *mock_io_ctx_impl, + "rbd_header.parent id", + {234, {cls::rbd::TrashSnapshotNamespace{}}, + "snap1", 123, {}, 0}, 0); + expect_open(mock_image_ctx, 0); + expect_snap_remove(mock_image_ctx, "snap1", -EPERM); + expect_close(mock_image_ctx, -EPERM); C_SaferCond ctx; auto req = MockDetachChildRequest::create(mock_image_ctx, &ctx); @@ -132,7 +321,10 @@ TEST_F(TestMockImageDetachChildRequest, ChildDetachError) { InSequence seq; expect_test_op_features(mock_image_ctx, true); - expect_child_detach(mock_image_ctx, -EPERM); + + librados::MockTestMemIoCtxImpl *mock_io_ctx_impl; + expect_create_ioctx(mock_image_ctx, &mock_io_ctx_impl); + expect_child_detach(mock_image_ctx, *mock_io_ctx_impl, -EPERM); C_SaferCond ctx; auto req = MockDetachChildRequest::create(mock_image_ctx, &ctx);