crimson/osd: optimize crimson-osd's client requests process parallelism by xxhdx1985126 · Pull Request #39772 · ceph/ceph

xxhdx1985126 · 2021-03-02T04:35:54Z

allow successive client requests to go into the "process" pipeline stage
once the current one is submitted to the pg backend.

Signed-off-by: Xuehan Xu xxhdx1985126@gmail.com

Checklist

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

xxhdx1985126 · 2021-03-02T05:14:56Z

jenkins test make check

tchaikov · 2021-03-02T05:36:27Z

jenkins test crimson perf

tchaikov · 2021-03-02T11:32:20Z

jenkins test crimson perf

github-actions · 2021-03-02T12:53:21Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

xxhdx1985126 · 2021-03-04T12:02:03Z

jenkins test crimson perf

xxhdx1985126 · 2021-03-08T04:19:56Z

There seems to be some bugs in this PR which I'm trying to fix right now.

github-actions · 2021-03-08T16:14:13Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

src/crimson/osd/pg.cc

xxhdx1985126 · 2021-03-11T08:30:35Z

https://pulpito.ceph.com/xuxuehan-2021-03-11_05:07:45-crimson-rados:thrash-wip-crimson-interruptible-future-pipeline-parallelism-distro-basic-smithi/
https://pulpito.ceph.com/xuxuehan-2021-03-10_11:57:36-crimson-rados:thrash-wip-crimson-interruptible-future-pipeline-parallelism-distro-basic-smithi/

github-actions · 2021-04-19T09:02:04Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

src/crimson/common/interruptible_future.h

src/crimson/osd/ops_executer.h

tchaikov · 2021-05-12T15:45:01Z

@rzarzynski could you take a final look?

liu-chunmei · 2021-05-12T23:31:07Z

src/crimson/osd/ops_executer.h

+  rep_op_fut_t maybe_mutated =
+    interruptor::make_ready_future<rep_op_fut_tuple>(
+	seastar::now(),
+	interruptor::make_interruptible(osd_op_errorator::now()));


does it need to initialize maybe_mutated here? it is set value in the following.

we can improve this part in a follow-up change. i think @xxhdx1985126 is just reusing the existing code structure.

please note, maybe_mutated initialized here is used if op_effects.empty().

liu-chunmei · 2021-05-12T23:32:23Z

src/crimson/osd/ops_executer.h

-      return interruptor::do_for_each(op_effects, [] (auto& op_effect) {
-        return op_effect->execute();
-      });
+    return maybe_mutated.then_unpack_interruptible(


why not safe_then_unpack_interruptible here?

the return type of PG::submit_transation is an interruptible_future_detail<InterruptCond, seastar::future<...>> now, it is a bit wierd here.

liu-chunmei · 2021-05-13T00:49:21Z

src/crimson/osd/pg.cc

  const auto oid = m->get_snapid() == CEPH_SNAPDIR ? m->get_hobj().get_head()
                                                   : m->get_hobj();
-  auto ox = std::make_unique<OpsExecuter>(
+  auto ox = seastar::make_lw_shared<OpsExecuter>(


why use lw_shared_ptr here?

@xxhdx1985126 i have the same question.

There are two places holding references to OpsExecuter, one being continuations following the all_completed future and the other the old one, now. The completion order of these two can not be determined at compile time. So I made this pointer a shared one.

I noticed all places hold ox by ox.get(), so it not transfer the smart ptr, it transfer the raw ptr in ox, does it have problem by using unique_ptr?

I think so. There may be chances that the unique_ptr gets destroyed before other raw ptrs, which would lead to memory access violations

liu-chunmei · 2021-05-13T01:09:13Z

src/crimson/osd/pg.cc

-      return PG::do_osd_ops_ertr::future<Ref<MOSDOpReply>>(
-      crimson::ct_error::eagain::make());
+      return PG::do_osd_ops_ertr::future<rep_op_fut_t>(
+              crimson::ct_error::eagain::make());


does it miss a seastar::now() future here? since return rep_op_fut_t it is a tuple。

please note, we are not creating a future with its value here. we are returning a future carrying error. in this case, we don't pass a tuple or the argument for constructing a tuple to the constructor of errorator::future<>. otherwise, we'd use make_ready_future<>() instead.

liu-chunmei · 2021-05-13T01:20:53Z

src/crimson/osd/pg_backend.h

      ::crimson::osd::IOInterruptCondition, T>;
+  using rep_op_fut_t =
+    std::tuple<interruptible_future<>,
+	       interruptible_future<crimson::osd::acked_peers_t>>;


there are two rep_op_fut_t definition in pg.h and pg_backend.h. can we use different name to identify them?

ok, i'll deal with it

src/crimson/osd/pg.cc

github-actions · 2021-05-13T06:34:04Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

xxhdx1985126 · 2021-05-16T06:43:41Z

jenkins test make check

These two stages are used to provide more parallelism in the pipeline, while preserving client requests order. Signed-off-by: Xuehan Xu <xxhdx1985126@gmail.com>

Signed-off-by: Xuehan Xu <xxhdx1985126@gmail.com>

Make client requests go to the concurrent pipeline stage "wait_repop" once they are "submitted" to the underlying objectstore, which means their on-disk order is guaranteed, so that successive client requests can go into the "process" pipeline stage. Signed-off-by: Xuehan Xu <xxhdx1985126@gmail.com>

src/crimson/osd/pg.cc

dismiss my approval as the last commit does not look right.

src/crimson/osd/pg.cc

otherwise any async execution of lambdas in PG::do_osd_ops_execute() may reference a outdated osd_op Signed-off-by: Xuehan Xu <xxhdx1985126@gmail.com>

xxhdx1985126 requested a review from a team as a code owner March 2, 2021 04:35

github-actions bot added the crimson label Mar 2, 2021

xxhdx1985126 requested review from athanatos, rzarzynski and tchaikov March 2, 2021 04:58

xxhdx1985126 mentioned this pull request Mar 2, 2021

crimson/os/alienstore: scatter alienstore threads onto specified cpu cores #39777

Merged

3 tasks

ceph deleted a comment from xxhdx1985126 Mar 2, 2021

github-actions bot added the needs-rebase label Mar 2, 2021

xxhdx1985126 force-pushed the wip-crimson-client-req-pipeline-parallelism branch from 895356c to 68d690d Compare March 3, 2021 02:50

github-actions bot removed the needs-rebase label Mar 3, 2021

markhpc added the performance label Mar 4, 2021

xxhdx1985126 changed the title ~~crimson/osd: optimize crimson-osd's client requests process parallelism~~ [DNM]crimson/osd: optimize crimson-osd's client requests process parallelism Mar 8, 2021

xxhdx1985126 force-pushed the wip-crimson-client-req-pipeline-parallelism branch from 68d690d to ac65eba Compare March 8, 2021 08:06

github-actions bot added the needs-rebase label Mar 8, 2021

tchaikov reviewed Mar 9, 2021

View reviewed changes

src/crimson/osd/pg.cc Show resolved Hide resolved

xxhdx1985126 force-pushed the wip-crimson-client-req-pipeline-parallelism branch from ac65eba to 47b25f5 Compare March 11, 2021 05:23

github-actions bot removed the needs-rebase label Mar 11, 2021

xxhdx1985126 force-pushed the wip-crimson-client-req-pipeline-parallelism branch from 47b25f5 to 469d390 Compare March 11, 2021 08:30

xxhdx1985126 changed the title ~~[DNM]crimson/osd: optimize crimson-osd's client requests process parallelism~~ crimson/osd: optimize crimson-osd's client requests process parallelism Mar 11, 2021

github-actions bot added the needs-rebase label Apr 19, 2021

xxhdx1985126 force-pushed the wip-crimson-client-req-pipeline-parallelism branch from 8641700 to 932ba74 Compare May 11, 2021 01:48

github-actions bot removed the needs-rebase label May 11, 2021

xxhdx1985126 force-pushed the wip-crimson-client-req-pipeline-parallelism branch from 932ba74 to c309aaf Compare May 11, 2021 04:05

tchaikov reviewed May 12, 2021

View reviewed changes

src/crimson/common/interruptible_future.h Outdated Show resolved Hide resolved

tchaikov reviewed May 12, 2021

View reviewed changes

src/crimson/osd/ops_executer.h Outdated Show resolved Hide resolved

xxhdx1985126 force-pushed the wip-crimson-client-req-pipeline-parallelism branch from c309aaf to 6e0a594 Compare May 12, 2021 15:34

tchaikov previously approved these changes May 12, 2021

View reviewed changes

liu-chunmei reviewed May 13, 2021

View reviewed changes

tchaikov reviewed May 13, 2021

View reviewed changes

src/crimson/osd/pg.cc Outdated Show resolved Hide resolved

github-actions bot added the needs-rebase label May 13, 2021

xxhdx1985126 force-pushed the wip-crimson-client-req-pipeline-parallelism branch from 6e0a594 to e72600a Compare May 15, 2021 10:39

github-actions bot removed the needs-rebase label May 15, 2021

xxhdx1985126 added 3 commits May 16, 2021 14:47

crimson/osd: add two more stages into pg's client request pipeline

0817a7c

These two stages are used to provide more parallelism in the pipeline, while preserving client requests order. Signed-off-by: Xuehan Xu <xxhdx1985126@gmail.com>

crimson/common: add new facilities to interruptible future

a0eaf67

Signed-off-by: Xuehan Xu <xxhdx1985126@gmail.com>

xxhdx1985126 force-pushed the wip-crimson-client-req-pipeline-parallelism branch from e72600a to fd1ee08 Compare May 16, 2021 15:24

tchaikov reviewed May 16, 2021

View reviewed changes

src/crimson/osd/pg.cc Show resolved Hide resolved

tchaikov reviewed May 17, 2021

View reviewed changes

src/crimson/osd/pg.cc Outdated Show resolved Hide resolved

crimson/osd: make do_osd_ops receive lvalue reference to osd ops vector

0a0848d

otherwise any async execution of lambdas in PG::do_osd_ops_execute() may reference a outdated osd_op Signed-off-by: Xuehan Xu <xxhdx1985126@gmail.com>

xxhdx1985126 force-pushed the wip-crimson-client-req-pipeline-parallelism branch from fd1ee08 to 0a0848d Compare May 17, 2021 13:51

tchaikov approved these changes May 17, 2021

View reviewed changes

rzarzynski approved these changes May 17, 2021

View reviewed changes

tchaikov merged commit 0b795da into ceph:master May 18, 2021

Matan-B mentioned this pull request Nov 6, 2023

crimson/osd: execute submit_error_log as an ExclusivePhase #54287

Merged

14 tasks

Matan-B added the crimson-perf label Jan 3, 2024

xxhdx1985126 deleted the wip-crimson-client-req-pipeline-parallelism branch April 23, 2024 02:48

Conversation

xxhdx1985126 commented Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

xxhdx1985126 commented Mar 2, 2021

Uh oh!

tchaikov commented Mar 2, 2021

Uh oh!

tchaikov commented Mar 2, 2021

Uh oh!

github-actions bot commented Mar 2, 2021

Uh oh!

xxhdx1985126 commented Mar 4, 2021

Uh oh!

xxhdx1985126 commented Mar 8, 2021

Uh oh!

github-actions bot commented Mar 8, 2021

Uh oh!

Uh oh!

xxhdx1985126 commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 19, 2021

Uh oh!

Uh oh!

Uh oh!

tchaikov commented May 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tchaikov May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xxhdx1985126 May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tchaikov May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented May 13, 2021

Uh oh!

xxhdx1985126 commented May 16, 2021

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

xxhdx1985126 commented Mar 2, 2021 •

edited

Loading

xxhdx1985126 commented Mar 11, 2021 •

edited

Loading

tchaikov May 13, 2021 •

edited

Loading

xxhdx1985126 May 13, 2021 •

edited

Loading

tchaikov May 13, 2021 •

edited

Loading