librbd: replace attach child call with new state machine

Signed-off-by: Mykola Golub <mgolub@suse.com>

diff --git a/src/librbd/image/CloneRequest.cc b/src/librbd/image/CloneRequest.cc
index eccf7a53048e502b3c5fc6cf6275b65651ee0e30..d36fa0bb723fe8c58236a308b42feaf37c281874 100644 (file)
--- 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<I>::handle_attach_parent(int r) {
     return;
   }
 
-  v2_set_op_feature();
+  attach_child();
 }
 
 template <typename I>
-void CloneRequest<I>::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<I>, &CloneRequest<I>::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 <typename I>
-void CloneRequest<I>::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 <typename I>
-void CloneRequest<I>::v2_child_attach() {
+void CloneRequest<I>::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<I>, &CloneRequest<I>::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 <typename I>
-void CloneRequest<I>::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 <typename I>
-void CloneRequest<I>::v1_add_child() {
-  ldout(m_cct, 15) << dendl;
-
-  librados::ObjectWriteOperation op;
-  cls_client::add_child(&op, m_pspec, m_id);
-
-  using klass = CloneRequest<I>;
-  librados::AioCompletion *comp =
-    create_rados_callback<klass, &klass::handle_v1_add_child>(this);
-  int r = m_ioctx.aio_operate(RBD_CHILDREN, comp, &op);
-  ceph_assert(r == 0);
-  comp->release();
+  auto ctx = create_context_callback<
+    CloneRequest<I>, &CloneRequest<I>::handle_attach_child>(this);
+  auto req = AttachChildRequest<I>::create(
+    m_imctx, m_parent_image_ctx, m_parent_image_ctx->snap_id, nullptr, 0,
+    m_clone_format, ctx);
+  req->send();
 }
 
 template <typename I>
-void CloneRequest<I>::handle_v1_add_child(int r) {
+void CloneRequest<I>::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 <typename I>
-void CloneRequest<I>::v1_refresh() {
-  ldout(m_cct, 15) << dendl;
-
-  using klass = CloneRequest<I>;
-  RefreshRequest<I> *req = RefreshRequest<I>::create(
-    *m_imctx, false, false,
-    create_context_callback<klass, &klass::handle_v1_refresh>(this));
-  req->send();
-}
-
-template <typename I>
-void CloneRequest<I>::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 7f4c0ee09d715e10966409884b9c22afff026bbc..aa03d97f0fb6a3f9639c1f0bca5fbcb05c51b480 100644 (file)
--- 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 673d7dfe2beedfc645f0dbdeddd11f00c0071f12..fd63ded44778f2053d0fc2f88f688aeb97d25ca5 100644 (file)
--- 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<MockTestImageCtx> {
+  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<MockTestImageCtx>* AttachChildRequest<MockTestImageCtx>::s_instance = nullptr;
+
 template <>
 struct AttachParentRequest<MockTestImageCtx> {
   Context* on_finish = nullptr;
@@ -98,27 +125,6 @@ struct CreateRequest<MockTestImageCtx> {
 
 CreateRequest<MockTestImageCtx>* CreateRequest<MockTestImageCtx>::s_instance = nullptr;
 
-template <>
-struct RefreshRequest<MockTestImageCtx> {
-  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<MockTestImageCtx>* RefreshRequest<MockTestImageCtx>::s_instance = nullptr;
-
 template <>
 struct RemoveRequest<MockTestImageCtx> {
   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<MockTestImageCtx> MockCloneRequest;
+  typedef AttachChildRequest<MockTestImageCtx> MockAttachChildRequest;
   typedef AttachParentRequest<MockTestImageCtx> MockAttachParentRequest;
   typedef CreateRequest<MockTestImageCtx> MockCreateRequest;
-  typedef RefreshRequest<MockTestImageCtx> MockRefreshRequest;
   typedef RemoveRequest<MockTestImageCtx> MockRemoveRequest;
   typedef mirror::EnableRequest<MockTestImageCtx> 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<uint64_t>(RBD_OPERATION_FEATURE_CLONE_CHILD), bl);
-    encode(static_cast<uint64_t>(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);