From 1a25490367343d7d4083961163c62f1c32cac105 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Fri, 8 Jul 2016 09:13:07 -0400 Subject: [PATCH] librbd: simple duplicate op checks for all maintenance operations Signed-off-by: Jason Dillaman (cherry picked from commit 77699bfe749bc7a898024638fb8347c53fe12123) Conflicts: src/test/librbd/mock/MockOperations.h: no shrink restriction --- src/librbd/ImageWatcher.cc | 12 ++-- src/librbd/Operations.cc | 74 +++++++++++++++++---- src/librbd/Operations.h | 17 ++--- src/librbd/journal/Replay.cc | 14 ++-- src/test/librbd/journal/test_mock_Replay.cc | 15 +++-- src/test/librbd/mock/MockOperations.h | 22 +++--- 6 files changed, 104 insertions(+), 50 deletions(-) diff --git a/src/librbd/ImageWatcher.cc b/src/librbd/ImageWatcher.cc index 3e291ba83f3b3..0698287f05144 100644 --- a/src/librbd/ImageWatcher.cc +++ b/src/librbd/ImageWatcher.cc @@ -737,7 +737,7 @@ bool ImageWatcher::handle_payload(const SnapCreatePayload &payload, ldout(m_image_ctx.cct, 10) << this << " remote snap_create request: " << payload.snap_name << dendl; - m_image_ctx.operations->execute_snap_create(payload.snap_name.c_str(), + m_image_ctx.operations->execute_snap_create(payload.snap_name, new C_ResponseMessage(ack_ctx), 0, false); return false; @@ -759,7 +759,7 @@ bool ImageWatcher::handle_payload(const SnapRenamePayload &payload, << payload.snap_name << dendl; m_image_ctx.operations->execute_snap_rename(payload.snap_id, - payload.snap_name.c_str(), + payload.snap_name, new C_ResponseMessage(ack_ctx)); return false; } else if (r < 0) { @@ -778,7 +778,7 @@ bool ImageWatcher::handle_payload(const SnapRemovePayload &payload, ldout(m_image_ctx.cct, 10) << this << " remote snap_remove request: " << payload.snap_name << dendl; - m_image_ctx.operations->execute_snap_remove(payload.snap_name.c_str(), + m_image_ctx.operations->execute_snap_remove(payload.snap_name, new C_ResponseMessage(ack_ctx)); return false; } else if (r < 0) { @@ -797,7 +797,7 @@ bool ImageWatcher::handle_payload(const SnapProtectPayload& payload, ldout(m_image_ctx.cct, 10) << this << " remote snap_protect request: " << payload.snap_name << dendl; - m_image_ctx.operations->execute_snap_protect(payload.snap_name.c_str(), + m_image_ctx.operations->execute_snap_protect(payload.snap_name, new C_ResponseMessage(ack_ctx)); return false; } else if (r < 0) { @@ -816,7 +816,7 @@ bool ImageWatcher::handle_payload(const SnapUnprotectPayload& payload, ldout(m_image_ctx.cct, 10) << this << " remote snap_unprotect request: " << payload.snap_name << dendl; - m_image_ctx.operations->execute_snap_unprotect(payload.snap_name.c_str(), + m_image_ctx.operations->execute_snap_unprotect(payload.snap_name, new C_ResponseMessage(ack_ctx)); return false; } else if (r < 0) { @@ -861,7 +861,7 @@ bool ImageWatcher::handle_payload(const RenamePayload& payload, ldout(m_image_ctx.cct, 10) << this << " remote rename request: " << payload.image_name << dendl; - m_image_ctx.operations->execute_rename(payload.image_name.c_str(), + m_image_ctx.operations->execute_rename(payload.image_name, new C_ResponseMessage(ack_ctx)); return false; } else if (r < 0) { diff --git a/src/librbd/Operations.cc b/src/librbd/Operations.cc index 82b62932a6b91..6c9d9e72581a0 100644 --- a/src/librbd/Operations.cc +++ b/src/librbd/Operations.cc @@ -469,15 +469,24 @@ int Operations::rename(const char *dstname) { } template -void Operations::execute_rename(const char *dstname, Context *on_finish) { +void Operations::execute_rename(const std::string &dest_name, + Context *on_finish) { assert(m_image_ctx.owner_lock.is_locked()); if (m_image_ctx.test_features(RBD_FEATURE_JOURNALING)) { assert(m_image_ctx.exclusive_lock == nullptr || m_image_ctx.exclusive_lock->is_lock_owner()); } + m_image_ctx.snap_lock.get_read(); + if (m_image_ctx.name == dest_name) { + m_image_ctx.snap_lock.put_read(); + on_finish->complete(-EEXIST); + return; + } + m_image_ctx.snap_lock.put_read(); + CephContext *cct = m_image_ctx.cct; - ldout(cct, 5) << this << " " << __func__ << ": dest_name=" << dstname + ldout(cct, 5) << this << " " << __func__ << ": dest_name=" << dest_name << dendl; if (m_image_ctx.old_format) { @@ -486,16 +495,16 @@ void Operations::execute_rename(const char *dstname, Context *on_finish) { on_finish = new FunctionContext([this, on_finish](int r) { m_image_ctx.image_watcher->register_watch(on_finish); }); - on_finish = new FunctionContext([this, dstname, on_finish](int r) { + on_finish = new FunctionContext([this, dest_name, on_finish](int r) { operation::RenameRequest *req = new operation::RenameRequest( - m_image_ctx, on_finish, dstname); + m_image_ctx, on_finish, dest_name); req->send(); }); m_image_ctx.image_watcher->unregister_watch(on_finish); return; } operation::RenameRequest *req = new operation::RenameRequest( - m_image_ctx, on_finish, dstname); + m_image_ctx, on_finish, dest_name); req->send(); } @@ -619,7 +628,7 @@ void Operations::snap_create(const char *snap_name, Context *on_finish) { } template -void Operations::execute_snap_create(const char *snap_name, +void Operations::execute_snap_create(const std::string &snap_name, Context *on_finish, uint64_t journal_op_tid, bool skip_object_map) { @@ -697,7 +706,7 @@ int Operations::snap_rollback(const char *snap_name, } template -void Operations::execute_snap_rollback(const char *snap_name, +void Operations::execute_snap_rollback(const std::string &snap_name, ProgressContext& prog_ctx, Context *on_finish) { assert(m_image_ctx.owner_lock.is_locked()); @@ -759,6 +768,7 @@ void Operations::snap_remove(const char *snap_name, Context *on_finish) { return; } + // quickly filter out duplicate ops m_image_ctx.snap_lock.get_read(); if (m_image_ctx.get_snap_id(snap_name) == CEPH_NOSNAP) { m_image_ctx.snap_lock.put_read(); @@ -785,7 +795,7 @@ void Operations::snap_remove(const char *snap_name, Context *on_finish) { } template -void Operations::execute_snap_remove(const char *snap_name, +void Operations::execute_snap_remove(const std::string &snap_name, Context *on_finish) { assert(m_image_ctx.owner_lock.is_locked()); { @@ -883,7 +893,7 @@ int Operations::snap_rename(const char *srcname, const char *dstname) { template void Operations::execute_snap_rename(const uint64_t src_snap_id, - const char *dst_name, + const std::string &dest_snap_name, Context *on_finish) { assert(m_image_ctx.owner_lock.is_locked()); if ((m_image_ctx.features & RBD_FEATURE_JOURNALING) != 0) { @@ -891,15 +901,23 @@ void Operations::execute_snap_rename(const uint64_t src_snap_id, m_image_ctx.exclusive_lock->is_lock_owner()); } + m_image_ctx.snap_lock.get_read(); + if (m_image_ctx.get_snap_id(dest_snap_name) != CEPH_NOSNAP) { + m_image_ctx.snap_lock.put_read(); + on_finish->complete(-EEXIST); + return; + } + m_image_ctx.snap_lock.put_read(); + CephContext *cct = m_image_ctx.cct; ldout(cct, 5) << this << " " << __func__ << ": " << "snap_id=" << src_snap_id << ", " - << "new_snap_name=" << dst_name << dendl; + << "new_snap_name=" << dest_snap_name << dendl; operation::SnapshotRenameRequest *req = new operation::SnapshotRenameRequest( m_image_ctx, new C_NotifyUpdate(m_image_ctx, on_finish), src_snap_id, - dst_name); + dest_snap_name); req->send(); } @@ -956,7 +974,7 @@ int Operations::snap_protect(const char *snap_name) { } template -void Operations::execute_snap_protect(const char *snap_name, +void Operations::execute_snap_protect(const std::string &snap_name, Context *on_finish) { assert(m_image_ctx.owner_lock.is_locked()); if (m_image_ctx.test_features(RBD_FEATURE_JOURNALING)) { @@ -964,6 +982,21 @@ void Operations::execute_snap_protect(const char *snap_name, m_image_ctx.exclusive_lock->is_lock_owner()); } + m_image_ctx.snap_lock.get_read(); + bool is_protected; + int r = m_image_ctx.is_snap_protected(m_image_ctx.get_snap_id(snap_name), + &is_protected); + if (r < 0) { + m_image_ctx.snap_lock.put_read(); + on_finish->complete(r); + return; + } else if (is_protected) { + m_image_ctx.snap_lock.put_read(); + on_finish->complete(-EBUSY); + return; + } + m_image_ctx.snap_lock.put_read(); + CephContext *cct = m_image_ctx.cct; ldout(cct, 5) << this << " " << __func__ << ": snap_name=" << snap_name << dendl; @@ -1027,7 +1060,7 @@ int Operations::snap_unprotect(const char *snap_name) { } template -void Operations::execute_snap_unprotect(const char *snap_name, +void Operations::execute_snap_unprotect(const std::string &snap_name, Context *on_finish) { assert(m_image_ctx.owner_lock.is_locked()); if (m_image_ctx.test_features(RBD_FEATURE_JOURNALING)) { @@ -1035,6 +1068,21 @@ void Operations::execute_snap_unprotect(const char *snap_name, m_image_ctx.exclusive_lock->is_lock_owner()); } + m_image_ctx.snap_lock.get_read(); + bool is_unprotected; + int r = m_image_ctx.is_snap_unprotected(m_image_ctx.get_snap_id(snap_name), + &is_unprotected); + if (r < 0) { + m_image_ctx.snap_lock.put_read(); + on_finish->complete(r); + return; + } else if (is_unprotected) { + m_image_ctx.snap_lock.put_read(); + on_finish->complete(-EINVAL); + return; + } + m_image_ctx.snap_lock.put_read(); + CephContext *cct = m_image_ctx.cct; ldout(cct, 5) << this << " " << __func__ << ": snap_name=" << snap_name << dendl; diff --git a/src/librbd/Operations.h b/src/librbd/Operations.h index 95af4dcf1e963..411b4ef87ec38 100644 --- a/src/librbd/Operations.h +++ b/src/librbd/Operations.h @@ -29,7 +29,7 @@ public: Context *on_finish); int rename(const char *dstname); - void execute_rename(const char *dstname, Context *on_finish); + void execute_rename(const std::string &dest_name, Context *on_finish); int resize(uint64_t size, ProgressContext& prog_ctx); void execute_resize(uint64_t size, ProgressContext &prog_ctx, @@ -37,26 +37,27 @@ public: int snap_create(const char *snap_name); void snap_create(const char *snap_name, Context *on_finish); - void execute_snap_create(const char *snap_name, Context *on_finish, + void execute_snap_create(const std::string &snap_name, Context *on_finish, uint64_t journal_op_tid, bool skip_object_map); int snap_rollback(const char *snap_name, ProgressContext& prog_ctx); - void execute_snap_rollback(const char *snap_name, ProgressContext& prog_ctx, - Context *on_finish); + void execute_snap_rollback(const std::string &snap_name, + ProgressContext& prog_ctx, Context *on_finish); int snap_remove(const char *snap_name); void snap_remove(const char *snap_name, Context *on_finish); - void execute_snap_remove(const char *snap_name, Context *on_finish); + void execute_snap_remove(const std::string &snap_name, Context *on_finish); int snap_rename(const char *srcname, const char *dstname); - void execute_snap_rename(const uint64_t src_snap_id, const char *dst_name, + void execute_snap_rename(const uint64_t src_snap_id, + const std::string &dest_snap_name, Context *on_finish); int snap_protect(const char *snap_name); - void execute_snap_protect(const char *snap_name, Context *on_finish); + void execute_snap_protect(const std::string &snap_name, Context *on_finish); int snap_unprotect(const char *snap_name); - void execute_snap_unprotect(const char *snap_name, Context *on_finish); + void execute_snap_unprotect(const std::string &snap_name, Context *on_finish); int prepare_image_update(); diff --git a/src/librbd/journal/Replay.cc b/src/librbd/journal/Replay.cc index 886052416566e..ce647ed60b178 100644 --- a/src/librbd/journal/Replay.cc +++ b/src/librbd/journal/Replay.cc @@ -39,40 +39,40 @@ struct ExecuteOp : public Context { } void execute(const journal::SnapCreateEvent &_) { - image_ctx.operations->execute_snap_create(event.snap_name.c_str(), + image_ctx.operations->execute_snap_create(event.snap_name, on_op_complete, event.op_tid, false); } void execute(const journal::SnapRemoveEvent &_) { - image_ctx.operations->execute_snap_remove(event.snap_name.c_str(), + image_ctx.operations->execute_snap_remove(event.snap_name, on_op_complete); } void execute(const journal::SnapRenameEvent &_) { image_ctx.operations->execute_snap_rename(event.snap_id, - event.snap_name.c_str(), + event.snap_name, on_op_complete); } void execute(const journal::SnapProtectEvent &_) { - image_ctx.operations->execute_snap_protect(event.snap_name.c_str(), + image_ctx.operations->execute_snap_protect(event.snap_name, on_op_complete); } void execute(const journal::SnapUnprotectEvent &_) { - image_ctx.operations->execute_snap_unprotect(event.snap_name.c_str(), + image_ctx.operations->execute_snap_unprotect(event.snap_name, on_op_complete); } void execute(const journal::SnapRollbackEvent &_) { - image_ctx.operations->execute_snap_rollback(event.snap_name.c_str(), + image_ctx.operations->execute_snap_rollback(event.snap_name, no_op_progress_callback, on_op_complete); } void execute(const journal::RenameEvent &_) { - image_ctx.operations->execute_rename(event.image_name.c_str(), + image_ctx.operations->execute_rename(event.image_name, on_op_complete); } diff --git a/src/test/librbd/journal/test_mock_Replay.cc b/src/test/librbd/journal/test_mock_Replay.cc index 8a9c86941d8b2..0ab7c9dbb3261 100644 --- a/src/test/librbd/journal/test_mock_Replay.cc +++ b/src/test/librbd/journal/test_mock_Replay.cc @@ -65,6 +65,7 @@ using ::testing::DoAll; using ::testing::InSequence; using ::testing::Return; using ::testing::SaveArg; +using ::testing::StrEq; using ::testing::WithArgs; MATCHER_P(CStrEq, str, "") { @@ -131,7 +132,7 @@ public: void expect_rename(MockReplayImageCtx &mock_image_ctx, Context **on_finish, const char *image_name) { - EXPECT_CALL(*mock_image_ctx.operations, execute_rename(CStrEq(image_name), _)) + EXPECT_CALL(*mock_image_ctx.operations, execute_rename(StrEq(image_name), _)) .WillOnce(DoAll(SaveArg<1>(on_finish), NotifyInvoke(&m_invoke_lock, &m_invoke_cond))); } @@ -146,7 +147,7 @@ public: void expect_snap_create(MockReplayImageCtx &mock_image_ctx, Context **on_finish, const char *snap_name, uint64_t op_tid) { - EXPECT_CALL(*mock_image_ctx.operations, execute_snap_create(CStrEq(snap_name), _, + EXPECT_CALL(*mock_image_ctx.operations, execute_snap_create(StrEq(snap_name), _, op_tid, false)) .WillOnce(DoAll(SaveArg<1>(on_finish), NotifyInvoke(&m_invoke_lock, &m_invoke_cond))); @@ -154,7 +155,7 @@ public: void expect_snap_remove(MockReplayImageCtx &mock_image_ctx, Context **on_finish, const char *snap_name) { - EXPECT_CALL(*mock_image_ctx.operations, execute_snap_remove(CStrEq(snap_name), _)) + EXPECT_CALL(*mock_image_ctx.operations, execute_snap_remove(StrEq(snap_name), _)) .WillOnce(DoAll(SaveArg<1>(on_finish), NotifyInvoke(&m_invoke_lock, &m_invoke_cond))); } @@ -162,28 +163,28 @@ public: void expect_snap_rename(MockReplayImageCtx &mock_image_ctx, Context **on_finish, uint64_t snap_id, const char *snap_name) { - EXPECT_CALL(*mock_image_ctx.operations, execute_snap_rename(snap_id, CStrEq(snap_name), _)) + EXPECT_CALL(*mock_image_ctx.operations, execute_snap_rename(snap_id, StrEq(snap_name), _)) .WillOnce(DoAll(SaveArg<2>(on_finish), NotifyInvoke(&m_invoke_lock, &m_invoke_cond))); } void expect_snap_protect(MockReplayImageCtx &mock_image_ctx, Context **on_finish, const char *snap_name) { - EXPECT_CALL(*mock_image_ctx.operations, execute_snap_protect(CStrEq(snap_name), _)) + EXPECT_CALL(*mock_image_ctx.operations, execute_snap_protect(StrEq(snap_name), _)) .WillOnce(DoAll(SaveArg<1>(on_finish), NotifyInvoke(&m_invoke_lock, &m_invoke_cond))); } void expect_snap_unprotect(MockReplayImageCtx &mock_image_ctx, Context **on_finish, const char *snap_name) { - EXPECT_CALL(*mock_image_ctx.operations, execute_snap_unprotect(CStrEq(snap_name), _)) + EXPECT_CALL(*mock_image_ctx.operations, execute_snap_unprotect(StrEq(snap_name), _)) .WillOnce(DoAll(SaveArg<1>(on_finish), NotifyInvoke(&m_invoke_lock, &m_invoke_cond))); } void expect_snap_rollback(MockReplayImageCtx &mock_image_ctx, Context **on_finish, const char *snap_name) { - EXPECT_CALL(*mock_image_ctx.operations, execute_snap_rollback(CStrEq(snap_name), _, _)) + EXPECT_CALL(*mock_image_ctx.operations, execute_snap_rollback(StrEq(snap_name), _, _)) .WillOnce(DoAll(SaveArg<2>(on_finish), NotifyInvoke(&m_invoke_lock, &m_invoke_cond))); } diff --git a/src/test/librbd/mock/MockOperations.h b/src/test/librbd/mock/MockOperations.h index 5bbd9b364968e..478e763eafc0a 100644 --- a/src/test/librbd/mock/MockOperations.h +++ b/src/test/librbd/mock/MockOperations.h @@ -7,6 +7,7 @@ #include "include/int_types.h" #include "include/rbd/librbd.hpp" #include "gmock/gmock.h" +#include class Context; @@ -17,27 +18,30 @@ struct MockOperations { Context *on_finish)); MOCK_METHOD2(execute_rebuild_object_map, void(ProgressContext &prog_ctx, Context *on_finish)); - MOCK_METHOD2(execute_rename, void(const char *dstname, Context *on_finish)); + MOCK_METHOD2(execute_rename, void(const std::string &dstname, + Context *on_finish)); MOCK_METHOD4(execute_resize, void(uint64_t size, ProgressContext &prog_ctx, Context *on_finish, uint64_t journal_op_tid)); - MOCK_METHOD2(snap_create, void(const char *snap_name, Context *on_finish)); - MOCK_METHOD4(execute_snap_create, void(const char *snap_name, + MOCK_METHOD2(snap_create, void(const std::string &snap_name, + Context *on_finish)); + MOCK_METHOD4(execute_snap_create, void(const std::string &snap_name, Context *on_finish, uint64_t journal_op_tid, bool skip_object_map)); - MOCK_METHOD2(snap_remove, void(const char *snap_name, Context *on_finish)); - MOCK_METHOD2(execute_snap_remove, void(const char *snap_name, + MOCK_METHOD2(snap_remove, void(const std::string &snap_name, + Context *on_finish)); + MOCK_METHOD2(execute_snap_remove, void(const std::string &snap_name, Context *on_finish)); MOCK_METHOD3(execute_snap_rename, void(uint64_t src_snap_id, - const char *snap_name, + const std::string &snap_name, Context *on_finish)); - MOCK_METHOD3(execute_snap_rollback, void(const char *snap_name, + MOCK_METHOD3(execute_snap_rollback, void(const std::string &snap_name, ProgressContext &prog_ctx, Context *on_finish)); - MOCK_METHOD2(execute_snap_protect, void(const char *snap_name, + MOCK_METHOD2(execute_snap_protect, void(const std::string &snap_name, Context *on_finish)); - MOCK_METHOD2(execute_snap_unprotect, void(const char *snap_name, + MOCK_METHOD2(execute_snap_unprotect, void(const std::string &snap_name, Context *on_finish)); }; -- 2.39.5