librbd: unlink newest mirror snapshot when at capacity, bump capacity

CreatePrimaryRequest::unlink_peer() invoked via "rbd mirror image
snapshot" command or via rbd_support mgr module when creating a new
scheduled mirror snapshot at rbd_mirroring_max_mirroring_snapshots
capacity on the primary cluster can race with Replayer::unlink_peer()
invoked by rbd-mirror when finishing syncing an older snapshot on the
secondary cluster.  Consider the following:

   [ primary: primary-snap1, primary-snap2, primary-snap3
     secondary: non-primary-snap1 (complete), non-primary-snap2 (syncing) ]

0. rbd-mirror is syncing snap1..snap2 delta
1. rbd_support creates primary-snap4
2. due to rbd_mirroring_max_mirroring_snapshots == 3, rbd_support picks
   primary-snap3 for unlinking
3. rbd-mirror finishes syncing snap1..snap2 delta and marks
   non-primary-snap2 complete

   [ snap1 (the old base) is no longer needed on either cluster ]

4. rbd-mirror unlinks and removes primary-snap1
5. rbd-mirror removes non-primary-snap1
6. rbd-mirror picks snap2 as the new base
7. rbd-mirror creates non-primary-snap3 and starts syncing snap2..snap3
   delta

   [ primary: primary-snap2, primary-snap3, primary-snap4
     secondary: non-primary-snap2 (complete), non-primary-snap3 (syncing) ]

8. rbd_support unlinks and removes primary-snap3 which is in-use by
   rbd-mirror

If snap trimming on the primary cluster kicks in soon enough, the
secondary image becomes corrupted: rbd-mirror would eventually finish
"syncing" non-primary-snap3 and mark it complete in spite of bogus data
in the HEAD -- the primary cluster OSDs would start returning ENOENT
for snap trimmed objects.  Luckily, rbd-mirror's attempt to pick snap3
as the new base would wedge the replayer with "split-brain detected:
failed to find matching non-primary snapshot in remote image" error.

Before commit a888bff8d00e ("librbd/mirror: tweak which snapshot is
unlinked when at capacity") this could happen pretty much all the time
as it was the second oldest snapshot that was unlinked.  This commit
changed it to be the third oldest snapshot, turning this into a more
narrow but still very much possible to hit race.

Unfortunately this race condition appears to be inherent to the way
snapshot-based mirroring is currently implemented:

a. when mirror snapshots are created on the producer side of the
   snapshot queue, they are already linked
b. mirror snapshots can be concurrently unlinked/removed on both
   sides of the snapshot queue by non-cooperating clients (local
   rbd_mirror_image_create_snapshot() vs remote rbd-mirror)
c. with mirror peer links off the list due to (a), there is no
   existing way for rbd-mirror to persistently mark a snapshot as
   in-use

As a workaround, bump rbd_mirroring_max_mirroring_snapshots to 5 and
always unlink the newest snapshot (i.e. slot 4) instead of the third
oldest snapshot (i.e. slot 2).  Hopefully this gives enough leeway,
as rbd-mirror would need to sync two snapshots (i.e. transition from
syncing 0-1 to 1-2 and then to 2-3) before potentially colliding with
rbd_mirror_image_create_snapshot() on slot 4.

Fixes: https://tracker.ceph.com/issues/55803
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

diff --git a/doc/rbd/rbd-mirroring.rst b/doc/rbd/rbd-mirroring.rst
index 464ed2ce92958e398ac51138ddaee5939da4e090..74a2a364e8d0d961e9da4b17aeb55c2b08a78680 100644 (file)
--- a/doc/rbd/rbd-mirroring.rst
+++ b/doc/rbd/rbd-mirroring.rst
@@ -326,7 +326,7 @@ For example::
 
         $ rbd --cluster site-a mirror image snapshot image-pool/image-1
 
-By default only ``3`` mirror-snapshots will be created per-image. The most
+By default up to ``5`` mirror-snapshots will be created per-image. The most
 recent mirror-snapshot is automatically pruned if the limit is reached.
 The limit can be overridden via the ``rbd_mirroring_max_mirroring_snapshots``
 configuration option if required. Additionally, mirror-snapshots are

diff --git a/src/common/options/rbd.yaml.in b/src/common/options/rbd.yaml.in
index 770365db510492abd80ed695dc0f46297153acec..2dc357ae766f2b1c60fc382307677d2bcc5d4571 100644 (file)
--- a/src/common/options/rbd.yaml.in
+++ b/src/common/options/rbd.yaml.in
@@ -466,7 +466,7 @@ options:
   type: uint
   level: advanced
   desc: mirroring snapshots limit
-  default: 3
+  default: 5
   services:
   - rbd
   min: 3

diff --git a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
index 9f8317ed936e2157215acca6f225e7dded3cb752..36cd3bddfc794d630037ec3ae28833f1fb2b0c7b 100644 (file)
--- a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
+++ b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
@@ -218,7 +218,7 @@ void CreatePrimaryRequest<I>::unlink_peer() {
         continue;
       }
       count++;
-      if (count == 3) {
+      if (count == max_snapshots) {
         unlink_snap_id = snap_it.first;
       }
       if (count > max_snapshots) {

diff --git a/src/test/librbd/test_mirroring.cc b/src/test/librbd/test_mirroring.cc
index 4d39ae27e077105c27efc3838d751516e0b11800..480db94d2bf3f81027436dd9658b76b4da40ddb6 100644 (file)
--- a/src/test/librbd/test_mirroring.cc
+++ b/src/test/librbd/test_mirroring.cc
@@ -1120,7 +1120,7 @@ TEST_F(TestMirroring, Snapshot)
   ASSERT_EQ(0, m_rbd.open(m_ioctx, image, image_name.c_str()));
 
   ASSERT_EQ(0, image.metadata_set(
-              "conf_rbd_mirroring_max_mirroring_snapshots", "3"));
+              "conf_rbd_mirroring_max_mirroring_snapshots", "5"));
 
   uint64_t snap_id;
 
@@ -1145,22 +1145,24 @@ TEST_F(TestMirroring, Snapshot)
   ASSERT_EQ(1U, snaps.size());
   ASSERT_EQ(snaps[0].id, snap_id);
 
-  ASSERT_EQ(0, image.mirror_image_create_snapshot(&snap_id));
-  ASSERT_EQ(0, image.mirror_image_create_snapshot(&snap_id));
-  ASSERT_EQ(0, image.mirror_image_create_snapshot(&snap_id));
+  for (int i = 0; i < 5; i++) {
+    ASSERT_EQ(0, image.mirror_image_create_snapshot(&snap_id));
+  }
   snaps.clear();
   ASSERT_EQ(0, image.snap_list(snaps));
-  ASSERT_EQ(3U, snaps.size());
-  ASSERT_EQ(snaps[2].id, snap_id);
+  ASSERT_EQ(5U, snaps.size());
+  ASSERT_EQ(snaps[4].id, snap_id);
 
   // automatic peer unlink on max_mirroring_snapshots reached
   ASSERT_EQ(0, image.mirror_image_create_snapshot(&snap_id));
   vector<librbd::snap_info_t> snaps1;
   ASSERT_EQ(0, image.snap_list(snaps1));
-  ASSERT_EQ(3U, snaps1.size());
+  ASSERT_EQ(5U, snaps1.size());
   ASSERT_EQ(snaps1[0].id, snaps[0].id);
   ASSERT_EQ(snaps1[1].id, snaps[1].id);
-  ASSERT_EQ(snaps1[2].id, snap_id);
+  ASSERT_EQ(snaps1[2].id, snaps[2].id);
+  ASSERT_EQ(snaps1[3].id, snaps[3].id);
+  ASSERT_EQ(snaps1[4].id, snap_id);
 
   librbd::snap_namespace_type_t snap_ns_type;
   ASSERT_EQ(0, image.snap_get_namespace_type(snap_id, &snap_ns_type));