From 7918ccc0cf3a2e3cdd50e51cb6222c2604a43bfd Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Mon, 19 Mar 2018 23:13:34 -0400 Subject: [PATCH] librbd: internally utilize snap id when setting snapshot Signed-off-by: Jason Dillaman --- src/librbd/ImageCtx.cc | 12 ++++------ src/librbd/ImageCtx.h | 3 +-- src/librbd/ImageState.cc | 13 ++++------- src/librbd/ImageState.h | 9 +++----- src/librbd/api/Image.cc | 12 +--------- src/librbd/image/OpenRequest.cc | 13 ++++++++++- src/librbd/image/RefreshParentRequest.cc | 16 +------------ src/librbd/image/SetSnapRequest.cc | 20 ++++++++-------- src/librbd/image/SetSnapRequest.h | 14 ++++------- src/librbd/internal.cc | 16 ++++++++++--- .../librbd/image/test_mock_CloneRequest.cc | 7 ++++-- src/test/librbd/mock/MockImageState.h | 2 +- .../test_mock_CreateImageRequest.cc | 10 +++++--- src/test/rbd_mirror/test_ImageSync.cc | 22 +++++++++++++----- src/test/rbd_mirror/test_mock_ImageSync.cc | 8 ------- .../image_replayer/CreateImageRequest.cc | 23 +++---------------- .../image_replayer/CreateImageRequest.h | 1 - 17 files changed, 86 insertions(+), 115 deletions(-) diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc index 7d64c5047f8b9..254f357323096 100644 --- a/src/librbd/ImageCtx.cc +++ b/src/librbd/ImageCtx.cc @@ -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; diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h index 4ef9a2005d301..e0e4ec556aa56 100644 --- a/src/librbd/ImageCtx.h +++ b/src/librbd/ImageCtx.h @@ -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; diff --git a/src/librbd/ImageState.cc b/src/librbd/ImageState.cc index 9ac9f1570a26b..17d364cc94a3c 100644 --- a/src/librbd/ImageState.cc +++ b/src/librbd/ImageState.cc @@ -381,15 +381,12 @@ ImageState::find_pending_refresh() const { } template -void ImageState::snap_set(const cls::rbd::SnapshotNamespace &snap_namespace, - const std::string &snap_name, - Context *on_finish) { +void ImageState::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::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, &ImageState::handle_set_snap>(this)); image::SetSnapRequest *req = image::SetSnapRequest::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(); diff --git a/src/librbd/ImageState.h b/src/librbd/ImageState.h index 2f4d3e33d13c8..d577f290d9202 100644 --- a/src/librbd/ImageState.h +++ b/src/librbd/ImageState.h @@ -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: diff --git a/src/librbd/api/Image.cc b/src/librbd/api/Image.cc index e918893c31c16..4213383ac42ca 100644 --- a/src/librbd/api/Image.cc +++ b/src/librbd/api/Image.cc @@ -240,19 +240,9 @@ int Image::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) { diff --git a/src/librbd/image/OpenRequest.cc b/src/librbd/image/OpenRequest.cc index b5e7415a4c455..26e909f892425 100644 --- a/src/librbd/image/OpenRequest.cc +++ b/src/librbd/image/OpenRequest.cc @@ -508,9 +508,20 @@ Context *OpenRequest::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; SetSnapRequest *req = SetSnapRequest::create( - *m_image_ctx, m_image_ctx->snap_namespace, m_image_ctx->snap_name, + *m_image_ctx, snap_id, create_context_callback(this)); req->send(); return nullptr; diff --git a/src/librbd/image/RefreshParentRequest.cc b/src/librbd/image/RefreshParentRequest.cc index e5502c3cedaaa..f1c2d1cb564db 100644 --- a/src/librbd/image/RefreshParentRequest.cc +++ b/src/librbd/image/RefreshParentRequest.cc @@ -150,25 +150,11 @@ void RefreshParentRequest::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; Context *ctx = create_context_callback< klass, &klass::handle_set_parent_snap, false>(this); SetSnapRequest *req = SetSnapRequest::create( - *m_parent_image_ctx, snap_namespace, snap_name, ctx); + *m_parent_image_ctx, m_parent_md.spec.snap_id, ctx); req->send(); } diff --git a/src/librbd/image/SetSnapRequest.cc b/src/librbd/image/SetSnapRequest.cc index ae7776c87c0a6..cfe22992e784b 100644 --- a/src/librbd/image/SetSnapRequest.cc +++ b/src/librbd/image/SetSnapRequest.cc @@ -21,13 +21,11 @@ namespace image { using util::create_context_callback; template -SetSnapRequest::SetSnapRequest(I &image_ctx, const cls::rbd::SnapshotNamespace& snap_namespace, - const std::string &snap_name, +SetSnapRequest::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 @@ -40,7 +38,7 @@ SetSnapRequest::~SetSnapRequest() { template void SetSnapRequest::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::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::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; } diff --git a/src/librbd/image/SetSnapRequest.h b/src/librbd/image/SetSnapRequest.h index f6f5da06860c7..c12ea9f27fceb 100644 --- a/src/librbd/image/SetSnapRequest.h +++ b/src/librbd/image/SetSnapRequest.h @@ -22,11 +22,9 @@ template class RefreshParentRequest; template 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 *m_exclusive_lock; ObjectMap *m_object_map; RefreshParentRequest *m_refresh_parent; diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc index 10ad429145a95..63af8e74e81df 100644 --- a/src/librbd/internal.cc +++ b/src/librbd/internal.cc @@ -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) { diff --git a/src/test/librbd/image/test_mock_CloneRequest.cc b/src/test/librbd/image/test_mock_CloneRequest.cc index fc223088f6db5..16fc92ab7ca7e 100644 --- a/src/test/librbd/image/test_mock_CloneRequest.cc +++ b/src/test/librbd/image/test_mock_CloneRequest.cc @@ -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()); } } diff --git a/src/test/librbd/mock/MockImageState.h b/src/test/librbd/mock/MockImageState.h index 2ec53adafc0f4..a817d333e1d5f 100644 --- a/src/test/librbd/mock/MockImageState.h +++ b/src/test/librbd/mock/MockImageState.h @@ -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()); diff --git a/src/test/rbd_mirror/image_replayer/test_mock_CreateImageRequest.cc b/src/test/rbd_mirror/image_replayer/test_mock_CreateImageRequest.cc index f067feb8c7c10..62c0e43e38562 100644 --- a/src/test/rbd_mirror/image_replayer/test_mock_CreateImageRequest.cc +++ b/src/test/rbd_mirror/image_replayer/test_mock_CreateImageRequest.cc @@ -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, diff --git a/src/test/rbd_mirror/test_ImageSync.cc b/src/test/rbd_mirror/test_ImageSync.cc index 221a216cc83f3..5ef2cbe77a3cf 100644 --- a/src/test/rbd_mirror/test_ImageSync.cc +++ b/src/test/rbd_mirror/test_ImageSync.cc @@ -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); diff --git a/src/test/rbd_mirror/test_mock_ImageSync.cc b/src/test/rbd_mirror/test_mock_ImageSync.cc index 69c141df60cb7..8414f8ce604ab 100644 --- a/src/test/rbd_mirror/test_mock_ImageSync.cc +++ b/src/test/rbd_mirror/test_mock_ImageSync.cc @@ -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, _)) diff --git a/src/tools/rbd_mirror/image_replayer/CreateImageRequest.cc b/src/tools/rbd_mirror/image_replayer/CreateImageRequest.cc index bc7b516896a44..351bf1e50ce6f 100644 --- a/src/tools/rbd_mirror/image_replayer/CreateImageRequest.cc +++ b/src/tools/rbd_mirror/image_replayer/CreateImageRequest.cc @@ -262,29 +262,12 @@ void CreateImageRequest::handle_open_local_parent_image(int r) { template void CreateImageRequest::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, &CreateImageRequest::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 void CreateImageRequest::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(); diff --git a/src/tools/rbd_mirror/image_replayer/CreateImageRequest.h b/src/tools/rbd_mirror/image_replayer/CreateImageRequest.h index b45deb66770cd..595e1992dc246 100644 --- a/src/tools/rbd_mirror/image_replayer/CreateImageRequest.h +++ b/src/tools/rbd_mirror/image_replayer/CreateImageRequest.h @@ -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(); -- 2.39.5