From 0527da67566c40f28cdf4de17c714bcdedb54b67 Mon Sep 17 00:00:00 2001 From: Mykola Golub Date: Wed, 16 Jan 2019 11:58:55 +0000 Subject: [PATCH] librbd: replace attach child call with new state machine Signed-off-by: Mykola Golub --- src/librbd/image/CloneRequest.cc | 122 +----------- src/librbd/image/CloneRequest.h | 34 +--- .../librbd/image/test_mock_CloneRequest.cc | 188 +++++------------- 3 files changed, 73 insertions(+), 271 deletions(-) diff --git a/src/librbd/image/CloneRequest.cc b/src/librbd/image/CloneRequest.cc index eccf7a53048e5..d36fa0bb723fe 100644 --- a/src/librbd/image/CloneRequest.cc +++ b/src/librbd/image/CloneRequest.cc @@ -7,11 +7,11 @@ #include "common/errno.h" #include "include/ceph_assert.h" #include "librbd/ImageState.h" +#include "librbd/image/AttachChildRequest.h" #include "librbd/image/AttachParentRequest.h" #include "librbd/image/CloneRequest.h" #include "librbd/image/CreateRequest.h" #include "librbd/image/RemoveRequest.h" -#include "librbd/image/RefreshRequest.h" #include "librbd/mirror/EnableRequest.h" #define dout_subsys ceph_subsys_rbd @@ -357,132 +357,32 @@ void CloneRequest::handle_attach_parent(int r) { return; } - v2_set_op_feature(); + attach_child(); } template -void CloneRequest::v2_set_op_feature() { - if (m_clone_format == 1) { - v1_add_child(); - return; - } - - ldout(m_cct, 15) << dendl; - - librados::ObjectWriteOperation op; - cls_client::op_features_set(&op, RBD_OPERATION_FEATURE_CLONE_CHILD, - RBD_OPERATION_FEATURE_CLONE_CHILD); - - auto aio_comp = create_rados_callback< - CloneRequest, &CloneRequest::handle_v2_set_op_feature>(this); - int r = m_ioctx.aio_operate(m_imctx->header_oid, aio_comp, &op); - ceph_assert(r == 0); - aio_comp->release(); -} - -template -void CloneRequest::handle_v2_set_op_feature(int r) { - ldout(m_cct, 15) << "r=" << r << dendl; - - if (r < 0) { - lderr(m_cct) << "failed to enable clone v2: " << cpp_strerror(r) << dendl; - m_r_saved = r; - close_child(); - return; - } - - v2_child_attach(); -} - -template -void CloneRequest::v2_child_attach() { +void CloneRequest::attach_child() { ldout(m_cct, 15) << dendl; - librados::ObjectWriteOperation op; - cls_client::child_attach(&op, m_parent_image_ctx->snap_id, - {m_imctx->md_ctx.get_id(), - m_imctx->md_ctx.get_namespace(), - m_imctx->id}); - - auto aio_comp = create_rados_callback< - CloneRequest, &CloneRequest::handle_v2_child_attach>(this); - int r = m_parent_image_ctx->md_ctx.aio_operate(m_parent_image_ctx->header_oid, aio_comp, &op); - ceph_assert(r == 0); - aio_comp->release(); -} - -template -void CloneRequest::handle_v2_child_attach(int r) { - ldout(m_cct, 15) << "r=" << r << dendl; - - if (r < 0) { - lderr(m_cct) << "failed to attach child image: " << cpp_strerror(r) - << dendl; - m_r_saved = r; - close_child(); - return; - } - - metadata_list(); -} - -template -void CloneRequest::v1_add_child() { - ldout(m_cct, 15) << dendl; - - librados::ObjectWriteOperation op; - cls_client::add_child(&op, m_pspec, m_id); - - using klass = CloneRequest; - librados::AioCompletion *comp = - create_rados_callback(this); - int r = m_ioctx.aio_operate(RBD_CHILDREN, comp, &op); - ceph_assert(r == 0); - comp->release(); + auto ctx = create_context_callback< + CloneRequest, &CloneRequest::handle_attach_child>(this); + auto req = AttachChildRequest::create( + m_imctx, m_parent_image_ctx, m_parent_image_ctx->snap_id, nullptr, 0, + m_clone_format, ctx); + req->send(); } template -void CloneRequest::handle_v1_add_child(int r) { +void CloneRequest::handle_attach_child(int r) { ldout(m_cct, 15) << "r=" << r << dendl; if (r < 0) { - lderr(m_cct) << "couldn't add child: " << cpp_strerror(r) << dendl; + lderr(m_cct) << "failed to attach parent: " << cpp_strerror(r) << dendl; m_r_saved = r; close_child(); return; } - v1_refresh(); -} - -template -void CloneRequest::v1_refresh() { - ldout(m_cct, 15) << dendl; - - using klass = CloneRequest; - RefreshRequest *req = RefreshRequest::create( - *m_imctx, false, false, - create_context_callback(this)); - req->send(); -} - -template -void CloneRequest::handle_v1_refresh(int r) { - ldout(m_cct, 15) << "r=" << r << dendl; - - bool snap_protected = false; - if (r == 0) { - m_parent_image_ctx->snap_lock.get_read(); - r = m_parent_image_ctx->is_snap_protected(m_parent_image_ctx->snap_id, &snap_protected); - m_parent_image_ctx->snap_lock.put_read(); - } - - if (r < 0 || !snap_protected) { - m_r_saved = -EINVAL; - close_child(); - return; - } - metadata_list(); } diff --git a/src/librbd/image/CloneRequest.h b/src/librbd/image/CloneRequest.h index 7f4c0ee09d715..aa03d97f0fb6a 100644 --- a/src/librbd/image/CloneRequest.h +++ b/src/librbd/image/CloneRequest.h @@ -65,23 +65,12 @@ private: * OPEN CHILD * * * * * * * * * * > REMOVE CHILD * | ^ * v | - * SET PARENT * * * * * * * * * * > CLOSE CHILD + * ATTACH PARENT * * * * * * * * > CLOSE CHILD * | ^ - * |\--------\ * - * | | * - * | v (clone v2 disabled) * - * | V1 ADD CHILD * * * * * * ^ - * | | * - * | v * - * | V1 VALIDATE PROTECTED * * ^ - * | | * - * v | * - * V2 SET CLONE * * * * * * * * * * * ^ - * | | * - * v | * - * V2 ATTACH CHILD * * * * * * * * * * - * | | * - * v v * + * v * + * ATTACH CHILD * * * * * * * * * * * * + * | * + * v * * GET PARENT META * * * * * * * * * ^ * | * * v (skip if not needed) * @@ -154,17 +143,8 @@ private: void attach_parent(); void handle_attach_parent(int r); - void v2_set_op_feature(); - void handle_v2_set_op_feature(int r); - - void v2_child_attach(); - void handle_v2_child_attach(int r); - - void v1_add_child(); - void handle_v1_add_child(int r); - - void v1_refresh(); - void handle_v1_refresh(int r); + void attach_child(); + void handle_attach_child(int r); void metadata_list(); void handle_metadata_list(int r); diff --git a/src/test/librbd/image/test_mock_CloneRequest.cc b/src/test/librbd/image/test_mock_CloneRequest.cc index 673d7dfe2beed..fd63ded44778f 100644 --- a/src/test/librbd/image/test_mock_CloneRequest.cc +++ b/src/test/librbd/image/test_mock_CloneRequest.cc @@ -10,10 +10,10 @@ #include "librbd/ImageState.h" #include "librbd/Operations.h" #include "librbd/image/TypeTraits.h" +#include "librbd/image/AttachChildRequest.h" #include "librbd/image/AttachParentRequest.h" #include "librbd/image/CreateRequest.h" #include "librbd/image/RemoveRequest.h" -#include "librbd/image/RefreshRequest.h" #include "librbd/mirror/EnableRequest.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -49,6 +49,33 @@ MockTestImageCtx* MockTestImageCtx::s_instance = nullptr; namespace image { +template <> +struct AttachChildRequest { + uint32_t clone_format; + Context* on_finish = nullptr; + static AttachChildRequest* s_instance; + static AttachChildRequest* create(MockTestImageCtx *, + MockTestImageCtx *, + const librados::snap_t &, + MockTestImageCtx *, + const librados::snap_t &, + uint32_t clone_format, + Context *on_finish) { + ceph_assert(s_instance != nullptr); + s_instance->clone_format = clone_format; + s_instance->on_finish = on_finish; + return s_instance; + } + + MOCK_METHOD0(send, void()); + + AttachChildRequest() { + s_instance = this; + } +}; + +AttachChildRequest* AttachChildRequest::s_instance = nullptr; + template <> struct AttachParentRequest { Context* on_finish = nullptr; @@ -98,27 +125,6 @@ struct CreateRequest { CreateRequest* CreateRequest::s_instance = nullptr; -template <> -struct RefreshRequest { - Context* on_finish = nullptr; - static RefreshRequest* s_instance; - static RefreshRequest* create(MockTestImageCtx &image_ctx, - bool acquiring_lock, bool skip_open_parent, - Context *on_finish) { - ceph_assert(s_instance != nullptr); - s_instance->on_finish = on_finish; - return s_instance; - } - - MOCK_METHOD0(send, void()); - - RefreshRequest() { - s_instance = this; - } -}; - -RefreshRequest* RefreshRequest::s_instance = nullptr; - template <> struct RemoveRequest { Context* on_finish = nullptr; @@ -182,7 +188,6 @@ namespace image { using ::testing::_; using ::testing::Invoke; -using ::testing::InvokeWithoutArgs; using ::testing::InSequence; using ::testing::Return; using ::testing::StrEq; @@ -191,9 +196,9 @@ using ::testing::WithArg; class TestMockImageCloneRequest : public TestMockFixture { public: typedef CloneRequest MockCloneRequest; + typedef AttachChildRequest MockAttachChildRequest; typedef AttachParentRequest MockAttachParentRequest; typedef CreateRequest MockCreateRequest; - typedef RefreshRequest MockRefreshRequest; typedef RemoveRequest MockRemoveRequest; typedef mirror::EnableRequest MockMirrorEnableRequest; @@ -268,40 +273,12 @@ public: })); } - void expect_op_features_set(librados::IoCtx& io_ctx, - const std::string& clone_id, int r) { - bufferlist bl; - encode(static_cast(RBD_OPERATION_FEATURE_CLONE_CHILD), bl); - encode(static_cast(RBD_OPERATION_FEATURE_CLONE_CHILD), bl); - - EXPECT_CALL(get_mock_io_ctx(io_ctx), - exec(util::header_name(clone_id), _, StrEq("rbd"), - StrEq("op_features_set"), ContentsEqual(bl), _, _)) - .WillOnce(Return(r)); - } - - void expect_child_attach(MockImageCtx &mock_image_ctx, int r) { - bufferlist bl; - encode(mock_image_ctx.snap_id, bl); - encode(cls::rbd::ChildImageSpec{m_ioctx.get_id(), "", mock_image_ctx.id}, - bl); - - EXPECT_CALL(get_mock_io_ctx(mock_image_ctx.md_ctx), - exec(mock_image_ctx.header_oid, _, StrEq("rbd"), - StrEq("child_attach"), ContentsEqual(bl), _, _)) - .WillOnce(Return(r)); - } - - void expect_add_child(librados::IoCtx& io_ctx, int r) { - EXPECT_CALL(get_mock_io_ctx(io_ctx), - exec(RBD_CHILDREN, _, StrEq("rbd"), StrEq("add_child"), _, _, _)) - .WillOnce(Return(r)); - } - - void expect_refresh(MockRefreshRequest& mock_refresh_request, int r) { - EXPECT_CALL(mock_refresh_request, send()) - .WillOnce(Invoke([this, &mock_refresh_request, r]() { - image_ctx->op_work_queue->queue(mock_refresh_request.on_finish, r); + void expect_attach_child(MockAttachChildRequest& mock_request, + uint32_t clone_format, int r) { + EXPECT_CALL(mock_request, send()) + .WillOnce(Invoke([this, &mock_request, clone_format, r]() { + EXPECT_EQ(mock_request.clone_format, clone_format); + image_ctx->op_work_queue->queue(mock_request.on_finish, r); })); } @@ -399,11 +376,8 @@ TEST_F(TestMockImageCloneRequest, SuccessV1) { MockAttachParentRequest mock_attach_parent_request; expect_attach_parent(mock_attach_parent_request, 0); - expect_add_child(m_ioctx, 0); - - MockRefreshRequest mock_refresh_request; - expect_refresh(mock_refresh_request, 0); - expect_is_snap_protected(mock_image_ctx, true, 0); + MockAttachChildRequest mock_attach_child_request; + expect_attach_child(mock_attach_child_request, 1, 0); expect_metadata_list(mock_image_ctx, {{"key", {}}}, 0); expect_metadata_set(m_ioctx, mock_image_ctx, {{"key", {}}}, 0); @@ -432,6 +406,7 @@ TEST_F(TestMockImageCloneRequest, SuccessV1) { TEST_F(TestMockImageCloneRequest, SuccessV2) { REQUIRE_FEATURE(RBD_FEATURE_LAYERING); + ASSERT_EQ(0, _rados.conf_set("rbd_default_clone_format", "2")); MockTestImageCtx mock_image_ctx(*image_ctx); expect_op_work_queue(mock_image_ctx); @@ -450,8 +425,8 @@ TEST_F(TestMockImageCloneRequest, SuccessV2) { MockAttachParentRequest mock_attach_parent_request; expect_attach_parent(mock_attach_parent_request, 0); - expect_op_features_set(m_ioctx, mock_image_ctx.id, 0); - expect_child_attach(mock_image_ctx, 0); + MockAttachChildRequest mock_attach_child_request; + expect_attach_child(mock_attach_child_request, 2, 0); expect_metadata_list(mock_image_ctx, {{"key", {}}}, 0); expect_metadata_set(m_ioctx, mock_image_ctx, {{"key", {}}}, 0); @@ -486,7 +461,6 @@ TEST_F(TestMockImageCloneRequest, SuccessAuto) { expect_op_work_queue(mock_image_ctx); InSequence seq; - expect_get_min_compat_client(1, 0); expect_open(mock_image_ctx, 0); expect_get_image_size(mock_image_ctx, mock_image_ctx.snaps.front(), 123); @@ -500,11 +474,8 @@ TEST_F(TestMockImageCloneRequest, SuccessAuto) { MockAttachParentRequest mock_attach_parent_request; expect_attach_parent(mock_attach_parent_request, 0); - expect_add_child(m_ioctx, 0); - - MockRefreshRequest mock_refresh_request; - expect_refresh(mock_refresh_request, 0); - expect_is_snap_protected(mock_image_ctx, true, 0); + MockAttachChildRequest mock_attach_child_request; + expect_attach_child(mock_attach_child_request, 2, 0); expect_metadata_list(mock_image_ctx, {{"key", {}}}, 0); expect_metadata_set(m_ioctx, mock_image_ctx, {{"key", {}}}, 0); @@ -642,47 +613,8 @@ TEST_F(TestMockImageCloneRequest, AttachParentError) { ASSERT_EQ(-EINVAL, ctx.wait()); } -TEST_F(TestMockImageCloneRequest, AddChildError) { - REQUIRE_FEATURE(RBD_FEATURE_LAYERING); - ASSERT_EQ(0, _rados.conf_set("rbd_default_clone_format", "1")); - - MockTestImageCtx mock_image_ctx(*image_ctx); - expect_op_work_queue(mock_image_ctx); - - InSequence seq; - expect_open(mock_image_ctx, 0); - - expect_get_image_size(mock_image_ctx, mock_image_ctx.snaps.front(), 123); - expect_is_snap_protected(mock_image_ctx, true, 0); - - MockCreateRequest mock_create_request; - expect_create(mock_create_request, 0); - - expect_open(mock_image_ctx, 0); - - MockAttachParentRequest mock_attach_parent_request; - expect_attach_parent(mock_attach_parent_request, 0); - - expect_add_child(m_ioctx, -EINVAL); - expect_close(mock_image_ctx, 0); - - MockRemoveRequest mock_remove_request; - expect_remove(mock_remove_request, 0); - - expect_close(mock_image_ctx, 0); - - C_SaferCond ctx; - ImageOptions clone_opts; - auto req = new MockCloneRequest(m_cct->_conf, m_ioctx, "parent id", "", 123, - m_ioctx, "clone name", "clone id", clone_opts, - "", "", image_ctx->op_work_queue, &ctx); - req->send(); - ASSERT_EQ(-EINVAL, ctx.wait()); -} - -TEST_F(TestMockImageCloneRequest, RefreshError) { +TEST_F(TestMockImageCloneRequest, AttachChildError) { REQUIRE_FEATURE(RBD_FEATURE_LAYERING); - ASSERT_EQ(0, _rados.conf_set("rbd_default_clone_format", "1")); MockTestImageCtx mock_image_ctx(*image_ctx); expect_op_work_queue(mock_image_ctx); @@ -701,10 +633,8 @@ TEST_F(TestMockImageCloneRequest, RefreshError) { MockAttachParentRequest mock_attach_parent_request; expect_attach_parent(mock_attach_parent_request, 0); - expect_add_child(m_ioctx, 0); - - MockRefreshRequest mock_refresh_request; - expect_refresh(mock_refresh_request, -EINVAL); + MockAttachChildRequest mock_attach_child_request; + expect_attach_child(mock_attach_child_request, 2, -EINVAL); expect_close(mock_image_ctx, 0); @@ -724,7 +654,6 @@ TEST_F(TestMockImageCloneRequest, RefreshError) { TEST_F(TestMockImageCloneRequest, MetadataListError) { REQUIRE_FEATURE(RBD_FEATURE_LAYERING); - ASSERT_EQ(0, _rados.conf_set("rbd_default_clone_format", "1")); MockTestImageCtx mock_image_ctx(*image_ctx); expect_op_work_queue(mock_image_ctx); @@ -743,11 +672,8 @@ TEST_F(TestMockImageCloneRequest, MetadataListError) { MockAttachParentRequest mock_attach_parent_request; expect_attach_parent(mock_attach_parent_request, 0); - expect_add_child(m_ioctx, 0); - - MockRefreshRequest mock_refresh_request; - expect_refresh(mock_refresh_request, 0); - expect_is_snap_protected(mock_image_ctx, true, 0); + MockAttachChildRequest mock_attach_child_request; + expect_attach_child(mock_attach_child_request, 2, 0); expect_metadata_list(mock_image_ctx, {{"key", {}}}, -EINVAL); @@ -787,8 +713,8 @@ TEST_F(TestMockImageCloneRequest, MetadataSetError) { MockAttachParentRequest mock_attach_parent_request; expect_attach_parent(mock_attach_parent_request, 0); - expect_op_features_set(m_ioctx, mock_image_ctx.id, 0); - expect_child_attach(mock_image_ctx, 0); + MockAttachChildRequest mock_attach_child_request; + expect_attach_child(mock_attach_child_request, 2, 0); expect_metadata_list(mock_image_ctx, {{"key", {}}}, 0); expect_metadata_set(m_ioctx, mock_image_ctx, {{"key", {}}}, -EINVAL); @@ -811,7 +737,6 @@ TEST_F(TestMockImageCloneRequest, MetadataSetError) { TEST_F(TestMockImageCloneRequest, GetMirrorModeError) { REQUIRE_FEATURE(RBD_FEATURE_LAYERING | RBD_FEATURE_JOURNALING); - ASSERT_EQ(0, _rados.conf_set("rbd_default_clone_format", "1")); MockTestImageCtx mock_image_ctx(*image_ctx); expect_op_work_queue(mock_image_ctx); @@ -830,11 +755,8 @@ TEST_F(TestMockImageCloneRequest, GetMirrorModeError) { MockAttachParentRequest mock_attach_parent_request; expect_attach_parent(mock_attach_parent_request, 0); - expect_add_child(m_ioctx, 0); - - MockRefreshRequest mock_refresh_request; - expect_refresh(mock_refresh_request, 0); - expect_is_snap_protected(mock_image_ctx, true, 0); + MockAttachChildRequest mock_attach_child_request; + expect_attach_child(mock_attach_child_request, 2, 0); expect_metadata_list(mock_image_ctx, {}, 0); @@ -877,8 +799,8 @@ TEST_F(TestMockImageCloneRequest, MirrorEnableError) { MockAttachParentRequest mock_attach_parent_request; expect_attach_parent(mock_attach_parent_request, 0); - expect_op_features_set(m_ioctx, mock_image_ctx.id, 0); - expect_child_attach(mock_image_ctx, 0); + MockAttachChildRequest mock_attach_child_request; + expect_attach_child(mock_attach_child_request, 2, 0); expect_metadata_list(mock_image_ctx, {}, 0); @@ -924,8 +846,8 @@ TEST_F(TestMockImageCloneRequest, CloseError) { MockAttachParentRequest mock_attach_parent_request; expect_attach_parent(mock_attach_parent_request, 0); - expect_op_features_set(m_ioctx, mock_image_ctx.id, 0); - expect_child_attach(mock_image_ctx, 0); + MockAttachChildRequest mock_attach_child_request; + expect_attach_child(mock_attach_child_request, 2, 0); expect_metadata_list(mock_image_ctx, {}, 0); expect_test_features(mock_image_ctx, RBD_FEATURE_JOURNALING, false); -- 2.39.5