librbd/mirror: detect trashed snapshots in UnlinkPeerRequest

If two instances of UnlinkPeerRequest race with each other (e.g. due
to rbd-mirror daemon unlinking from a previous mirror snapshot and the
user taking another mirror snapshot at same time), the snapshot that
UnlinkPeerRequest was created for may be in the process of being removed
(which may mean trashed by SnapshotRemoveRequest::trash_snap()) or fully
removed by the time unlink_peer() grabs the image lock.  Because trashed
snapshots weren't handled explicitly, UnlinkPeerRequest could spuriously
fail with EINVAL ("not mirror snapshot" case) instead of the expected
ENOENT ("missing snapshot" case).  This in turn could lead to spurious
ImageReplayer failures with it stopping prematurely.

Fixes: https://tracker.ceph.com/issues/68279
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
(cherry picked from commit 3596ca077097a4e0ff8e8d05a410c2044332391e)

diff --git a/src/librbd/mirror/snapshot/UnlinkPeerRequest.cc b/src/librbd/mirror/snapshot/UnlinkPeerRequest.cc
index 35313f6277981bb6876724dcc3b316043aea2d91..350c290215ac1487e712758bcf740815e94a4948 100644 (file)
--- a/src/librbd/mirror/snapshot/UnlinkPeerRequest.cc
+++ b/src/librbd/mirror/snapshot/UnlinkPeerRequest.cc
@@ -79,10 +79,13 @@ void UnlinkPeerRequest<I>::unlink_peer() {
     }
   }
 
-  if (r == -ENOENT) {
-    ldout(cct, 15) << "missing snapshot: snap_id=" << m_snap_id << dendl;
+  if (r == -ENOENT ||
+      std::holds_alternative<cls::rbd::TrashSnapshotNamespace>(
+        snap_namespace)) {
+    ldout(cct, 15) << "missing or trashed snapshot: snap_id=" << m_snap_id
+                   << dendl;
     m_image_ctx->image_lock.unlock_shared();
-    finish(r);
+    finish(-ENOENT);
     return;
   }
 

diff --git a/src/test/librbd/mirror/snapshot/test_mock_UnlinkPeerRequest.cc b/src/test/librbd/mirror/snapshot/test_mock_UnlinkPeerRequest.cc
index 869bdecff479f6fb32ed3b1ee296abf1ac83d57e..32608f3c2f56ba01258a70514a4e503ca46e720f 100644 (file)
--- a/src/test/librbd/mirror/snapshot/test_mock_UnlinkPeerRequest.cc
+++ b/src/test/librbd/mirror/snapshot/test_mock_UnlinkPeerRequest.cc
@@ -291,6 +291,31 @@ TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, SnapshotDNE) {
   ASSERT_EQ(-ENOENT, ctx.wait());
 }
 
+TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, TrashedSnapshot) {
+  REQUIRE_FORMAT_V2();
+
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(m_image_name, &ictx));
+
+  MockTestImageCtx mock_image_ctx(*ictx);
+  cls::rbd::TrashSnapshotNamespace ns{
+    cls::rbd::SNAPSHOT_NAMESPACE_TYPE_MIRROR, "mirror_snap"};
+  auto snap_id = snap_create(mock_image_ctx, ns, "trash_snap");
+
+  expect_get_snap_info(mock_image_ctx, snap_id);
+
+  InSequence seq;
+
+  expect_is_refresh_required(mock_image_ctx, true);
+  expect_refresh_image(mock_image_ctx, 0);
+
+  C_SaferCond ctx;
+  auto req = new MockUnlinkPeerRequest(&mock_image_ctx, snap_id, "peer_uuid",
+                                       true, &ctx);
+  req->send();
+  ASSERT_EQ(-ENOENT, ctx.wait());
+}
+
 TEST_F(TestMockMirrorSnapshotUnlinkPeerRequest, PeerDNE) {
   REQUIRE_FORMAT_V2();