librbd: use 'open snap by id' in clone request

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

diff --git a/src/librbd/image/CloneRequest.cc b/src/librbd/image/CloneRequest.cc
index 7d6ef5764eaf408b535913f8f1b9f39193458ecd..279a46d58dc1dd8eacb834daad34668a37d6dc89 100644 (file)
--- a/src/librbd/image/CloneRequest.cc
+++ b/src/librbd/image/CloneRequest.cc
@@ -117,8 +117,16 @@ void CloneRequest<I>::validate_options() {
 template <typename I>
 void CloneRequest<I>::open_parent() {
   ldout(m_cct, 20) << dendl;
+  assert(m_parent_snap_name.empty() ^ (m_parent_snap_id == CEPH_NOSNAP));
 
-  m_parent_image_ctx = I::create("", m_parent_image_id, "", m_parent_io_ctx, true);
+  if (m_parent_snap_id != CEPH_NOSNAP) {
+    m_parent_image_ctx = I::create("", m_parent_image_id, m_parent_snap_id,
+                                   m_parent_io_ctx, true);
+  } else {
+    m_parent_image_ctx = I::create("", m_parent_image_id,
+                                   m_parent_snap_name.c_str(), m_parent_io_ctx,
+                                   true);
+  }
 
   Context *ctx = create_context_callback<
     CloneRequest<I>, &CloneRequest<I>::handle_open_parent>(this);
@@ -138,52 +146,7 @@ void CloneRequest<I>::handle_open_parent(int r) {
     return;
   }
 
-  set_parent_snap();
-}
-
-template <typename I>
-void CloneRequest<I>::set_parent_snap() {
-  assert((m_parent_snap_name.empty()) ^ (m_parent_snap_id == CEPH_NOSNAP));
-
-  if (m_parent_snap_id == CEPH_NOSNAP) {
-    // look up user snapshot by name
-    m_parent_image_ctx->snap_lock.get_read();
-    auto it = m_parent_image_ctx->snap_ids.find(
-      {cls::rbd::UserSnapshotNamespace{}, m_parent_snap_name});
-    if (it == m_parent_image_ctx->snap_ids.end()) {
-      m_parent_image_ctx->snap_lock.put_read();
-
-      lderr(m_cct) << "failed to located parent snapshot: " <<
-                   m_parent_snap_name << dendl;
-      m_r_saved = -ENOENT;
-      close_parent();
-      return;
-    }
-
-    m_parent_snap_id = it->second;
-    m_parent_image_ctx->snap_lock.put_read();
-  }
-
-  ldout(m_cct, 20) << "parent_snap_id=" << m_parent_snap_id << dendl;
-
-  Context *ctx = create_context_callback<
-    CloneRequest<I>, &CloneRequest<I>::handle_set_parent_snap>(this);
-  m_parent_image_ctx->state->snap_set(m_parent_snap_id, ctx);
-}
-
-template <typename I>
-void CloneRequest<I>::handle_set_parent_snap(int r) {
-  ldout(m_cct, 20) << "r=" << r << dendl;
-
-  if (r < 0) {
-    lderr(m_cct) << "failed to set parent snapshot: " << cpp_strerror(r)
-                 << dendl;
-
-    m_r_saved = r;
-    close_parent();
-    return;
-  }
-
+  m_parent_snap_id = m_parent_image_ctx->snap_id;
   m_pspec = {m_parent_io_ctx.get_id(), m_parent_image_id, m_parent_snap_id};
   validate_parent();
 }

diff --git a/src/librbd/image/CloneRequest.h b/src/librbd/image/CloneRequest.h
index d98ebc4f6487facb3d5e6b5fcab6c1a50f89b2c7..3707d9dc05a48561d23080af789a68a16b674cbd 100644 (file)
--- a/src/librbd/image/CloneRequest.h
+++ b/src/librbd/image/CloneRequest.h
@@ -52,17 +52,14 @@ private:
    * <start>
    *    |
    *    v
-   * OPEN PARENT                       <finish>
+   * OPEN PARENT
+   *    |
+   *    v
+   * VALIDATE CHILD                    <finish>
    *    |                                 ^
    *    v                                 |
-   * SET PARENT SNAP  * * * * * * * > CLOSE PARENT
-   *    |                       *         ^
-   *    v                       *         |
-   * VALIDATE CHILD             *         |
-   *    |                       *         |
-   *    v                       *         |
-   * CREATE CHILD * * * * * * * *         |
-   *    |                                 |
+   * CREATE CHILD * * * * * * * * * > CLOSE PARENT
+   *    |                                 ^
    *    v                                 |
    * OPEN CHILD * * * * * * * * * * > REMOVE CHILD
    *    |                                 ^
@@ -141,9 +138,6 @@ private:
   void open_parent();
   void handle_open_parent(int r);
 
-  void set_parent_snap();
-  void handle_set_parent_snap(int r);
-
   void validate_parent();
 
   void validate_child();

diff --git a/src/test/librbd/image/test_mock_CloneRequest.cc b/src/test/librbd/image/test_mock_CloneRequest.cc
index f149008ebbb5f88052c9d20768b92caa9578876a..8c4d27070131688b14552ef3dda7f7fb6965198b 100644 (file)
--- a/src/test/librbd/image/test_mock_CloneRequest.cc
+++ b/src/test/librbd/image/test_mock_CloneRequest.cc
@@ -29,6 +29,13 @@ struct MockTestImageCtx : public MockImageCtx {
     assert(s_instance != nullptr);
     return s_instance;
   }
+  static MockTestImageCtx* create(const std::string &image_name,
+                                  const std::string &image_id,
+                                  librados::snap_t snap_id, IoCtx& p,
+                                  bool read_only) {
+    assert(s_instance != nullptr);
+    return s_instance;
+  }
 
   MockTestImageCtx(ImageCtx &image_ctx) : MockImageCtx(image_ctx) {
     s_instance = this;
@@ -229,14 +236,6 @@ public:
     }
   }
 
-  void expect_snap_set(librbd::MockTestImageCtx &mock_image_ctx,
-                       uint64_t snap_id, int r) {
-    EXPECT_CALL(*mock_image_ctx.state, snap_set(snap_id, _))
-      .WillOnce(WithArg<1>(Invoke([this, r](Context *on_finish) {
-          image_ctx->op_work_queue->queue(on_finish, r);
-        })));
-  }
-
   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"),
@@ -364,7 +363,6 @@ TEST_F(TestMockImageCloneRequest, SuccessV1) {
 
   InSequence seq;
   expect_open(mock_image_ctx, 0);
-  expect_snap_set(mock_image_ctx, 123, 0);
 
   expect_get_image_size(mock_image_ctx, mock_image_ctx.snaps.front(), 123);
   expect_is_snap_protected(mock_image_ctx, true, 0);
@@ -413,7 +411,6 @@ TEST_F(TestMockImageCloneRequest, SuccessV2) {
 
   InSequence seq;
   expect_open(mock_image_ctx, 0);
-  expect_snap_set(mock_image_ctx, 123, 0);
 
   expect_get_image_size(mock_image_ctx, mock_image_ctx.snaps.front(), 123);
   expect_is_snap_protected(mock_image_ctx, true, 0);
@@ -462,7 +459,6 @@ TEST_F(TestMockImageCloneRequest, SuccessAuto) {
   InSequence seq;
   expect_get_min_compat_client(1, 0);
   expect_open(mock_image_ctx, 0);
-  expect_snap_set(mock_image_ctx, 123, 0);
 
   expect_get_image_size(mock_image_ctx, mock_image_ctx.snaps.front(), 123);
   expect_is_snap_protected(mock_image_ctx, true, 0);
@@ -521,26 +517,6 @@ TEST_F(TestMockImageCloneRequest, OpenParentError) {
   ASSERT_EQ(-EINVAL, ctx.wait());
 }
 
-TEST_F(TestMockImageCloneRequest, SetParentSnapError) {
-  REQUIRE_FEATURE(RBD_FEATURE_LAYERING);
-
-  MockTestImageCtx mock_image_ctx(*image_ctx);
-  expect_op_work_queue(mock_image_ctx);
-
-  InSequence seq;
-  expect_open(mock_image_ctx, 0);
-  expect_snap_set(mock_image_ctx, 123, -EINVAL);
-  expect_close(mock_image_ctx, 0);
-
-  C_SaferCond ctx;
-  ImageOptions clone_opts;
-  auto req = new MockCloneRequest(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, CreateError) {
   REQUIRE_FEATURE(RBD_FEATURE_LAYERING);
 
@@ -549,7 +525,6 @@ TEST_F(TestMockImageCloneRequest, CreateError) {
 
   InSequence seq;
   expect_open(mock_image_ctx, 0);
-  expect_snap_set(mock_image_ctx, 123, 0);
 
   expect_get_image_size(mock_image_ctx, mock_image_ctx.snaps.front(), 123);
   expect_is_snap_protected(mock_image_ctx, true, 0);
@@ -576,7 +551,6 @@ TEST_F(TestMockImageCloneRequest, OpenError) {
 
   InSequence seq;
   expect_open(mock_image_ctx, 0);
-  expect_snap_set(mock_image_ctx, 123, 0);
 
   expect_get_image_size(mock_image_ctx, mock_image_ctx.snaps.front(), 123);
   expect_is_snap_protected(mock_image_ctx, true, 0);
@@ -608,7 +582,6 @@ TEST_F(TestMockImageCloneRequest, SetParentError) {
 
   InSequence seq;
   expect_open(mock_image_ctx, 0);
-  expect_snap_set(mock_image_ctx, 123, 0);
 
   expect_get_image_size(mock_image_ctx, mock_image_ctx.snaps.front(), 123);
   expect_is_snap_protected(mock_image_ctx, true, 0);
@@ -643,7 +616,6 @@ TEST_F(TestMockImageCloneRequest, AddChildError) {
 
   InSequence seq;
   expect_open(mock_image_ctx, 0);
-  expect_snap_set(mock_image_ctx, 123, 0);
 
   expect_get_image_size(mock_image_ctx, mock_image_ctx.snaps.front(), 123);
   expect_is_snap_protected(mock_image_ctx, true, 0);
@@ -679,7 +651,6 @@ TEST_F(TestMockImageCloneRequest, RefreshError) {
 
   InSequence seq;
   expect_open(mock_image_ctx, 0);
-  expect_snap_set(mock_image_ctx, 123, 0);
 
   expect_get_image_size(mock_image_ctx, mock_image_ctx.snaps.front(), 123);
   expect_is_snap_protected(mock_image_ctx, true, 0);
@@ -719,7 +690,6 @@ TEST_F(TestMockImageCloneRequest, MetadataListError) {
 
   InSequence seq;
   expect_open(mock_image_ctx, 0);
-  expect_snap_set(mock_image_ctx, 123, 0);
 
   expect_get_image_size(mock_image_ctx, mock_image_ctx.snaps.front(), 123);
   expect_is_snap_protected(mock_image_ctx, true, 0);
@@ -761,7 +731,6 @@ TEST_F(TestMockImageCloneRequest, MetadataSetError) {
 
   InSequence seq;
   expect_open(mock_image_ctx, 0);
-  expect_snap_set(mock_image_ctx, 123, 0);
 
   expect_get_image_size(mock_image_ctx, mock_image_ctx.snaps.front(), 123);
   expect_is_snap_protected(mock_image_ctx, true, 0);
@@ -803,7 +772,6 @@ TEST_F(TestMockImageCloneRequest, GetMirrorModeError) {
 
   InSequence seq;
   expect_open(mock_image_ctx, 0);
-  expect_snap_set(mock_image_ctx, 123, 0);
 
   expect_get_image_size(mock_image_ctx, mock_image_ctx.snaps.front(), 123);
   expect_is_snap_protected(mock_image_ctx, true, 0);
@@ -848,7 +816,6 @@ TEST_F(TestMockImageCloneRequest, MirrorEnableError) {
 
   InSequence seq;
   expect_open(mock_image_ctx, 0);
-  expect_snap_set(mock_image_ctx, 123, 0);
 
   expect_get_image_size(mock_image_ctx, mock_image_ctx.snaps.front(), 123);
   expect_is_snap_protected(mock_image_ctx, true, 0);
@@ -894,7 +861,6 @@ TEST_F(TestMockImageCloneRequest, CloseError) {
 
   InSequence seq;
   expect_open(mock_image_ctx, 0);
-  expect_snap_set(mock_image_ctx, 123, 0);
 
   expect_get_image_size(mock_image_ctx, mock_image_ctx.snaps.front(), 123);
   expect_is_snap_protected(mock_image_ctx, true, 0);
@@ -935,7 +901,6 @@ TEST_F(TestMockImageCloneRequest, RemoveError) {
 
   InSequence seq;
   expect_open(mock_image_ctx, 0);
-  expect_snap_set(mock_image_ctx, 123, 0);
 
   expect_get_image_size(mock_image_ctx, mock_image_ctx.snaps.front(), 123);
   expect_is_snap_protected(mock_image_ctx, true, 0);
@@ -967,7 +932,6 @@ TEST_F(TestMockImageCloneRequest, CloseParentError) {
 
   InSequence seq;
   expect_open(mock_image_ctx, 0);
-  expect_snap_set(mock_image_ctx, 123, 0);
 
   expect_get_image_size(mock_image_ctx, mock_image_ctx.snaps.front(), 123);
   expect_is_snap_protected(mock_image_ctx, true, 0);