Merge pull request ceph#50530 from Matan-B/wip-matanb-crimson-only-new-rbd-v2

crimson/osd: Handle rollback to head obejct

Reviewed-by: Samuel Just <[email protected]>

Author: Matan-B

src/crimson/osd/pg_backend.cc

@@ -784,27 +784,43 @@ PGBackend::rollback_iertr::future<> PGBackend::rollback(
                   __func__, os.oi.soid ,snapid);
   hobject_t target_coid = os.oi.soid;
   target_coid.snap = snapid;
-  return obc_loader.with_clone_obc_only<RWState::RWREAD>(
+  return obc_loader.with_clone_obc_only<RWState::RWWRITE>(
     head, target_coid,
     [this, &os, &txn, &delta_stats, &osd_op_params]
-    (auto clone_obc) {
+    (auto resolved_obc) {
+    if (resolved_obc->obs.oi.soid.is_head()) {
+      // no-op: The resolved oid returned the head object
+      logger().debug("PGBackend::rollback: loaded head_obc: {}"
+                     " do nothing",
+                     resolved_obc->obs.oi.soid);
+      return rollback_iertr::now();
+    }
+    /* TODO: https://tracker.ceph.com/issues/59114 This implementation will not
+     * behave correctly for a rados operation consisting of a mutation followed
+     * by a rollback to a snapshot since the last mutation of the object.
+     * The correct behavior would be for the rollback to undo the mutation
+     * earlier in the operation by resolving to the clone created at the start
+     * of the operation (see resolve_oid).
+     * Instead, it will select HEAD leaving that mutation intact since the SnapSet won't
+     * yet contain that clone. This behavior exists in classic as well.
+     */
     logger().debug("PGBackend::rollback: loaded clone_obc: {}",
-                   clone_obc->obs.oi.soid);
+                   resolved_obc->obs.oi.soid);
     // 1) Delete current head
     if (os.exists) {
       txn.remove(coll->get_cid(), ghobject_t{os.oi.soid,
                                   ghobject_t::NO_GEN, shard});
     }
     // 2) Clone correct snapshot into head
-    txn.clone(coll->get_cid(), ghobject_t{clone_obc->obs.oi.soid},
+    txn.clone(coll->get_cid(), ghobject_t{resolved_obc->obs.oi.soid},
                                ghobject_t{os.oi.soid});
     //    Copy clone obc.os.oi to os.oi
     os.oi.clear_flag(object_info_t::FLAG_WHITEOUT);
-    os.oi.copy_user_bits(clone_obc->obs.oi);
+    os.oi.copy_user_bits(resolved_obc->obs.oi);
     delta_stats.num_bytes -= os.oi.size;
-    delta_stats.num_bytes += clone_obc->obs.oi.size;
+    delta_stats.num_bytes += resolved_obc->obs.oi.size;
     osd_op_params.clean_regions.mark_data_region_dirty(0,
-      std::max(os.oi.size, clone_obc->obs.oi.size));
+      std::max(os.oi.size, resolved_obc->obs.oi.size));
     osd_op_params.clean_regions.mark_omap_dirty();
     // TODO: 3) Calculate clone_overlaps by following overlaps
     //          forward from rollback snapshot

src/test/librados/snapshots.cc

@@ -131,6 +131,7 @@ TEST_F(LibRadosSnapshotsSelfManaged, Rollback) {
   ::std::reverse(my_snaps.begin(), my_snaps.end());
   char buf[bufsize];
   memset(buf, 0xcc, sizeof(buf));
+  // First write
   ASSERT_EQ(0, rados_write(ioctx, "foo", buf, sizeof(buf), 0));
 
   my_snaps.push_back(-2);
@@ -141,7 +142,9 @@ TEST_F(LibRadosSnapshotsSelfManaged, Rollback) {
   ::std::reverse(my_snaps.begin(), my_snaps.end());
   char buf2[sizeof(buf)];
   memset(buf2, 0xdd, sizeof(buf2));
+  // Second write
   ASSERT_EQ(0, rados_write(ioctx, "foo", buf2, sizeof(buf2), 0));
+  // Rollback to my_snaps[1] - Object is expeceted to conatin the first write
   rados_ioctx_selfmanaged_snap_rollback(ioctx, "foo", my_snaps[1]);
   char buf3[sizeof(buf)];
   ASSERT_EQ((int)sizeof(buf3), rados_read(ioctx, "foo", buf3, sizeof(buf3), 0));
@@ -154,6 +157,51 @@ TEST_F(LibRadosSnapshotsSelfManaged, Rollback) {
   ASSERT_EQ(0, rados_remove(ioctx, "foo"));
 }
 
+TEST_F(LibRadosSnapshotsSelfManaged, FutureSnapRollback) {
+  std::vector<uint64_t> my_snaps;
+  // Snapshot 1
+  my_snaps.push_back(-2);
+  ASSERT_EQ(0, rados_ioctx_selfmanaged_snap_create(ioctx, &my_snaps.back()));
+  ::std::reverse(my_snaps.begin(), my_snaps.end());
+  ASSERT_EQ(0, rados_ioctx_selfmanaged_snap_set_write_ctx(ioctx, my_snaps[0],
+					&my_snaps[0], my_snaps.size()));
+  ::std::reverse(my_snaps.begin(), my_snaps.end());
+  char buf[bufsize];
+  memset(buf, 0xcc, sizeof(buf));
+  // First write
+  ASSERT_EQ(0, rados_write(ioctx, "foo", buf, sizeof(buf), 0));
+
+  // Snapshot 2
+  my_snaps.push_back(-2);
+  ASSERT_EQ(0, rados_ioctx_selfmanaged_snap_create(ioctx, &my_snaps.back()));
+  ::std::reverse(my_snaps.begin(), my_snaps.end());
+  ASSERT_EQ(0, rados_ioctx_selfmanaged_snap_set_write_ctx(ioctx, my_snaps[0],
+					&my_snaps[0], my_snaps.size()));
+  ::std::reverse(my_snaps.begin(), my_snaps.end());
+  char buf2[sizeof(buf)];
+  memset(buf2, 0xdd, sizeof(buf2));
+  // Second write
+  ASSERT_EQ(0, rados_write(ioctx, "foo", buf2, sizeof(buf2), 0));
+  // Snapshot 3
+  my_snaps.push_back(-2);
+  ASSERT_EQ(0, rados_ioctx_selfmanaged_snap_create(ioctx, &my_snaps.back()));
+
+  // Rollback to the last snap id - Object is expected to conatin
+  // latest write (head object)
+  rados_ioctx_selfmanaged_snap_rollback(ioctx, "foo", my_snaps[2]);
+  char buf3[sizeof(buf)];
+  ASSERT_EQ((int)sizeof(buf3), rados_read(ioctx, "foo", buf3, sizeof(buf3), 0));
+  ASSERT_EQ(0, memcmp(buf3, buf2, sizeof(buf)));
+
+  ASSERT_EQ(0, rados_ioctx_selfmanaged_snap_remove(ioctx, my_snaps.back()));
+  my_snaps.pop_back();
+  ASSERT_EQ(0, rados_ioctx_selfmanaged_snap_remove(ioctx, my_snaps.back()));
+  my_snaps.pop_back();
+  ASSERT_EQ(0, rados_remove(ioctx, "foo"));
+}
+
+
+
 // EC testing
 TEST_F(LibRadosSnapshotsEC, SnapList) {
   SKIP_IF_CRIMSON();

src/test/librados/snapshots_cxx.cc

@@ -458,6 +458,53 @@ TEST_F(LibRadosSnapshotsSelfManagedPP, OrderSnap) {
   comp4->release();
 }
 
+TEST_F(LibRadosSnapshotsSelfManagedPP, WriteRollback) {
+  // https://tracker.ceph.com/issues/59114
+  GTEST_SKIP();
+  uint64_t snapid = 5;
+
+  // buf1
+  char buf[bufsize];
+  memset(buf, 0xcc, sizeof(buf));
+  bufferlist bl;
+  bl.append(buf, sizeof(buf));
+
+  // buf2
+  char buf2[sizeof(buf)];
+  memset(buf2, 0xdd, sizeof(buf2));
+  bufferlist bl2;
+  bl2.append(buf2, sizeof(buf2));
+
+  // First write
+  ObjectWriteOperation op_write1;
+  op_write1.write(0, bl);
+  // Operate
+  librados::AioCompletion *comp_write = cluster.aio_create_completion();
+  ASSERT_EQ(0, ioctx.aio_operate("foo", comp_write, &op_write1, 0));
+  ASSERT_EQ(0, comp_write->wait_for_complete());
+  ASSERT_EQ(0, comp_write->get_return_value());
+  comp_write->release();
+
+  // Take Snapshot
+  ASSERT_EQ(0, ioctx.selfmanaged_snap_create(&snapid));
+
+  // Rollback + Second write in the same op
+  ObjectWriteOperation op_write2_snap_rollback;
+  op_write2_snap_rollback.write(0, bl2);
+  op_write2_snap_rollback.selfmanaged_snap_rollback(snapid);
+  // Operate
+  librados::AioCompletion *comp_write2 = cluster.aio_create_completion();
+  ASSERT_EQ(0, ioctx.aio_operate("foo", comp_write2, &op_write2_snap_rollback, 0));
+  ASSERT_EQ(0, comp_write2->wait_for_complete());
+  ASSERT_EQ(0, comp_write2->get_return_value());
+  comp_write2->release();
+
+  // Resolved should be first write
+  bufferlist bl3;
+  EXPECT_EQ((int)sizeof(buf), ioctx.read("foo", bl3, sizeof(buf), 0));
+  EXPECT_EQ(0, memcmp(buf, bl3.c_str(), sizeof(buf)));
+}
+
 TEST_F(LibRadosSnapshotsSelfManagedPP, ReusePurgedSnap) {
   std::vector<uint64_t> my_snaps;
   my_snaps.push_back(-2);