Merge pull request ceph#62013 from Matan-B/wip-matanb-crimson-coroutine-fixes

crimson: revert coroutine workarounds 

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

Author: Matan-B

src/crimson/osd/ops_executer.cc

@@ -867,19 +867,9 @@ OpsExecuter::flush_changes_and_submit(
     if (auto log_rit = log_entries.rbegin(); log_rit != log_entries.rend()) {
       ceph_assert(log_rit->version == osd_op_params->at_version);
     }
-
-    /*
-     * This works around the gcc bug causing the generated code to incorrectly
-     * execute unconditionally before the predicate.
-     *
-     * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101244
-     */
-    auto clone_obc = cloning_ctx
-      ? std::move(cloning_ctx->clone_obc)
-      : nullptr;
     auto [_submitted, _all_completed] = co_await pg->submit_transaction(
       std::move(obc),
-      std::move(clone_obc),
+      cloning_ctx ? std::move(cloning_ctx->clone_obc) : nullptr,
       std::move(txn),
       std::move(*osd_op_params),
       std::move(log_entries)

src/crimson/osd/osd_operations/client_request.cc

@@ -219,25 +219,8 @@ ClientRequest::interruptible_future<> ClientRequest::with_pg_process_interruptib
   DEBUGDPP("{}.{}: pg active, entering process[_pg]_op",
 	   *pgref, *this, this_instance_id);
 
-  {
-    /* The following works around two different gcc bugs:
-     *  1. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101244
-     *     This example isn't preciesly as described in the bug, but it seems
-     *     similar.  It causes the generated code to incorrectly execute
-     *     process_pg_op unconditionally before the predicate.  It seems to be
-     *     fixed in gcc 12.2.1.
-     *  2. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102217
-     *     This one appears to cause the generated code to double-free
-     *     awaiter holding the future.  This one seems to be fixed
-     *     in gcc 13.2.1.
-     *
-     * Assigning the intermediate result and moving it into the co_await
-     * expression bypasses both bugs.
-     */
-    auto fut = (is_pg_op() ? process_pg_op(pgref) :
-		process_op(ihref, pgref, this_instance_id));
-    co_await std::move(fut);
-  }
+  co_await (is_pg_op() ? process_pg_op(pgref) :
+	    process_op(ihref, pgref, this_instance_id));
 
   DEBUGDPP("{}.{}: process[_pg]_op complete, completing handle",
 	   *pgref, *this, this_instance_id);

src/crimson/osd/osd_operations/replicated_request.cc

@@ -70,21 +70,12 @@ RepRequest::interruptible_future<> RepRequest::with_pg_interruptible(
   LOG_PREFIX(RepRequest::with_pg_interruptible);
   DEBUGI("{}", *this);
   co_await this->template enter_stage<interruptor>(repop_pipeline(*pg).process);
-  {
-    /* Splitting this expression into a fut and a seperate co_await
-     * works around a gcc 11 bug (observed on 11.4.1 and gcc 11.5.0)
-     * which results in the pg ref captured by the lambda being
-     * destructed twice.  We can probably remove these workarounds
-     * once we disallow gcc 11 */
-    auto fut = interruptor::make_interruptible(
-      this->template with_blocking_event<
-      PG_OSDMapGate::OSDMapBlocker::BlockingEvent
-      >([this, pg](auto &&trigger) {
-	return pg->osdmap_gate.wait_for_map(
-	  std::move(trigger), req->min_epoch);
-      }));
-    co_await std::move(fut);
-  }
+  co_await interruptor::make_interruptible(this->template with_blocking_event<
+    PG_OSDMapGate::OSDMapBlocker::BlockingEvent
+    >([this, pg](auto &&trigger) {
+      return pg->osdmap_gate.wait_for_map(
+	std::move(trigger), req->min_epoch);
+    }));
 
   if (pg->can_discard_replica_op(*req)) {
     co_return;

src/test/crimson/test_interruptible_future.cc

@@ -305,8 +305,6 @@ TEST_F(seastar_test_suite_t, interruptible_repeat_eagain)
   });
 }
 
-#if 0
-// This seems to cause a hang in the gcc-9 linker on bionic
 TEST_F(seastar_test_suite_t, handle_error)
 {
   run_async([] {
@@ -324,4 +322,3 @@ TEST_F(seastar_test_suite_t, handle_error)
     ret.unsafe_get();
   });
 }
-#endif