]> git.apps.os.sepia.ceph.com Git - ceph.git/commitdiff
librbd: internally utilize snap id when setting snapshot
authorJason Dillaman <dillaman@redhat.com>
Tue, 20 Mar 2018 03:13:34 +0000 (23:13 -0400)
committerJason Dillaman <dillaman@redhat.com>
Tue, 20 Mar 2018 04:33:00 +0000 (12:33 +0800)
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
17 files changed:
src/librbd/ImageCtx.cc
src/librbd/ImageCtx.h
src/librbd/ImageState.cc
src/librbd/ImageState.h
src/librbd/api/Image.cc
src/librbd/image/OpenRequest.cc
src/librbd/image/RefreshParentRequest.cc
src/librbd/image/SetSnapRequest.cc
src/librbd/image/SetSnapRequest.h
src/librbd/internal.cc
src/test/librbd/image/test_mock_CloneRequest.cc
src/test/librbd/mock/MockImageState.h
src/test/rbd_mirror/image_replayer/test_mock_CreateImageRequest.cc
src/test/rbd_mirror/test_ImageSync.cc
src/test/rbd_mirror/test_mock_ImageSync.cc
src/tools/rbd_mirror/image_replayer/CreateImageRequest.cc
src/tools/rbd_mirror/image_replayer/CreateImageRequest.h

index 7d64c5047f8b97736fb330e1a4bf11f7d70caadf..254f3573230962df705d3980f03861252cc75162 100644 (file)
@@ -312,15 +312,13 @@ public:
     return flags;
   }
 
-  int ImageCtx::snap_set(cls::rbd::SnapshotNamespace in_snap_namespace,
-                        string in_snap_name)
-  {
+  int ImageCtx::snap_set(uint64_t in_snap_id) {
     assert(snap_lock.is_wlocked());
-    snap_t in_snap_id = get_snap_id(in_snap_namespace, in_snap_name);
-    if (in_snap_id != CEPH_NOSNAP) {
+    auto it = snap_info.find(in_snap_id);
+    if (in_snap_id != CEPH_NOSNAP && it != snap_info.end()) {
       snap_id = in_snap_id;
-      snap_namespace = in_snap_namespace;
-      snap_name = in_snap_name;
+      snap_namespace = it->second.snap_namespace;
+      snap_name = it->second.name;
       snap_exists = true;
       data_ctx.snap_set_read(snap_id);
       return 0;
index 4ef9a2005d3012ae002cc28a18813e13f4827713..e0e4ec556aa5691386291801eb395d6538aa5592 100644 (file)
@@ -234,8 +234,7 @@ namespace librbd {
     void perf_stop();
     void set_read_flag(unsigned flag);
     int get_read_flags(librados::snap_t snap_id);
-    int snap_set(cls::rbd::SnapshotNamespace in_snap_namespace,
-                std::string in_snap_name);
+    int snap_set(uint64_t snap_id);
     void snap_unset();
     librados::snap_t get_snap_id(const cls::rbd::SnapshotNamespace& in_snap_namespace,
                                  const std::string& in_snap_name) const;
index 9ac9f1570a26be1445d1669163d1ea4db29c2ec5..17d364cc94a3c5bec6f209190f058cda136097f5 100644 (file)
@@ -381,15 +381,12 @@ ImageState<I>::find_pending_refresh() const {
 }
 
 template <typename I>
-void ImageState<I>::snap_set(const cls::rbd::SnapshotNamespace &snap_namespace,
-                            const std::string &snap_name,
-                            Context *on_finish) {
+void ImageState<I>::snap_set(uint64_t snap_id, Context *on_finish) {
   CephContext *cct = m_image_ctx->cct;
-  ldout(cct, 20) << __func__ << ": snap_name=" << snap_name << dendl;
+  ldout(cct, 20) << __func__ << ": snap_id=" << snap_id << dendl;
 
   Action action(ACTION_TYPE_SET_SNAP);
-  action.snap_namespace = snap_namespace;
-  action.snap_name = snap_name;
+  action.snap_id = snap_id;
 
   m_lock.Lock();
   execute_action_unlock(action, on_finish);
@@ -691,13 +688,13 @@ void ImageState<I>::send_set_snap_unlock() {
 
   CephContext *cct = m_image_ctx->cct;
   ldout(cct, 10) << this << " " << __func__ << ": "
-                 << "snap_name=" << action_contexts.first.snap_name << dendl;
+                 << "snap_id=" << action_contexts.first.snap_id << dendl;
 
   Context *ctx = create_async_context_callback(
     *m_image_ctx, create_context_callback<
       ImageState<I>, &ImageState<I>::handle_set_snap>(this));
   image::SetSnapRequest<I> *req = image::SetSnapRequest<I>::create(
-    *m_image_ctx, action_contexts.first.snap_namespace, action_contexts.first.snap_name, ctx);
+    *m_image_ctx, action_contexts.first.snap_id, ctx);
 
   m_lock.Unlock();
   req->send();
index 2f4d3e33d13c8936fc73a9d1cc88e871af64ec2f..d577f290d92028fbb391dde9cd6336ed68411b6d 100644 (file)
@@ -40,9 +40,7 @@ public:
   int refresh_if_required();
   void refresh(Context *on_finish);
 
-  void snap_set(const cls::rbd::SnapshotNamespace &snap_namespace,
-               const std::string &snap_name,
-               Context *on_finish);
+  void snap_set(uint64_t snap_id, Context *on_finish);
 
   void prepare_lock(Context *on_ready);
   void handle_prepare_lock_complete();
@@ -75,8 +73,7 @@ private:
   struct Action {
     ActionType action_type;
     uint64_t refresh_seq = 0;
-    cls::rbd::SnapshotNamespace snap_namespace;
-    std::string snap_name;
+    uint64_t snap_id = CEPH_NOSNAP;
     Context *on_ready = nullptr;
 
     Action(ActionType action_type) : action_type(action_type) {
@@ -89,7 +86,7 @@ private:
       case ACTION_TYPE_REFRESH:
         return (refresh_seq == action.refresh_seq);
       case ACTION_TYPE_SET_SNAP:
-        return (snap_name == action.snap_name) && (snap_namespace == action.snap_namespace);
+        return (snap_id == action.snap_id);
       case ACTION_TYPE_LOCK:
         return false;
       default:
index e918893c31c16d04865ae68bc51963382ff51c8e..4213383ac42cae63814c4899cf5f1424d586fa35 100644 (file)
@@ -240,19 +240,9 @@ int Image<I>::deep_copy(I *src, librados::IoCtx& dest_md_ctx,
       }
       return r;
     }
-    std::string snap_name;
-    {
-      RWLock::RLocker parent_snap_locker(src_parent_image_ctx->snap_lock);
-      auto it = src_parent_image_ctx->snap_info.find(parent_spec.snap_id);
-      if (it == src_parent_image_ctx->snap_info.end()) {
-        return -ENOENT;
-      }
-      snap_name = it->second.name;
-    }
 
     C_SaferCond cond;
-    src_parent_image_ctx->state->snap_set(cls::rbd::UserSnapshotNamespace(),
-                                          snap_name, &cond);
+    src_parent_image_ctx->state->snap_set(parent_spec.snap_id, &cond);
     r = cond.wait();
     if (r < 0) {
       if (r != -ENOENT) {
index b5e7415a4c45521ad6aaa65bf2b6edddbdd77114..26e909f89242517ece6f76f1b8bc9dd007a42346 100644 (file)
@@ -508,9 +508,20 @@ Context *OpenRequest<I>::send_set_snap(int *result) {
   CephContext *cct = m_image_ctx->cct;
   ldout(cct, 10) << this << " " << __func__ << dendl;
 
+  uint64_t snap_id = CEPH_NOSNAP;
+  {
+    RWLock::RLocker snap_locker(m_image_ctx->snap_lock);
+    snap_id = m_image_ctx->get_snap_id(m_image_ctx->snap_namespace,
+                                       m_image_ctx->snap_name);
+  }
+  if (snap_id == CEPH_NOSNAP) {
+    *result = -ENOENT;
+    return m_on_finish;
+  }
+
   using klass = OpenRequest<I>;
   SetSnapRequest<I> *req = SetSnapRequest<I>::create(
-    *m_image_ctx, m_image_ctx->snap_namespace, m_image_ctx->snap_name,
+    *m_image_ctx, snap_id,
     create_context_callback<klass, &klass::handle_set_snap>(this));
   req->send();
   return nullptr;
index e5502c3cedaaa3f646f1dae9e584302e4339d8b1..f1c2d1cb564db28d09f8200bb95b7417277c25db 100644 (file)
@@ -150,25 +150,11 @@ void RefreshParentRequest<I>::send_set_parent_snap() {
   CephContext *cct = m_child_image_ctx.cct;
   ldout(cct, 10) << this << " " << __func__ << dendl;
 
-  cls::rbd::SnapshotNamespace snap_namespace;
-  std::string snap_name;
-  {
-    RWLock::RLocker snap_locker(m_parent_image_ctx->snap_lock);
-    const SnapInfo *info = m_parent_image_ctx->get_snap_info(m_parent_md.spec.snap_id);
-    if (!info) {
-      lderr(cct) << "failed to locate snapshot: Snapshot with this id not found" << dendl;
-      send_complete(-ENOENT);
-      return;
-    }
-    snap_namespace = info->snap_namespace;
-    snap_name = info->name;
-  }
-
   using klass = RefreshParentRequest<I>;
   Context *ctx = create_context_callback<
     klass, &klass::handle_set_parent_snap, false>(this);
   SetSnapRequest<I> *req = SetSnapRequest<I>::create(
-    *m_parent_image_ctx, snap_namespace, snap_name, ctx);
+    *m_parent_image_ctx, m_parent_md.spec.snap_id, ctx);
   req->send();
 }
 
index ae7776c87c0a6a27ae3a17aa3747b9914a5ec723..cfe22992e784b784c9ff38b8acabb89ff8970812 100644 (file)
@@ -21,13 +21,11 @@ namespace image {
 using util::create_context_callback;
 
 template <typename I>
-SetSnapRequest<I>::SetSnapRequest(I &image_ctx, const cls::rbd::SnapshotNamespace& snap_namespace,
-                                 const std::string &snap_name,
+SetSnapRequest<I>::SetSnapRequest(I &image_ctx, uint64_t snap_id,
                                   Context *on_finish)
-  : m_image_ctx(image_ctx), m_snap_namespace(snap_namespace),
-    m_snap_name(snap_name), m_on_finish(on_finish),
-    m_snap_id(CEPH_NOSNAP), m_exclusive_lock(nullptr), m_object_map(nullptr),
-    m_refresh_parent(nullptr), m_writes_blocked(false) {
+  : m_image_ctx(image_ctx), m_snap_id(snap_id), m_on_finish(on_finish),
+    m_exclusive_lock(nullptr), m_object_map(nullptr), m_refresh_parent(nullptr),
+    m_writes_blocked(false) {
 }
 
 template <typename I>
@@ -40,7 +38,7 @@ SetSnapRequest<I>::~SetSnapRequest() {
 
 template <typename I>
 void SetSnapRequest<I>::send() {
-  if (m_snap_name.empty()) {
+  if (m_snap_id == CEPH_NOSNAP) {
     send_init_exclusive_lock();
   } else {
     send_block_writes();
@@ -123,9 +121,9 @@ Context *SetSnapRequest<I>::handle_block_writes(int *result) {
 
   {
     RWLock::RLocker snap_locker(m_image_ctx.snap_lock);
-    m_snap_id = m_image_ctx.get_snap_id(m_snap_namespace, m_snap_name);
-    if (m_snap_id == CEPH_NOSNAP) {
-      ldout(cct, 5) << "failed to locate snapshot '" << m_snap_name << "'"
+    auto it = m_image_ctx.snap_info.find(m_snap_id);
+    if (it == m_image_ctx.snap_info.end()) {
+      ldout(cct, 5) << "failed to locate snapshot '" << m_snap_id << "'"
                     << dendl;
 
       *result = -ENOENT;
@@ -330,7 +328,7 @@ int SetSnapRequest<I>::apply() {
   RWLock::WLocker parent_locker(m_image_ctx.parent_lock);
   if (m_snap_id != CEPH_NOSNAP) {
     assert(m_image_ctx.exclusive_lock == nullptr);
-    int r = m_image_ctx.snap_set(m_snap_namespace, m_snap_name);
+    int r = m_image_ctx.snap_set(m_snap_id);
     if (r < 0) {
       return r;
     }
index f6f5da06860c7bd31be778260101cd54d198f3b8..c12ea9f27fceb09ae173ca080f284bc21b06db11 100644 (file)
@@ -22,11 +22,9 @@ template <typename> class RefreshParentRequest;
 template <typename ImageCtxT = ImageCtx>
 class SetSnapRequest {
 public:
-  static SetSnapRequest *create(ImageCtxT &image_ctx,
-                                const cls::rbd::SnapshotNamespace& snap_namespace,
-                               const std::string &snap_name,
+  static SetSnapRequest *create(ImageCtxT &image_ctx, uint64_t snap_id,
                                 Context *on_finish) {
-    return new SetSnapRequest(image_ctx, snap_namespace, snap_name, on_finish);
+    return new SetSnapRequest(image_ctx, snap_id, on_finish);
   }
 
   ~SetSnapRequest();
@@ -77,16 +75,12 @@ private:
    * @endverbatim
    */
 
-  SetSnapRequest(ImageCtxT &image_ctx, const cls::rbd::SnapshotNamespace& snap_namespace,
-                const std::string &snap_name,
-                Context *on_finish);
+  SetSnapRequest(ImageCtxT &image_ctx, uint64_t snap_id, Context *on_finish);
 
   ImageCtxT &m_image_ctx;
-  cls::rbd::SnapshotNamespace m_snap_namespace;
-  std::string m_snap_name;
+  uint64_t m_snap_id;
   Context *m_on_finish;
 
-  uint64_t m_snap_id;
   ExclusiveLock<ImageCtxT> *m_exclusive_lock;
   ObjectMap<ImageCtxT> *m_object_map;
   RefreshParentRequest<ImageCtxT> *m_refresh_parent;
index 10ad429145a956fb4c274838ecf70d642e047b77..63af8e74e81dfa2dcb277f1aa737943953d15714 100644 (file)
@@ -1967,7 +1967,8 @@ bool compare_by_name(const child_info_t& c1, const child_info_t& c2)
     return r;
   }
 
-  int snap_set(ImageCtx *ictx, const cls::rbd::SnapshotNamespace &snap_namespace,
+  int snap_set(ImageCtx *ictx,
+               const cls::rbd::SnapshotNamespace &snap_namespace,
               const char *snap_name)
   {
     ldout(ictx->cct, 20) << "snap_set " << ictx << " snap = "
@@ -1977,10 +1978,19 @@ bool compare_by_name(const child_info_t& c1, const child_info_t& c2)
     // snapshot and the user is trying to fix that
     ictx->state->refresh_if_required();
 
-    C_SaferCond ctx;
+    uint64_t snap_id = CEPH_NOSNAP;
     std::string name(snap_name == nullptr ? "" : snap_name);
-    ictx->state->snap_set(snap_namespace, name, &ctx);
+    if (!name.empty()) {
+      RWLock::RLocker snap_locker(ictx->snap_lock);
+      snap_id = ictx->get_snap_id(cls::rbd::UserSnapshotNamespace{},
+                                  snap_name);
+      if (snap_id == CEPH_NOSNAP) {
+        return -ENOENT;
+      }
+    }
 
+    C_SaferCond ctx;
+    ictx->state->snap_set(snap_id, &ctx);
     int r = ctx.wait();
     if (r < 0) {
       if (r != -ENOENT) {
index fc223088f6db5c2d96e36cbd092b7aa975e9bf77..16fc92ab7ca7e4417614a47e02245e910c72a628 100644 (file)
@@ -177,9 +177,12 @@ public:
       ASSERT_EQ(0, image_ctx->operations->snap_protect(
                      cls::rbd::UserSnapshotNamespace{}, "snap"));
 
+      uint64_t snap_id = image_ctx->snap_ids[
+        {cls::rbd::UserSnapshotNamespace{}, "snap"}];
+      ASSERT_NE(CEPH_NOSNAP, snap_id);
+
       C_SaferCond ctx;
-      image_ctx->state->snap_set(cls::rbd::UserSnapshotNamespace{}, "snap",
-                                 &ctx);
+      image_ctx->state->snap_set(snap_id, &ctx);
       ASSERT_EQ(0, ctx.wait());
     }
   }
index 2ec53adafc0f47c9c9d7311022e01d237ce77a5e..a817d333e1d5f5434678e803c14300b365628eac 100644 (file)
@@ -21,7 +21,7 @@ struct MockImageState {
   MOCK_METHOD0(close, int());
   MOCK_METHOD1(close, void(Context*));
 
-  MOCK_METHOD3(snap_set, void(const cls::rbd::SnapshotNamespace &, const std::string &, Context*));
+  MOCK_METHOD2(snap_set, void(uint64_t snap_id, Context*));
 
   MOCK_METHOD1(prepare_lock, void(Context*));
   MOCK_METHOD0(handle_prepare_lock_complete, void());
index f067feb8c7c102bf881f48beca5c7f424872bdce..62c0e43e3856297238e9086feb87f77953f7731f 100644 (file)
@@ -287,10 +287,14 @@ public:
 
   void expect_snap_set(librbd::MockTestImageCtx &mock_image_ctx,
                        const std::string &snap_name, int r) {
-    EXPECT_CALL(*mock_image_ctx.state, snap_set(_, StrEq(snap_name), _))
-      .WillOnce(WithArg<2>(Invoke([this, r](Context *on_finish) {
+    EXPECT_CALL(*mock_image_ctx.state, snap_set(_, _))
+      .WillOnce(Invoke([this, &mock_image_ctx, snap_name, r](uint64_t snap_id, Context *on_finish) {
+          RWLock::RLocker snap_locker(mock_image_ctx.snap_lock);
+          auto id = mock_image_ctx.image_ctx->get_snap_id(
+            cls::rbd::UserSnapshotNamespace{}, snap_name);
+          ASSERT_NE(snap_id, id);
           m_threads->work_queue->queue(on_finish, r);
-        })));
+        }));
   }
 
   void expect_clone_image(MockCloneRequest &mock_clone_request,
index 221a216cc83f3067432ef93958371c68aa87dd9e..5ef2cbe77a3cfacea7bcf4b714405388dc87bfeb 100644 (file)
@@ -284,12 +284,17 @@ TEST_F(TestImageSync, SnapshotStress) {
   read_local_bl.append(std::string(object_size, '1'));
 
   for (auto &snap_name : snap_names) {
+    uint64_t remote_snap_id;
+    {
+      RWLock::RLocker remote_snap_locker(m_remote_image_ctx->snap_lock);
+      remote_snap_id = m_remote_image_ctx->get_snap_id(
+        cls::rbd::UserSnapshotNamespace{}, snap_name);
+    }
+
     uint64_t remote_size;
     {
       C_SaferCond ctx;
-      m_remote_image_ctx->state->snap_set(cls::rbd::UserSnapshotNamespace(),
-                                         snap_name,
-                                         &ctx);
+      m_remote_image_ctx->state->snap_set(remote_snap_id, &ctx);
       ASSERT_EQ(0, ctx.wait());
 
       RWLock::RLocker remote_snap_locker(m_remote_image_ctx->snap_lock);
@@ -297,12 +302,17 @@ TEST_F(TestImageSync, SnapshotStress) {
         m_remote_image_ctx->snap_id);
     }
 
+    uint64_t local_snap_id;
+    {
+      RWLock::RLocker snap_locker(m_local_image_ctx->snap_lock);
+      local_snap_id = m_local_image_ctx->get_snap_id(
+        cls::rbd::UserSnapshotNamespace{}, snap_name);
+    }
+
     uint64_t local_size;
     {
       C_SaferCond ctx;
-      m_local_image_ctx->state->snap_set(cls::rbd::UserSnapshotNamespace(),
-                                        snap_name,
-                                        &ctx);
+      m_local_image_ctx->state->snap_set(local_snap_id, &ctx);
       ASSERT_EQ(0, ctx.wait());
 
       RWLock::RLocker snap_locker(m_local_image_ctx->snap_lock);
index 69c141df60cb777898f1e521f8eab473efb909e7..8414f8ce604ab15d43afce1b574157f426028ef8 100644 (file)
@@ -170,14 +170,6 @@ public:
       .WillOnce(Return(123));
   }
 
-  void expect_snap_set(librbd::MockTestImageCtx &mock_image_ctx,
-                       const std::string &snap_name, int r) {
-    EXPECT_CALL(*mock_image_ctx.state, snap_set(_, StrEq(snap_name), _))
-      .WillOnce(WithArg<2>(Invoke([this, r](Context *on_finish) {
-          m_threads->work_queue->queue(on_finish, r);
-        })));
-  }
-
   void expect_notify_sync_request(MockInstanceWatcher &mock_instance_watcher,
                                   const std::string &sync_id, int r) {
     EXPECT_CALL(mock_instance_watcher, notify_sync_request(sync_id, _))
index bc7b516896a44b1cee48cb01bb1d5ed6152c17ea..351bf1e50ce6fffec94638a84a37cc59d7ba44f4 100644 (file)
@@ -262,29 +262,12 @@ void CreateImageRequest<I>::handle_open_local_parent_image(int r) {
 
 template <typename I>
 void CreateImageRequest<I>::set_local_parent_snap() {
-  dout(20) << dendl;
-
-  {
-    RWLock::RLocker remote_snap_locker(m_remote_parent_image_ctx->snap_lock);
-    auto it = m_remote_parent_image_ctx->snap_info.find(
-      m_remote_parent_spec.snap_id);
-    if (it != m_remote_parent_image_ctx->snap_info.end()) {
-      m_parent_snap_name = it->second.name;
-    }
-  }
-
-  if (m_parent_snap_name.empty()) {
-    m_ret_val = -ENOENT;
-    close_local_parent_image();
-    return;
-  }
-  dout(20) << ": parent_snap_name=" << m_parent_snap_name << dendl;
+  dout(20) << ": parent_snap_id=" << m_remote_parent_spec.snap_id << dendl;
 
   Context *ctx = create_context_callback<
     CreateImageRequest<I>,
     &CreateImageRequest<I>::handle_set_local_parent_snap>(this);
-  m_local_parent_image_ctx->state->snap_set(cls::rbd::UserSnapshotNamespace(),
-                                           m_parent_snap_name,
+  m_local_parent_image_ctx->state->snap_set(m_remote_parent_spec.snap_id,
                                            ctx);
 }
 
@@ -292,7 +275,7 @@ template <typename I>
 void CreateImageRequest<I>::handle_set_local_parent_snap(int r) {
   dout(20) << ": r=" << r << dendl;
   if (r < 0) {
-    derr << ": failed to set parent snapshot " << m_parent_snap_name
+    derr << ": failed to set parent snapshot " << m_remote_parent_spec.snap_id
          << ": " << cpp_strerror(r) << dendl;
     m_ret_val = r;
     close_local_parent_image();
index b45deb66770cd4fd1e22ca0d9615b579fb36678e..595e1992dc2466a84320077763a40cdd63e93c8a 100644 (file)
@@ -101,7 +101,6 @@ private:
   bufferlist m_out_bl;
   std::string m_parent_global_image_id;
   std::string m_parent_pool_name;
-  std::string m_parent_snap_name;
   int m_ret_val = 0;
 
   void create_image();