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>
(cherry picked from commit ef83c0f347c38f3eee70d554d5433a1a17b209b6)

Conflicts:
	src/common/options/rbd.yaml.in [ options are defined in
	  src/common/options.cc in octopus ]

diff --git a/doc/rbd/rbd-mirroring.rst b/doc/rbd/rbd-mirroring.rst
index 9e83aa58baee5879fe182444163fe6b75a9d05be..f5444ada914a114c5299bd71dd40566d963261ce 100644 (file)
--- a/doc/rbd/rbd-mirroring.rst
+++ b/doc/rbd/rbd-mirroring.rst
@@ -303,7 +303,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.cc b/src/common/options.cc
index 3670fa0f4a232405cc563bb5de0cbc35b88b70f7..afd291d060d1febca4c26ea2953610e86b9d1c74 100644 (file)
--- a/src/common/options.cc
+++ b/src/common/options.cc
@@ -7436,7 +7436,7 @@ static std::vector<Option> get_rbd_options() {
     .set_description("time-delay in seconds for rbd-mirror asynchronous replication"),
 
     Option("rbd_mirroring_max_mirroring_snapshots", Option::TYPE_UINT, Option::LEVEL_ADVANCED)
-    .set_default(3)
+    .set_default(5)
     .set_min(3)
     .set_description("mirroring snapshots limit"),
 

diff --git a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
index 65a8a05b97a462facd483cdcdb110df84d5e55ca..1461d188d5bfb71a4dd11f0cbd4803ccfed5da80 100644 (file)
--- a/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
+++ b/src/librbd/mirror/snapshot/CreatePrimaryRequest.cc
@@ -216,7 +216,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 918b57f555ee345f4d87d499a3321dff0f38f69a..fc92817938dbcf2639ab0b72770f74cd90652c30 100644 (file)
--- a/src/test/librbd/test_mirroring.cc
+++ b/src/test/librbd/test_mirroring.cc
@@ -1119,7 +1119,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;
 
@@ -1144,22 +1144,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));