osd: Revert "osd: avoid two copy with same src cancel each other"

This reverts commit 617f711. Fixes: https://tracker.ceph.com/issues/49726 Signed-off-by: Kefu Chai <[email protected]>
mychoxin · Mar 12, 2021 · 6c5ab5d · 6c5ab5d
1 parent 1d590a0
commit 6c5ab5d
Show file tree

Hide file tree

Showing 3 changed files with 5 additions and 117 deletions.
diff --git a/src/osd/PrimaryLogPG.cc b/src/osd/PrimaryLogPG.cc
@@ -10060,6 +10060,7 @@ void PrimaryLogPG::cancel_copy(CopyOpRef cop, bool requeue,
   copy_ops.erase(cop->obc->obs.oi.soid);
   cop->obc->stop_block();
 
+  kick_object_context_blocked(cop->obc);
   cop->results.should_requeue = requeue;
   CopyCallbackResults result(-ECANCELED, &cop->results);
   cop->cb->complete(result);
@@ -10070,15 +10071,13 @@ void PrimaryLogPG::cancel_copy(CopyOpRef cop, bool requeue,
   cop->obc = ObjectContextRef();
 }
 
-void PrimaryLogPG::cancel_and_kick_copy_ops(bool requeue, vector<ceph_tid_t> *tids)
+void PrimaryLogPG::cancel_copy_ops(bool requeue, vector<ceph_tid_t> *tids)
 {
   dout(10) << __func__ << dendl;
   map<hobject_t,CopyOpRef>::iterator p = copy_ops.begin();
   while (p != copy_ops.end()) {
-    ObjectContextRef obc = p->second->obc;
     // requeue this op? can I queue up all of them?
     cancel_copy((p++)->second, requeue, tids);
-    kick_object_context_blocked(obc);
   }
 }
 
@@ -12482,7 +12481,7 @@ void PrimaryLogPG::on_shutdown()
   m_scrubber->unreg_next_scrub();
 
   vector<ceph_tid_t> tids;
-  cancel_and_kick_copy_ops(false, &tids);
+  cancel_copy_ops(false, &tids);
   cancel_flush_ops(false, &tids);
   cancel_proxy_ops(false, &tids);
   cancel_manifest_ops(false, &tids);
@@ -12599,7 +12598,7 @@ void PrimaryLogPG::on_change(ObjectStore::Transaction &t)
   requeue_ops(waiting_for_readable);
 
   vector<ceph_tid_t> tids;
-  cancel_and_kick_copy_ops(is_primary(), &tids);
+  cancel_copy_ops(is_primary(), &tids);
   cancel_flush_ops(is_primary(), &tids);
   cancel_proxy_ops(is_primary(), &tids);
   cancel_manifest_ops(is_primary(), &tids);

diff --git a/src/osd/PrimaryLogPG.h b/src/osd/PrimaryLogPG.h
@@ -1367,7 +1367,7 @@ class PrimaryLogPG : public PG, public PGBackend::Listener {
   void finish_copyfrom(CopyFromCallback *cb);
   void finish_promote(int r, CopyResults *results, ObjectContextRef obc);
   void cancel_copy(CopyOpRef cop, bool requeue, std::vector<ceph_tid_t> *tids);
-  void cancel_and_kick_copy_ops(bool requeue, std::vector<ceph_tid_t> *tids);
+  void cancel_copy_ops(bool requeue, std::vector<ceph_tid_t> *tids);
 
   friend struct C_Copyfrom;
 

diff --git a/src/test/librados/tier_cxx.cc b/src/test/librados/tier_cxx.cc
@@ -1506,117 +1506,6 @@ TEST_F(LibRadosTwoPoolsPP, ListSnap){
   ioctx.selfmanaged_snap_remove(my_snaps[0]);
 }
 
-// This test case reproduces https://tracker.ceph.com/issues/49409
-TEST_F(LibRadosTwoPoolsPP, EvictSnapRollbackReadRace) {
-  // create object
-  {
-    bufferlist bl;
-    int len = string("hi there").length() * 2;
-    // append more chrunk data make sure the second promote
-    // op coming before the first promote op finished
-    for (int i=0; i<4*1024*1024/len; ++i)
-      bl.append("hi therehi there");
-    ObjectWriteOperation op;
-    op.write_full(bl);
-    ASSERT_EQ(0, ioctx.operate("foo", &op));
-  }
-
-  // create two snapshot, a clone
-  vector<uint64_t> my_snaps(2);
-  ASSERT_EQ(0, ioctx.selfmanaged_snap_create(&my_snaps[1]));
-  ASSERT_EQ(0, ioctx.selfmanaged_snap_create(&my_snaps[0]));
-  ASSERT_EQ(0, ioctx.selfmanaged_snap_set_write_ctx(my_snaps[0],
-                                                         my_snaps));
-  {
-    bufferlist bl;
-    bl.append("ciao!");
-    ObjectWriteOperation op;
-    op.write_full(bl);
-    ASSERT_EQ(0, ioctx.operate("foo", &op));
-  }
-
-  // configure cache
-  bufferlist inbl;
-  ASSERT_EQ(0, cluster.mon_command(
-    "{\"prefix\": \"osd tier add\", \"pool\": \"" + pool_name +
-    "\", \"tierpool\": \"" + cache_pool_name +
-    "\", \"force_nonempty\": \"--force-nonempty\" }",
-    inbl, NULL, NULL));
-  ASSERT_EQ(0, cluster.mon_command(
-    "{\"prefix\": \"osd tier set-overlay\", \"pool\": \"" + pool_name +
-    "\", \"overlaypool\": \"" + cache_pool_name + "\"}",
-    inbl, NULL, NULL));
-  ASSERT_EQ(0, cluster.mon_command(
-    "{\"prefix\": \"osd tier cache-mode\", \"pool\": \"" + cache_pool_name +
-    "\", \"mode\": \"writeback\"}",
-    inbl, NULL, NULL));
-
-  // wait for maps to settle
-  cluster.wait_for_latest_osdmap();
-
-  // read, trigger a promote on the head
-  {
-    bufferlist bl;
-    ASSERT_EQ(1, ioctx.read("foo", bl, 1, 0));
-    ASSERT_EQ('c', bl[0]);
-  }
-
-  // try more times
-  int retries = 50;
-  for (int i=0; i<retries; ++i)
-  {
-    {
-      librados::AioCompletion * completion = cluster.aio_create_completion();
-      librados::AioCompletion * completion1 = cluster.aio_create_completion();
-
-      // send a snap rollback op and a snap read op parallel
-      // trigger two promote(copy) to the same snap clone obj
-      // the second snap read op is read-ordered make sure
-      // op not wait for objects_blocked_on_snap_promotion
-      ObjectWriteOperation op;
-      op.selfmanaged_snap_rollback(my_snaps[0]);
-      ASSERT_EQ(0, ioctx.aio_operate(
-        "foo", completion, &op));
-
-      ioctx.snap_set_read(my_snaps[1]);
-      std::map<uint64_t, uint64_t> extents;
-      bufferlist read_bl;
-      int rval = -1;
-      ObjectReadOperation op1;
-      op1.sparse_read(0, 8, &extents, &read_bl, &rval);
-      ASSERT_EQ(0, ioctx.aio_operate("foo", completion1, &op1, &read_bl));
-      ioctx.snap_set_read(librados::SNAP_HEAD);
-
-      completion->wait_for_safe();
-      ASSERT_EQ(0, completion->get_return_value());
-      completion->release();
-
-      completion1->wait_for_safe();
-      ASSERT_EQ(0, completion1->get_return_value());
-      completion1->release();
-    }
-
-    // evict foo snap
-    ioctx.snap_set_read(my_snaps[0]);
-    {
-      ObjectReadOperation op;
-      op.cache_evict();
-      librados::AioCompletion *completion = cluster.aio_create_completion();
-      ASSERT_EQ(0, ioctx.aio_operate(
-        "foo", completion, &op,
-        librados::OPERATION_IGNORE_CACHE, NULL));
-      completion->wait_for_safe();
-      ASSERT_EQ(0, completion->get_return_value());
-      completion->release();
-    }
-    ioctx.snap_set_read(librados::SNAP_HEAD);
-  }
-
-  // cleanup
-  ioctx.selfmanaged_snap_remove(my_snaps[0]);
-  ioctx.selfmanaged_snap_remove(my_snaps[1]);
-}
-
 TEST_F(LibRadosTwoPoolsPP, TryFlush) {
   // configure cache
   bufferlist inbl;