librbd: simple duplicate op checks for all maintenance operations

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 77699bfe749bc7a898024638fb8347c53fe12123)

Conflicts:
	src/test/librbd/mock/MockOperations.h: no shrink restriction

diff --git a/src/librbd/ImageWatcher.cc b/src/librbd/ImageWatcher.cc
index 3e291ba83f3b31665966bcaa81df0db5e065da5d..0698287f05144c3728cc97ba00bdcc6e5be2b182 100644 (file)
--- 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 82b62932a6b912bbfc7aae82ea5c83235dd87795..6c9d9e72581a041d73dbfee97bf013d92a2f078c 100644 (file)
--- a/src/librbd/Operations.cc
+++ b/src/librbd/Operations.cc
@@ -469,15 +469,24 @@ int Operations<I>::rename(const char *dstname) {
 }
 
 template <typename I>
-void Operations<I>::execute_rename(const char *dstname, Context *on_finish) {
+void Operations<I>::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<I>::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<I> *req = new operation::RenameRequest<I>(
-         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<I> *req = new operation::RenameRequest<I>(
-    m_image_ctx, on_finish, dstname);
+    m_image_ctx, on_finish, dest_name);
   req->send();
 }
 
@@ -619,7 +628,7 @@ void Operations<I>::snap_create(const char *snap_name, Context *on_finish) {
 }
 
 template <typename I>
-void Operations<I>::execute_snap_create(const char *snap_name,
+void Operations<I>::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<I>::snap_rollback(const char *snap_name,
 }
 
 template <typename I>
-void Operations<I>::execute_snap_rollback(const char *snap_name,
+void Operations<I>::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<I>::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<I>::snap_remove(const char *snap_name, Context *on_finish) {
 }
 
 template <typename I>
-void Operations<I>::execute_snap_remove(const char *snap_name,
+void Operations<I>::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<I>::snap_rename(const char *srcname, const char *dstname) {
 
 template <typename I>
 void Operations<I>::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<I>::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<I> *req =
     new operation::SnapshotRenameRequest<I>(
       m_image_ctx, new C_NotifyUpdate<I>(m_image_ctx, on_finish), src_snap_id,
-      dst_name);
+      dest_snap_name);
   req->send();
 }
 
@@ -956,7 +974,7 @@ int Operations<I>::snap_protect(const char *snap_name) {
 }
 
 template <typename I>
-void Operations<I>::execute_snap_protect(const char *snap_name,
+void Operations<I>::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<I>::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<I>::snap_unprotect(const char *snap_name) {
 }
 
 template <typename I>
-void Operations<I>::execute_snap_unprotect(const char *snap_name,
+void Operations<I>::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<I>::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 95af4dcf1e96369c1b4c5f046d368c4c9abb2e1b..411b4ef87ec38d27b09e83193d864ac63a9d0e08 100644 (file)
--- 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 886052416566edb2f2b0a52b99fdcf470c1cddc3..ce647ed60b178c668d32996a63ca8cef9d204d45 100644 (file)
--- 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 8a9c86941d8b24f8776c13551f6e576a6f6ab1c2..0ab7c9dbb3261c18ba99c0860b663cf5f64a21ec 100644 (file)
--- 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 5bbd9b364968e6eccd05bbce4165b33b0f956ac9..478e763eafc0a63a09c35122b8cbe9ad2cb5148a 100644 (file)
--- 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 <string>
 
 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));
 };