From: Ilya Dryomov <idryomov@gmail.com>
Date: Tue, 22 Aug 2023 15:27:50 +0000 (+0200)
Subject: librbd: don't attempt to remove image state on orphan snapshots
X-Git-Tag: v19.0.0~567^2~2
X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=cfae3f79bd0513e2753b0deb8c2624ab07cf2d1b;p=ceph.git

librbd: don't attempt to remove image state on orphan snapshots

Despite being mirror snapshots, orphan snapshots don't have image
state: see CreateNonPrimaryRequest::write_image_state() for a similar
is_orphan() check.  Attempting to remove image state generates bogus
"failed to read image state object" and "failed to remove image state"
errors.

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---

diff --git a/src/librbd/operation/SnapshotRemoveRequest.cc b/src/librbd/operation/SnapshotRemoveRequest.cc
index cc975d176350c..f3b4dc62e046b 100644
--- a/src/librbd/operation/SnapshotRemoveRequest.cc
+++ b/src/librbd/operation/SnapshotRemoveRequest.cc
@@ -355,9 +355,10 @@ void SnapshotRemoveRequest<I>::handle_remove_object_map(int r) {
 template <typename I>
 void SnapshotRemoveRequest<I>::remove_image_state() {
   I &image_ctx = this->m_image_ctx;
-  auto type = cls::rbd::get_snap_namespace_type(m_snap_namespace);
 
-  if (type != cls::rbd::SNAPSHOT_NAMESPACE_TYPE_MIRROR) {
+  const auto* info = std::get_if<cls::rbd::MirrorSnapshotNamespace>(
+    &m_snap_namespace);
+  if (info == nullptr || info->is_orphan()) {
     release_snap_id();
     return;
   }
diff --git a/src/test/librbd/operation/test_mock_SnapshotRemoveRequest.cc b/src/test/librbd/operation/test_mock_SnapshotRemoveRequest.cc
index 1189f76fb4696..4469cb80ddec9 100644
--- a/src/test/librbd/operation/test_mock_SnapshotRemoveRequest.cc
+++ b/src/test/librbd/operation/test_mock_SnapshotRemoveRequest.cc
@@ -512,9 +512,10 @@ TEST_F(TestMockOperationSnapshotRemoveRequest, MirrorSnapshot) {
   expect_snapshot_trash_add(mock_image_ctx, 0);
 
   uint64_t snap_id = ictx->snap_info.rbegin()->first;
+  cls::rbd::MirrorSnapshotNamespace ns{
+    cls::rbd::MIRROR_SNAPSHOT_STATE_NON_PRIMARY, {}, "mirror uuid", 123};
   expect_snapshot_get(mock_image_ctx,
-                      {snap_id, {cls::rbd::MirrorSnapshotNamespace{}},
-                       "mirror", 123, {}, 0}, 0);
+                      {snap_id, {ns}, "mirror", 456, {}, 0}, 0);
 
   expect_get_parent_spec(mock_image_ctx, 0);
   expect_object_map_snap_remove(mock_image_ctx, 0);
@@ -526,8 +527,55 @@ TEST_F(TestMockOperationSnapshotRemoveRequest, MirrorSnapshot) {
 
   C_SaferCond cond_ctx;
   MockSnapshotRemoveRequest *req = new MockSnapshotRemoveRequest(
-    mock_image_ctx, &cond_ctx, cls::rbd::MirrorSnapshotNamespace(),
-    "mirror", snap_id);
+    mock_image_ctx, &cond_ctx, ns, "mirror", snap_id);
+  {
+    std::shared_lock owner_locker{mock_image_ctx.owner_lock};
+    req->send();
+  }
+  ASSERT_EQ(0, cond_ctx.wait());
+}
+
+TEST_F(TestMockOperationSnapshotRemoveRequest, MirrorSnapshotOrphan) {
+  REQUIRE_FORMAT_V2();
+
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(m_image_name, &ictx));
+  ASSERT_EQ(0, snap_create(*ictx, "snap1"));
+  ASSERT_EQ(0, ictx->state->refresh_if_required());
+
+  MockImageCtx mock_image_ctx(*ictx);
+
+  MockExclusiveLock mock_exclusive_lock;
+  if (ictx->test_features(RBD_FEATURE_EXCLUSIVE_LOCK)) {
+    mock_image_ctx.exclusive_lock = &mock_exclusive_lock;
+  }
+
+  MockObjectMap mock_object_map;
+  if (ictx->test_features(RBD_FEATURE_OBJECT_MAP)) {
+    mock_image_ctx.object_map = &mock_object_map;
+  }
+
+  expect_op_work_queue(mock_image_ctx);
+
+  ::testing::InSequence seq;
+  expect_snapshot_trash_add(mock_image_ctx, 0);
+
+  uint64_t snap_id = ictx->snap_info.rbegin()->first;
+  cls::rbd::MirrorSnapshotNamespace ns{
+    cls::rbd::MIRROR_SNAPSHOT_STATE_NON_PRIMARY, {}, "", CEPH_NOSNAP};
+  expect_snapshot_get(mock_image_ctx,
+                      {snap_id, {ns}, "mirror", 456, {}, 0}, 0);
+
+  expect_get_parent_spec(mock_image_ctx, 0);
+  expect_object_map_snap_remove(mock_image_ctx, 0);
+  MockRemoveImageStateRequest mock_remove_image_state_request;
+  expect_release_snap_id(mock_image_ctx);
+  expect_snap_remove(mock_image_ctx, 0);
+  expect_rm_snap(mock_image_ctx);
+
+  C_SaferCond cond_ctx;
+  MockSnapshotRemoveRequest *req = new MockSnapshotRemoveRequest(
+    mock_image_ctx, &cond_ctx, ns, "mirror", snap_id);
   {
     std::shared_lock owner_locker{mock_image_ctx.owner_lock};
     req->send();