From 6141039a7a62e6049736065cc3de7a4e399bba7b Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Tue, 4 Sep 2018 13:25:10 -0400 Subject: [PATCH] librbd: replace existing attach parent calls with new state machine Signed-off-by: Jason Dillaman --- src/librbd/deep_copy/SetHeadRequest.cc | 27 +++--- src/librbd/deep_copy/SetHeadRequest.h | 6 +- src/librbd/image/CloneRequest.cc | 24 +++-- src/librbd/image/CloneRequest.h | 4 +- .../deep_copy/test_mock_SetHeadRequest.cc | 41 +++++++-- .../librbd/image/test_mock_CloneRequest.cc | 87 +++++++++++++++---- 6 files changed, 132 insertions(+), 57 deletions(-) diff --git a/src/librbd/deep_copy/SetHeadRequest.cc b/src/librbd/deep_copy/SetHeadRequest.cc index 16bbb04ad55..34bd93506d2 100644 --- a/src/librbd/deep_copy/SetHeadRequest.cc +++ b/src/librbd/deep_copy/SetHeadRequest.cc @@ -7,6 +7,7 @@ #include "cls/rbd/cls_rbd_types.h" #include "librbd/ExclusiveLock.h" #include "librbd/Utils.h" +#include "librbd/image/AttachParentRequest.h" #include "librbd/image/DetachParentRequest.h" #define dout_subsys ceph_subsys_rbd @@ -106,7 +107,7 @@ void SetHeadRequest::send_detach_parent() { (m_image_ctx->parent_md.spec == m_parent_spec && m_image_ctx->parent_md.overlap == m_parent_overlap)) { m_image_ctx->parent_lock.put_read(); - send_set_parent(); + send_attach_parent(); return; } m_image_ctx->parent_lock.put_read(); @@ -144,11 +145,11 @@ void SetHeadRequest::handle_detach_parent(int r) { m_image_ctx->parent_md.overlap = 0; } - send_set_parent(); + send_attach_parent(); } template -void SetHeadRequest::send_set_parent() { +void SetHeadRequest::send_attach_parent() { m_image_ctx->parent_lock.get_read(); if (m_image_ctx->parent_md.spec == m_parent_spec && m_image_ctx->parent_md.overlap == m_parent_overlap) { @@ -159,10 +160,6 @@ void SetHeadRequest::send_set_parent() { m_image_ctx->parent_lock.put_read(); ldout(m_cct, 20) << dendl; - - librados::ObjectWriteOperation op; - librbd::cls_client::set_parent(&op, m_parent_spec, m_parent_overlap); - auto finish_op_ctx = start_lock_op(); if (finish_op_ctx == nullptr) { lderr(m_cct) << "lost exclusive lock" << dendl; @@ -171,21 +168,23 @@ void SetHeadRequest::send_set_parent() { } auto ctx = new FunctionContext([this, finish_op_ctx](int r) { - handle_set_parent(r); + handle_attach_parent(r); finish_op_ctx->complete(0); }); - librados::AioCompletion *comp = create_rados_callback(ctx); - int r = m_image_ctx->md_ctx.aio_operate(m_image_ctx->header_oid, comp, &op); - ceph_assert(r == 0); - comp->release(); + auto req = image::AttachParentRequest::create( + *m_image_ctx, + {m_parent_spec.pool_id, "", m_parent_spec.image_id, + m_parent_spec.snap_id}, + m_parent_overlap, ctx); + req->send(); } template -void SetHeadRequest::handle_set_parent(int r) { +void SetHeadRequest::handle_attach_parent(int r) { ldout(m_cct, 20) << "r=" << r << dendl; if (r < 0) { - lderr(m_cct) << "failed to set parent: " << cpp_strerror(r) << dendl; + lderr(m_cct) << "failed to attach parent: " << cpp_strerror(r) << dendl; finish(r); return; } diff --git a/src/librbd/deep_copy/SetHeadRequest.h b/src/librbd/deep_copy/SetHeadRequest.h index 14d8013599a..cddad95785a 100644 --- a/src/librbd/deep_copy/SetHeadRequest.h +++ b/src/librbd/deep_copy/SetHeadRequest.h @@ -49,7 +49,7 @@ private: * DETACH_PARENT * | * v (skip if not needed) - * SET_PARENT + * ATTACH_PARENT * | * v * @@ -71,8 +71,8 @@ private: void send_detach_parent(); void handle_detach_parent(int r); - void send_set_parent(); - void handle_set_parent(int r); + void send_attach_parent(); + void handle_attach_parent(int r); Context *start_lock_op(); diff --git a/src/librbd/image/CloneRequest.cc b/src/librbd/image/CloneRequest.cc index 818f2354994..f1b0acb72af 100644 --- a/src/librbd/image/CloneRequest.cc +++ b/src/librbd/image/CloneRequest.cc @@ -7,6 +7,7 @@ #include "common/errno.h" #include "include/ceph_assert.h" #include "librbd/ImageState.h" +#include "librbd/image/AttachParentRequest.h" #include "librbd/image/CloneRequest.h" #include "librbd/image/CreateRequest.h" #include "librbd/image/RemoveRequest.h" @@ -316,30 +317,27 @@ void CloneRequest::handle_open_child(int r) { return; } - set_parent(); + attach_parent(); } template -void CloneRequest::set_parent() { +void CloneRequest::attach_parent() { ldout(m_cct, 20) << dendl; - librados::ObjectWriteOperation op; - librbd::cls_client::set_parent(&op, m_pspec, m_size); - - using klass = CloneRequest; - librados::AioCompletion *comp = - create_rados_callback(this); - int r = m_imctx->md_ctx.aio_operate(m_imctx->header_oid, comp, &op); - ceph_assert(r == 0); - comp->release(); + auto ctx = create_context_callback< + CloneRequest, &CloneRequest::handle_attach_parent>(this); + auto req = AttachParentRequest::create( + *m_imctx, {m_pspec.pool_id, "", m_pspec.image_id, m_pspec.snap_id}, + m_size, ctx); + req->send(); } template -void CloneRequest::handle_set_parent(int r) { +void CloneRequest::handle_attach_parent(int r) { ldout(m_cct, 20) << "r=" << r << dendl; if (r < 0) { - lderr(m_cct) << "couldn't set parent: " << cpp_strerror(r) << dendl; + lderr(m_cct) << "failed to attach parent: " << cpp_strerror(r) << dendl; m_r_saved = r; close_child(); return; diff --git a/src/librbd/image/CloneRequest.h b/src/librbd/image/CloneRequest.h index 3707d9dc05a..afcbbd2386b 100644 --- a/src/librbd/image/CloneRequest.h +++ b/src/librbd/image/CloneRequest.h @@ -149,8 +149,8 @@ private: void open_child(); void handle_open_child(int r); - void set_parent(); - void handle_set_parent(int r); + void attach_parent(); + void handle_attach_parent(int r); void v2_set_op_feature(); void handle_v2_set_op_feature(int r); diff --git a/src/test/librbd/deep_copy/test_mock_SetHeadRequest.cc b/src/test/librbd/deep_copy/test_mock_SetHeadRequest.cc index c94ccf4214e..3eef98a6117 100644 --- a/src/test/librbd/deep_copy/test_mock_SetHeadRequest.cc +++ b/src/test/librbd/deep_copy/test_mock_SetHeadRequest.cc @@ -10,6 +10,7 @@ #include "test/librados_test_stub/MockTestMemIoCtxImpl.h" #include "test/librbd/mock/MockImageCtx.h" #include "librbd/deep_copy/SetHeadRequest.h" +#include "librbd/image/AttachParentRequest.h" #include "librbd/image/DetachParentRequest.h" namespace librbd { @@ -25,6 +26,28 @@ struct MockTestImageCtx : public librbd::MockImageCtx { namespace image { +template <> +struct AttachParentRequest { + Context* on_finish = nullptr; + static AttachParentRequest* s_instance; + static AttachParentRequest* create(MockTestImageCtx&, + const cls::rbd::ParentImageSpec& pspec, + uint64_t parent_overlap, + Context *on_finish) { + ceph_assert(s_instance != nullptr); + s_instance->on_finish = on_finish; + return s_instance; + } + + MOCK_METHOD0(send, void()); + + AttachParentRequest() { + s_instance = this; + } +}; + +AttachParentRequest* AttachParentRequest::s_instance = nullptr; + template <> class DetachParentRequest { public: @@ -70,6 +93,7 @@ using ::testing::WithArg; class TestMockDeepCopySetHeadRequest : public TestMockFixture { public: typedef SetHeadRequest MockSetHeadRequest; + typedef image::AttachParentRequest MockAttachParentRequest; typedef image::DetachParentRequest MockDetachParentRequest; librbd::ImageCtx *m_image_ctx; @@ -108,10 +132,10 @@ public: .WillOnce(FinishRequest(&mock_request, r, &mock_image_ctx)); } - void expect_set_parent(librbd::MockTestImageCtx &mock_image_ctx, int r) { - EXPECT_CALL(get_mock_io_ctx(mock_image_ctx.md_ctx), - exec(mock_image_ctx.header_oid, _, StrEq("rbd"), StrEq("set_parent"), _, _, _)) - .WillOnce(Return(r)); + void expect_attach_parent(MockImageCtx &mock_image_ctx, + MockAttachParentRequest& mock_request, int r) { + EXPECT_CALL(mock_request, send()) + .WillOnce(FinishRequest(&mock_request, r, &mock_image_ctx)); } MockSetHeadRequest *create_request( @@ -201,7 +225,8 @@ TEST_F(TestMockDeepCopySetHeadRequest, RemoveSetParent) { MockDetachParentRequest mock_detach_parent; expect_detach_parent(mock_image_ctx, mock_detach_parent, 0); expect_start_op(mock_exclusive_lock); - expect_set_parent(mock_image_ctx, 0); + MockAttachParentRequest mock_attach_parent; + expect_attach_parent(mock_image_ctx, mock_attach_parent, 0); C_SaferCond ctx; auto request = create_request(mock_image_ctx, m_image_ctx->size, @@ -217,7 +242,8 @@ TEST_F(TestMockDeepCopySetHeadRequest, SetParentSpec) { InSequence seq; expect_start_op(mock_exclusive_lock); - expect_set_parent(mock_image_ctx, 0); + MockAttachParentRequest mock_attach_parent; + expect_attach_parent(mock_image_ctx, mock_attach_parent, 0); C_SaferCond ctx; auto request = create_request(mock_image_ctx, m_image_ctx->size, @@ -253,7 +279,8 @@ TEST_F(TestMockDeepCopySetHeadRequest, SetParentError) { InSequence seq; expect_start_op(mock_exclusive_lock); - expect_set_parent(mock_image_ctx, -ESTALE); + MockAttachParentRequest mock_attach_parent; + expect_attach_parent(mock_image_ctx, mock_attach_parent, -ESTALE); C_SaferCond ctx; auto request = create_request(mock_image_ctx, m_image_ctx->size, diff --git a/src/test/librbd/image/test_mock_CloneRequest.cc b/src/test/librbd/image/test_mock_CloneRequest.cc index 94b89265d72..7a103a1ecc2 100644 --- a/src/test/librbd/image/test_mock_CloneRequest.cc +++ b/src/test/librbd/image/test_mock_CloneRequest.cc @@ -10,6 +10,7 @@ #include "librbd/ImageState.h" #include "librbd/Operations.h" #include "librbd/image/TypeTraits.h" +#include "librbd/image/AttachParentRequest.h" #include "librbd/image/CreateRequest.h" #include "librbd/image/RemoveRequest.h" #include "librbd/image/RefreshRequest.h" @@ -48,6 +49,28 @@ MockTestImageCtx* MockTestImageCtx::s_instance = nullptr; namespace image { +template <> +struct AttachParentRequest { + Context* on_finish = nullptr; + static AttachParentRequest* s_instance; + static AttachParentRequest* create(MockTestImageCtx&, + const cls::rbd::ParentImageSpec& pspec, + uint64_t parent_overlap, + Context *on_finish) { + ceph_assert(s_instance != nullptr); + s_instance->on_finish = on_finish; + return s_instance; + } + + MOCK_METHOD0(send, void()); + + AttachParentRequest() { + s_instance = this; + } +}; + +AttachParentRequest* AttachParentRequest::s_instance = nullptr; + template <> struct CreateRequest { Context* on_finish = nullptr; @@ -167,6 +190,7 @@ using ::testing::WithArg; class TestMockImageCloneRequest : public TestMockFixture { public: typedef CloneRequest MockCloneRequest; + typedef AttachParentRequest MockAttachParentRequest; typedef CreateRequest MockCreateRequest; typedef RefreshRequest MockRefreshRequest; typedef RemoveRequest MockRemoveRequest; @@ -236,12 +260,10 @@ public: } } - void expect_set_parent(MockImageCtx &mock_image_ctx, int r) { - EXPECT_CALL(get_mock_io_ctx(mock_image_ctx.md_ctx), - exec(mock_image_ctx.header_oid, _, StrEq("rbd"), - StrEq("set_parent"), _, _, _)) - .WillOnce(InvokeWithoutArgs([r]() { - return r; + void expect_attach_parent(MockAttachParentRequest& mock_request, int r) { + EXPECT_CALL(mock_request, send()) + .WillOnce(Invoke([this, &mock_request, r]() { + image_ctx->op_work_queue->queue(mock_request.on_finish, r); })); } @@ -372,7 +394,10 @@ TEST_F(TestMockImageCloneRequest, SuccessV1) { expect_create(mock_create_request, 0); expect_open(mock_image_ctx, 0); - expect_set_parent(mock_image_ctx, 0); + + MockAttachParentRequest mock_attach_parent_request; + expect_attach_parent(mock_attach_parent_request, 0); + expect_add_child(m_ioctx, 0); MockRefreshRequest mock_refresh_request; @@ -420,7 +445,9 @@ TEST_F(TestMockImageCloneRequest, SuccessV2) { expect_create(mock_create_request, 0); expect_open(mock_image_ctx, 0); - expect_set_parent(mock_image_ctx, 0); + + 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); @@ -468,7 +495,10 @@ TEST_F(TestMockImageCloneRequest, SuccessAuto) { expect_create(mock_create_request, 0); expect_open(mock_image_ctx, 0); - expect_set_parent(mock_image_ctx, 0); + + MockAttachParentRequest mock_attach_parent_request; + expect_attach_parent(mock_attach_parent_request, 0); + expect_add_child(m_ioctx, 0); MockRefreshRequest mock_refresh_request; @@ -575,7 +605,7 @@ TEST_F(TestMockImageCloneRequest, OpenError) { ASSERT_EQ(-EINVAL, ctx.wait()); } -TEST_F(TestMockImageCloneRequest, SetParentError) { +TEST_F(TestMockImageCloneRequest, AttachParentError) { REQUIRE_FEATURE(RBD_FEATURE_LAYERING); MockTestImageCtx mock_image_ctx(*image_ctx); @@ -591,7 +621,10 @@ TEST_F(TestMockImageCloneRequest, SetParentError) { expect_create(mock_create_request, 0); expect_open(mock_image_ctx, 0); - expect_set_parent(mock_image_ctx, -EINVAL); + + MockAttachParentRequest mock_attach_parent_request; + expect_attach_parent(mock_attach_parent_request, -EINVAL); + expect_close(mock_image_ctx, 0); MockRemoveRequest mock_remove_request; @@ -625,7 +658,10 @@ TEST_F(TestMockImageCloneRequest, AddChildError) { expect_create(mock_create_request, 0); expect_open(mock_image_ctx, 0); - expect_set_parent(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); @@ -660,7 +696,10 @@ TEST_F(TestMockImageCloneRequest, RefreshError) { expect_create(mock_create_request, 0); expect_open(mock_image_ctx, 0); - expect_set_parent(mock_image_ctx, 0); + + MockAttachParentRequest mock_attach_parent_request; + expect_attach_parent(mock_attach_parent_request, 0); + expect_add_child(m_ioctx, 0); MockRefreshRequest mock_refresh_request; @@ -699,7 +738,10 @@ TEST_F(TestMockImageCloneRequest, MetadataListError) { expect_create(mock_create_request, 0); expect_open(mock_image_ctx, 0); - expect_set_parent(mock_image_ctx, 0); + + MockAttachParentRequest mock_attach_parent_request; + expect_attach_parent(mock_attach_parent_request, 0); + expect_add_child(m_ioctx, 0); MockRefreshRequest mock_refresh_request; @@ -740,7 +782,9 @@ TEST_F(TestMockImageCloneRequest, MetadataSetError) { expect_create(mock_create_request, 0); expect_open(mock_image_ctx, 0); - expect_set_parent(mock_image_ctx, 0); + + 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); @@ -781,7 +825,10 @@ TEST_F(TestMockImageCloneRequest, GetMirrorModeError) { expect_create(mock_create_request, 0); expect_open(mock_image_ctx, 0); - expect_set_parent(mock_image_ctx, 0); + + MockAttachParentRequest mock_attach_parent_request; + expect_attach_parent(mock_attach_parent_request, 0); + expect_add_child(m_ioctx, 0); MockRefreshRequest mock_refresh_request; @@ -825,7 +872,9 @@ TEST_F(TestMockImageCloneRequest, MirrorEnableError) { expect_create(mock_create_request, 0); expect_open(mock_image_ctx, 0); - expect_set_parent(mock_image_ctx, 0); + + 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); @@ -870,7 +919,9 @@ TEST_F(TestMockImageCloneRequest, CloseError) { expect_create(mock_create_request, 0); expect_open(mock_image_ctx, 0); - expect_set_parent(mock_image_ctx, 0); + + 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); -- 2.39.5