osd: dissect and abstract RMWPipeline from ECBackend for sharing it with crimson #52211

rzarzynski · 2023-06-27T14:33:45Z

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

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 dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

athanatos

Seems reasonable, let's use a Op::Ref or Ref rather than explicitly using std::unique_ptr each time, though.

athanatos · 2023-06-30T02:42:20Z

src/osd/ECBackend.cc

-#define dout_prefix _prefix(_dout, this)
+//#define DOUT_PREFIX_ARGS this
+//#undef dout_prefix
+//#define dout_prefix _prefix(_dout, this)


Can we just remove these?

Added the overload:

static ostream& _prefix(std::ostream *_dout, ECBackend::RMWPipeline *rmw_pipeline) {

athanatos · 2023-06-30T02:43:24Z

src/osd/ECBackend.cc

-    &committed_to=this->committed_to,
-    &tid_to_op_map=this->tid_to_op_map,
-    cct=(CephContext*)nullptr
-  ] {


Can we just squash this into the prior commit? It's not particularly useful for review.

Sure, will squash.

athanatos · 2023-06-30T02:44:08Z

src/osd/ECBackend.cc

@@ -1161,30 +1161,30 @@ void ECBackend::handle_sub_write_reply(
  const ECSubWriteReply &op,
  const ZTracer::Trace &trace)
 {
-  map<ceph_tid_t, Op>::iterator i = rmw_pipeline.tid_to_op_map.find(op.tid);
+  map<ceph_tid_t, std::unique_ptr<Op>>::iterator i = rmw_pipeline.tid_to_op_map.find(op.tid);


Op::ref or Ref?

Switched to OpRef.

athanatos · 2023-06-30T02:46:20Z

src/osd/ECBackend.cc

@@ -1557,7 +1593,10 @@ void ECBackend::submit_transaction(
  )
 {
  ceph_assert(!rmw_pipeline.tid_to_op_map.count(tid));
-  Op *op = &(rmw_pipeline.tid_to_op_map[tid]);
+  auto concete_op = std::make_unique<ECClassicalOp>();


athanatos · 2023-06-30T02:46:41Z

src/osd/ECBackend.cc

+      std::set<hobject_t> *temp_added,
+      std::set<hobject_t> *temp_removed,
+      DoutPrefixProvider *dpp,
+      const ceph_release_t require_osd_release) override


athanatos · 2023-06-30T02:47:26Z

src/osd/ECBackend.cc

+      DoutPrefixProvider *dpp,
+      const ceph_release_t require_osd_release) override
+  {
+    if (t) {


Can t actually be null?

Nope, it's already dereferencec without extra checks.

athanatos · 2023-06-30T02:52:02Z

src/osd/ECBackend.cc

@@ -1572,8 +1611,23 @@ void ECBackend::submit_transaction(
  if (client_op) {
    op->trace = client_op->pg_trace;
  }
+  op->plan = ECTransaction::get_write_plan(


For symmetry, seems like get_write_plan should also be a an ECBackend::Op method.

Added a static wrapper.

athanatos · 2023-06-30T02:54:01Z

src/osd/ECTransaction.h

    return plan;
  }

  void generate_transactions(
+    PGTransaction* _t,


We assert non-null, probably better to pass as a reference?

Added a new commit for that.

athanatos · 2023-06-30T02:55:42Z

src/osd/ECBackend.cc

-      get_osdmap()->require_osd_release);
-  }
+  op->generate_transactions(
+    op->plan,


We can probably remove the members from the param list?

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

rzarzynski · 2023-07-06T19:51:58Z

@athanatos: would you mind taking another look?

rzarzynski · 2023-07-06T19:58:40Z

src/osd/ECBackend.cc

+    return ECTransaction::get_write_plan(
+      sinfo,
+      t,
+      std::move(get_hinfo),


std::forward.

rzarzynski · 2023-07-07T11:24:22Z

src/osd/ECBackend.cc

@@ -1556,22 +1556,21 @@ struct ECClassicalOp : ECBackend::RMWPipeline::Op {
      DoutPrefixProvider *dpp,
      const ceph_release_t require_osd_release) final
  {
-    if (t) {


Oops, I think I discovered where the t-empty ops are born:

bool ECBackend::RMWPipeline::try_finish_rmw() { // ... if (get_osdmap()->require_osd_release >= ceph_release_t::kraken) { if (op->version > get_parent()->get_log().get_can_rollback_to() && waiting_reads.empty() && waiting_commit.empty()) { // submit a dummy transaction to kick the rollforward auto tid = get_parent()->get_tid(); Op *nop = &(tid_to_op_map[tid]); nop->hoid = op->hoid; nop->trim_to = op->trim_to; nop->roll_forward_to = op->version; nop->tid = tid; nop->reqid = op->reqid; waiting_reads.push_back(*nop); }

Ah, I see. Ok, add a comment to that effect so that future readers won't be confused.

1. `WritePlan` doesn't hold `PGTransactionUPtr` anymore. 2. `Op` (soon `RMWPipeline::Op`) gets dynamically polymorphic `generate_transaction()`. The classical OSD implements this interface with `ECClassicalOp` which hosts `PGTransaction`. Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

The intention behind this refactor is to emphasize that: 1. callers of `start_rmw()` are supposed to create any concrete type of `RMWPipeline::Op`. 2. `RMWPipeline` operates solely on the abstract `Op` and control its life-time. Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

rzarzynski · 2023-08-21T12:00:34Z

jenkins test api

ljflores · 2023-09-08T22:10:50Z

Rados approved: https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrellocomcJxeRJYse1822-wip-yuri4-testing-2023-08-10-1739

rzarzynski requested a review from a team as a code owner June 27, 2023 14:33

github-actions bot added the core label Jun 27, 2023

rzarzynski added the crimson label Jun 27, 2023

rzarzynski requested a review from athanatos June 29, 2023 20:16

athanatos requested changes Jun 30, 2023

View reviewed changes

osd: dissect the EC RMW pipeline from ECBackend into dedicated class

0a3d559

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

rzarzynski force-pushed the wip-crimson-ec-classical-refactor branch from a14ca6a to c5fbf12 Compare July 6, 2023 19:49

rzarzynski requested a review from athanatos July 6, 2023 19:51

rzarzynski commented Jul 6, 2023

View reviewed changes

rzarzynski force-pushed the wip-crimson-ec-classical-refactor branch from c5fbf12 to 7f44047 Compare July 6, 2023 20:04

athanatos approved these changes Jul 6, 2023

View reviewed changes

rzarzynski commented Jul 7, 2023

View reviewed changes

rzarzynski added 3 commits July 7, 2023 11:26

osd: move ECBackend::Op into RMWPipeline::Op

22d2d4f

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

rzarzynski force-pushed the wip-crimson-ec-classical-refactor branch from 7f44047 to 49db2d9 Compare July 7, 2023 11:27

github-actions bot added the tests label Jul 7, 2023

rzarzynski added the needs-qa label Jul 13, 2023

Matan-B mentioned this pull request Jul 13, 2023

osd: decouple recovery from client reads in ECBackend, establish ReadPipeline #52340

Closed

14 tasks

ljflores added the wip-yuri4-testing label Aug 10, 2023

ljflores merged commit 81c209d into ceph:main Sep 8, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

osd: dissect and abstract RMWPipeline from ECBackend for sharing it with crimson #52211

osd: dissect and abstract RMWPipeline from ECBackend for sharing it with crimson #52211

rzarzynski commented Jun 27, 2023

athanatos left a comment

athanatos Jun 30, 2023

rzarzynski Jul 6, 2023

athanatos Jun 30, 2023

rzarzynski Jun 30, 2023

rzarzynski Jul 6, 2023

athanatos Jun 30, 2023

rzarzynski Jul 6, 2023

athanatos Jun 30, 2023

rzarzynski Jul 6, 2023

athanatos Jun 30, 2023

rzarzynski Jul 6, 2023

athanatos Jun 30, 2023

rzarzynski Jul 6, 2023

athanatos Jun 30, 2023

rzarzynski Jun 30, 2023

rzarzynski Jul 6, 2023

athanatos Jun 30, 2023

rzarzynski Jul 6, 2023

athanatos Jun 30, 2023

rzarzynski Jul 6, 2023

rzarzynski commented Jul 6, 2023

rzarzynski Jul 6, 2023

rzarzynski Jul 6, 2023

rzarzynski Jul 7, 2023

athanatos Jul 7, 2023

rzarzynski commented Aug 21, 2023

ljflores commented Sep 8, 2023

osd: dissect and abstract RMWPipeline from ECBackend for sharing it with crimson #52211

osd: dissect and abstract RMWPipeline from ECBackend for sharing it with crimson #52211

Conversation

rzarzynski commented Jun 27, 2023

Contribution Guidelines

Checklist

athanatos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzarzynski commented Jul 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzarzynski commented Aug 21, 2023

ljflores commented Sep 8, 2023