Merge pull request #51166 from chrisphoffman/wip-rbd-59393

librbd: localize snap_remove op for mirror snapshots

Reviewed-by: Mykola Golub <mgolub@suse.com>
Reviewed-by: Ilya Dryomov <idryomov@gmail.com>

src/cls/rbd/cls_rbd.cc

@@ -5720,24 +5720,6 @@ int image_snapshot_unlink_peer(cls_method_context_t hctx,
     return -ENOENT;
   }
 
-  if (mirror_ns->mirror_peer_uuids.size() == 1) {
-    // if this is the last peer to unlink and we have at least one additional
-    // newer mirror snapshot, return a special error to inform the caller it
-    // should remove the snapshot instead.
-    auto search_lambda = [snap_id](const cls_rbd_snap& snap_meta) {
-      if (snap_meta.id > snap_id &&
-          std::holds_alternative<cls::rbd::MirrorSnapshotNamespace>(
-                   snap_meta.snapshot_namespace)) {
-        return -EEXIST;
-      }
-      return 0;
-    };
-    r = image::snapshot::iterate(hctx, search_lambda);
-    if (r == -EEXIST) {
-      return -ERESTART;
-    }
-  }
-
   mirror_ns->mirror_peer_uuids.erase(mirror_peer_uuid);
 
   r = image::snapshot::write(hctx, snap_key, std::move(snap));

src/librbd/api/Mirror.cc

@@ -1654,7 +1654,7 @@ int Mirror<I>::peer_site_remove(librados::IoCtx& io_ctx,
       for (auto snap_id : snap_ids) {
         C_SaferCond cond;
         auto req = mirror::snapshot::UnlinkPeerRequest<I>::create(
-          img_ctx, snap_id, uuid, &cond);
+          img_ctx, snap_id, uuid, true, &cond);
         req->send();
         r = cond.wait();
         if (r == -ENOENT) {

src/librbd/mirror/snapshot/CreatePrimaryRequest.cc

@@ -239,7 +239,8 @@ void CreatePrimaryRequest<I>::unlink_peer() {
   auto ctx = create_context_callback<
     CreatePrimaryRequest<I>,
     &CreatePrimaryRequest<I>::handle_unlink_peer>(this);
-  auto req = UnlinkPeerRequest<I>::create(m_image_ctx, snap_id, peer_uuid, ctx);
+  auto req = UnlinkPeerRequest<I>::create(m_image_ctx, snap_id, peer_uuid, true,
+                                          ctx);
   req->send();
 }
 

src/librbd/mirror/snapshot/UnlinkPeerRequest.cc

@@ -62,18 +62,19 @@ void UnlinkPeerRequest<I>::unlink_peer() {
 
   m_image_ctx->image_lock.lock_shared();
   int r = -ENOENT;
-  cls::rbd::MirrorSnapshotNamespace* mirror_ns = nullptr;
-  m_newer_mirror_snapshots = false;
+  cls::rbd::SnapshotNamespace snap_namespace;
+  std::string snap_name;
+  bool have_newer_mirror_snapshot = false;
   for (auto snap_it = m_image_ctx->snap_info.find(m_snap_id);
        snap_it != m_image_ctx->snap_info.end(); ++snap_it) {
     if (snap_it->first == m_snap_id) {
       r = 0;
-      mirror_ns = std::get_if<cls::rbd::MirrorSnapshotNamespace>(
-        &snap_it->second.snap_namespace);
+      snap_namespace = snap_it->second.snap_namespace;
+      snap_name = snap_it->second.name;
     } else if (std::holds_alternative<cls::rbd::MirrorSnapshotNamespace>(
                  snap_it->second.snap_namespace)) {
       ldout(cct, 15) << "located newer mirror snapshot" << dendl;
-      m_newer_mirror_snapshots = true;
+      have_newer_mirror_snapshot = true;
       break;
     }
   }
@@ -85,6 +86,8 @@ void UnlinkPeerRequest<I>::unlink_peer() {
     return;
   }
 
+  auto mirror_ns = std::get_if<cls::rbd::MirrorSnapshotNamespace>(
+    &snap_namespace);
   if (mirror_ns == nullptr) {
     lderr(cct) << "not mirror snapshot (snap_id=" << m_snap_id << ")" << dendl;
     m_image_ctx->image_lock.unlock_shared();
@@ -94,12 +97,32 @@ void UnlinkPeerRequest<I>::unlink_peer() {
 
   // if there is or will be no more peers in the mirror snapshot and we have
   // a more recent mirror snapshot, remove the older one
-  if ((mirror_ns->mirror_peer_uuids.count(m_mirror_peer_uuid) == 0) ||
-      (mirror_ns->mirror_peer_uuids.size() <= 1U && m_newer_mirror_snapshots)) {
+  if ((mirror_ns->mirror_peer_uuids.empty() ||
+       (mirror_ns->mirror_peer_uuids.size() == 1 &&
+        mirror_ns->mirror_peer_uuids.count(m_mirror_peer_uuid) != 0)) &&
+      have_newer_mirror_snapshot) {
+    if (m_allow_remove) {
+      m_image_ctx->image_lock.unlock_shared();
+      remove_snapshot(snap_namespace, snap_name);
+      return;
+    } else {
+      ldout(cct, 15) << "skipping removal of snapshot: snap_id=" << m_snap_id
+                     << ", mirror_peer_uuid=" << m_mirror_peer_uuid
+                     << ", mirror_peer_uuids=" << mirror_ns->mirror_peer_uuids
+                     << dendl;
+    }
+  }
+
+  if (mirror_ns->mirror_peer_uuids.count(m_mirror_peer_uuid) == 0) {
+    ldout(cct, 15) << "no peer to unlink: snap_id=" << m_snap_id
+                   << ", mirror_peer_uuid=" << m_mirror_peer_uuid
+                   << ", mirror_peer_uuids=" << mirror_ns->mirror_peer_uuids
+                   << dendl;
     m_image_ctx->image_lock.unlock_shared();
-    remove_snapshot();
+    finish(0);
     return;
   }
+
   m_image_ctx->image_lock.unlock_shared();
 
   ldout(cct, 15) << "snap_id=" << m_snap_id << ", "
@@ -120,6 +143,10 @@ void UnlinkPeerRequest<I>::handle_unlink_peer(int r) {
   ldout(cct, 15) << "r=" << r << dendl;
 
   if (r == -ERESTART || r == -ENOENT) {
+    if (r == -ERESTART) {
+      ldout(cct, 15) << "unlinking last peer not supported" << dendl;
+      m_allow_remove = true;
+    }
     refresh_image();
     return;
   }
@@ -161,44 +188,12 @@ void UnlinkPeerRequest<I>::handle_notify_update(int r) {
 }
 
 template <typename I>
-void UnlinkPeerRequest<I>::remove_snapshot() {
+void UnlinkPeerRequest<I>::remove_snapshot(
+    const cls::rbd::SnapshotNamespace& snap_namespace,
+    const std::string& snap_name) {
   CephContext *cct = m_image_ctx->cct;
   ldout(cct, 15) << dendl;
 
-  cls::rbd::SnapshotNamespace snap_namespace;
-  std::string snap_name;
-  int r = 0;
-  {
-    std::shared_lock image_locker{m_image_ctx->image_lock};
-
-    auto snap_info = m_image_ctx->get_snap_info(m_snap_id);
-    if (!snap_info) {
-      r = -ENOENT;
-    } else {
-      snap_namespace = snap_info->snap_namespace;
-      snap_name = snap_info->name;
-    }
-  }
-
-  if (r == -ENOENT) {
-    ldout(cct, 15) << "failed to locate snapshot " << m_snap_id << dendl;
-    finish(0);
-    return;
-  }
-
-  auto info = std::get<cls::rbd::MirrorSnapshotNamespace>(
-    snap_namespace);
-
-  info.mirror_peer_uuids.erase(m_mirror_peer_uuid);
-  if (!info.mirror_peer_uuids.empty() || !m_newer_mirror_snapshots) {
-    ldout(cct, 15) << "skipping removal of snapshot: "
-                   << "snap_id=" << m_snap_id << ": "
-                   << "mirror_peer_uuid=" << m_mirror_peer_uuid << ", "
-                   << "mirror_peer_uuids=" << info.mirror_peer_uuids << dendl;
-    finish(0);
-    return;
-  }
-
   auto ctx = create_context_callback<
     UnlinkPeerRequest<I>, &UnlinkPeerRequest<I>::handle_remove_snapshot>(this);
   m_image_ctx->operations->snap_remove(snap_namespace, snap_name, ctx);

src/librbd/mirror/snapshot/UnlinkPeerRequest.h

@@ -5,6 +5,7 @@
 #define CEPH_LIBRBD_MIRROR_SNAPSHOT_UNLINK_PEER_REQUEST_H
 
 #include "include/buffer.h"
+#include "cls/rbd/cls_rbd_client.h"
 
 #include <string>
 #include <set>
@@ -23,15 +24,17 @@ class UnlinkPeerRequest {
 public:
   static UnlinkPeerRequest *create(ImageCtxT *image_ctx, uint64_t snap_id,
                                    const std::string &mirror_peer_uuid,
-                                   Context *on_finish) {
+                                   bool allow_remove, Context *on_finish) {
     return new UnlinkPeerRequest(image_ctx, snap_id, mirror_peer_uuid,
-                                 on_finish);
+                                 allow_remove, on_finish);
   }
 
   UnlinkPeerRequest(ImageCtxT *image_ctx, uint64_t snap_id,
-                    const std::string &mirror_peer_uuid, Context *on_finish)
+                    const std::string &mirror_peer_uuid, bool allow_remove,
+                    Context *on_finish)
     : m_image_ctx(image_ctx), m_snap_id(snap_id),
-      m_mirror_peer_uuid(mirror_peer_uuid), m_on_finish(on_finish) {
+      m_mirror_peer_uuid(mirror_peer_uuid), m_allow_remove(allow_remove),
+      m_on_finish(on_finish) {
   }
 
   void send();
@@ -67,10 +70,9 @@ class UnlinkPeerRequest {
   ImageCtxT *m_image_ctx;
   uint64_t m_snap_id;
   std::string m_mirror_peer_uuid;
+  bool m_allow_remove;
   Context *m_on_finish;
 
-  bool m_newer_mirror_snapshots = false;
-
   void refresh_image();
   void handle_refresh_image(int r);
 
@@ -80,7 +82,8 @@ class UnlinkPeerRequest {
   void notify_update();
   void handle_notify_update(int r);
 
-  void remove_snapshot();
+  void remove_snapshot(const cls::rbd::SnapshotNamespace& snap_namespace,
+                       const std::string& snap_name);
   void handle_remove_snapshot(int r);
 
   void finish(int r);

src/test/cls_rbd/test_cls_rbd.cc

@@ -2277,14 +2277,14 @@ TEST_F(TestClsRbd, mirror_snapshot) {
   ASSERT_EQ(1U, sn->mirror_peer_uuids.size());
   ASSERT_EQ(1U, sn->mirror_peer_uuids.count("peer2"));
 
-  ASSERT_EQ(-ERESTART,
-            mirror_image_snapshot_unlink_peer(&ioctx, oid, 1, "peer2"));
+  ASSERT_EQ(0, mirror_image_snapshot_unlink_peer(&ioctx, oid, 1, "peer2"));
+  ASSERT_EQ(-ENOENT, mirror_image_snapshot_unlink_peer(&ioctx, oid, 1,
+                                                       "peer2"));
   ASSERT_EQ(0, snapshot_get(&ioctx, oid, 1, &snap));
   sn = std::get_if<cls::rbd::MirrorSnapshotNamespace>(
     &snap.snapshot_namespace);
   ASSERT_NE(nullptr, sn);
-  ASSERT_EQ(1U, sn->mirror_peer_uuids.size());
-  ASSERT_EQ(1U, sn->mirror_peer_uuids.count("peer2"));
+  ASSERT_EQ(0U, sn->mirror_peer_uuids.size());
 
   ASSERT_EQ(0, snapshot_get(&ioctx, oid, 2, &snap));
   auto nsn = std::get_if<cls::rbd::MirrorSnapshotNamespace>(

src/test/librbd/mirror/snapshot/test_mock_CreatePrimaryRequest.cc

@@ -58,15 +58,18 @@ template <>
 struct UnlinkPeerRequest<MockTestImageCtx> {
   uint64_t snap_id = CEPH_NOSNAP;
   std::string mirror_peer_uuid;
+  bool allow_remove;
   Context* on_finish = nullptr;
   static UnlinkPeerRequest* s_instance;
   static UnlinkPeerRequest *create(MockTestImageCtx *image_ctx,
                                    uint64_t snap_id,
                                    const std::string &mirror_peer_uuid,
+                                   bool allow_remove,
                                    Context *on_finish) {
     ceph_assert(s_instance != nullptr);
     s_instance->snap_id = snap_id;
     s_instance->mirror_peer_uuid = mirror_peer_uuid;
+    s_instance->allow_remove = allow_remove;
     s_instance->on_finish = on_finish;
     return s_instance;
   }
@@ -175,13 +178,15 @@ class TestMockMirrorSnapshotCreatePrimaryRequest : public TestMockFixture {
   void expect_unlink_peer(MockTestImageCtx &mock_image_ctx,
                           MockUnlinkPeerRequest &mock_unlink_peer_request,
                           uint64_t snap_id, const std::string &peer_uuid,
-                          bool is_linked, bool complete, int r) {
+                          bool is_linked, bool complete, bool allow_remove,
+                          int r) {
     EXPECT_CALL(mock_unlink_peer_request, send())
       .WillOnce(Invoke([&mock_image_ctx, &mock_unlink_peer_request,
-                        snap_id, peer_uuid, is_linked, complete, r]() {
+                        snap_id, peer_uuid, is_linked, complete, allow_remove, r]() {
                          ASSERT_EQ(mock_unlink_peer_request.mirror_peer_uuid,
                                    peer_uuid);
                          ASSERT_EQ(mock_unlink_peer_request.snap_id, snap_id);
+                         ASSERT_EQ(mock_unlink_peer_request.allow_remove, allow_remove);
                          if (r == 0) {
                            auto it = mock_image_ctx.snap_info.find(snap_id);
                            ASSERT_NE(it, mock_image_ctx.snap_info.end());
@@ -325,7 +330,7 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkIncomplete) {
   auto it = mock_image_ctx.snap_info.rbegin();
   auto snap_id = it->first;
   expect_unlink_peer(mock_image_ctx, mock_unlink_peer_request, snap_id, "uuid",
-                     true, false, 0);
+                     true, false, true, 0);
   C_SaferCond ctx;
   auto req = new MockCreatePrimaryRequest(&mock_image_ctx, "gid", CEPH_NOSNAP,
                                           0U, 0U, nullptr, &ctx);
@@ -362,7 +367,7 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkPeer) {
   auto it = mock_image_ctx.snap_info.rbegin();
   auto snap_id = it->first;
   expect_unlink_peer(mock_image_ctx, mock_unlink_peer_request, snap_id, "uuid",
-                     true, true, 0);
+                     true, true, true, 0);
   C_SaferCond ctx;
   auto req = new MockCreatePrimaryRequest(&mock_image_ctx, "gid", CEPH_NOSNAP,
                                           0U, 0U, nullptr, &ctx);
@@ -397,7 +402,7 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkNoPeer) {
   auto it = mock_image_ctx.snap_info.rbegin();
   auto snap_id = it->first;
   expect_unlink_peer(mock_image_ctx, mock_unlink_peer_request, snap_id, "uuid",
-                     false, true, 0);
+                     false, true, true, 0);
 
   C_SaferCond ctx;
   auto req = new MockCreatePrimaryRequest(&mock_image_ctx, "gid", CEPH_NOSNAP,
@@ -438,9 +443,9 @@ TEST_F(TestMockMirrorSnapshotCreatePrimaryRequest, SuccessUnlinkMultiplePeers) {
   auto it = mock_image_ctx.snap_info.rbegin();
   auto snap_id = it->first;
   expect_unlink_peer(mock_image_ctx, mock_unlink_peer_request, snap_id, "uuid1",
-                     true, true, 0);
+                     true, true, true, 0);
   expect_unlink_peer(mock_image_ctx, mock_unlink_peer_request, snap_id, "uuid2",
-                     true, true, 0);
+                     true, true, true, 0);
   C_SaferCond ctx;
   auto req = new MockCreatePrimaryRequest(&mock_image_ctx, "gid", CEPH_NOSNAP,
                                           0U, 0U, nullptr, &ctx);