Skip to content

crimson: osd_operation cleanups and fix for MOSDRepOpReply ordering#62836

Merged
athanatos merged 11 commits intoceph:mainfrom
athanatos:sjust/wip-crimson-repop-reply-ordering-69439
Apr 29, 2025
Merged

crimson: osd_operation cleanups and fix for MOSDRepOpReply ordering#62836
athanatos merged 11 commits intoceph:mainfrom
athanatos:sjust/wip-crimson-repop-reply-ordering-69439

Conversation

@athanatos
Copy link
Contributor

@athanatos athanatos commented Apr 15, 2025

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)
Show available Jenkins commands

@athanatos athanatos requested review from a team as code owners April 15, 2025 23:13
@athanatos athanatos marked this pull request as draft April 15, 2025 23:13
@athanatos
Copy link
Contributor Author

athanatos commented Apr 15, 2025

waiting on cleaner main to test, DNM

@athanatos
Copy link
Contributor Author

athanatos commented Apr 15, 2025

Should wait on #62619

@Matan-B Matan-B added this to Crimson Apr 21, 2025
@Matan-B Matan-B moved this to Awaits review in Crimson Apr 21, 2025
@athanatos athanatos force-pushed the sjust/wip-crimson-repop-reply-ordering-69439 branch from 69c3edd to 304de12 Compare April 21, 2025 16:32
@github-actions
Copy link

github-actions bot commented Apr 22, 2025

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

@athanatos
crimson: handle_rep_op_reply downgrade stale message to debug
d5f6d03 
This isn't actually unusual or alarming.

Signed-off-by: Samuel Just <sjust@redhat.com>
@athanatos
crimson: finish_decode() of MOSDRepOp on target core
bd95adc 
Signed-off-by: Samuel Just <sjust@redhat.com>
@athanatos
crimson/.../client_request_common: convert do_recover_missing to use …
05230fc 
…log macros

Signed-off-by: Samuel Just <sjust@redhat.com>
@athanatos
crimson/.../client_request_common: convert do_recover_missing to use …
a85888f 
…coroutine

Signed-off-by: Samuel Just <sjust@redhat.com>
@athanatos
crimson: remove CommonClientRequest, move do_recover_missing to PG
ee11626 
do_recover_missing was the only thing left, and inheriting from a class
to get a static method is somewhat confusing.  Simply move
do_recover_missing to PG.

Signed-off-by: Samuel Just <sjust@redhat.com>
@athanatos athanatos force-pushed the sjust/wip-crimson-repop-reply-ordering-69439 branch from 304de12 to 00d49dd Compare April 25, 2025 03:51
@athanatos athanatos force-pushed the sjust/wip-crimson-repop-reply-ordering-69439 branch from 00d49dd to e2394ff Compare April 25, 2025 17:04
@athanatos
crimson/.../pg_shard_manager: add start_pg_operation_active
1a75da4 
Messages between OSDs for PGs that have already completed peering
require fewer checks than otherwise.

Signed-off-by: Samuel Just <sjust@redhat.com>
@athanatos
crimson/.../osd.cc: convert active ops to start_pg_operation_active
607c773 
Signed-off-by: Samuel Just <sjust@redhat.com>
@athanatos
crimson/.../osd_operation.h: add RemoteOperation
6da6b0f 
Subsequent commits will switch various ops to inherit from
this thereby removing some boilerplate.

Signed-off-by: Samuel Just <sjust@redhat.com>
@athanatos
crimson: fix DynamicPerfStats usage in ClientRequest
794bb02 
ClientRequest::get_connection() return l_conn, which will be
null by the time PG::add_client_request_lat is called in
ClientRequest::do_process.  Modify get_connection() to
return a Connection& from whichever of l_conn or r_conn
isn't null.

Signed-off-by: Samuel Just <sjust@redhat.com>
@athanatos
crimson: convert cross-core operations to use RemoteOperation
b031373 
Signed-off-by: Samuel Just <sjust@redhat.com>
@athanatos athanatos force-pushed the sjust/wip-crimson-repop-reply-ordering-69439 branch from e2394ff to f9baafe Compare April 28, 2025 21:20
@athanatos
Copy link
Contributor Author

athanatos commented Apr 28, 2025

sjust-2025-04-25_20:38:04-crimson-rados-wip-sjust-crimson-testing-2025-04-25-1745601451-distro-default-gibba

50/51 pass, single failure 8259693 seems to be https://tracker.ceph.com/issues/70502

@athanatos
Copy link
Contributor Author

athanatos commented Apr 28, 2025 •
edited
Loading

In my test environment, this branch does not seem to negatively impact performance (seems to be a very slight improvement).

@athanatos
crimson: add operation wrapper for MOSDRepOpReply
0c15eb5 
This should avoid reordering between cores.

Fixes: https://tracker.ceph.com/issues/69439
Signed-off-by: Samuel Just <sjust@redhat.com>
@athanatos athanatos force-pushed the sjust/wip-crimson-repop-reply-ordering-69439 branch from f9baafe to 0c15eb5 Compare April 28, 2025 23:09
@athanatos athanatos marked this pull request as ready for review April 29, 2025 00:08
@athanatos athanatos requested a review from Matan-B April 29, 2025 00:08
Matan-B
Matan-B reviewed Apr 29, 2025
View reviewed changes
Copy link
Contributor

@Matan-B Matan-B left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! Only some questions before merging

@athanatos
Copy link
Contributor Author

athanatos commented Apr 29, 2025

jenkins test make check

1 similar comment
@athanatos
Copy link
Contributor Author

athanatos commented Apr 29, 2025

jenkins test make check

@Matan-B Matan-B moved this from Awaits review to Needs QA in Crimson Apr 29, 2025
@Matan-B
Copy link
Contributor

Matan-B commented Apr 29, 2025

jenkins test api

2 similar comments
@athanatos
Copy link
Contributor Author

athanatos commented Apr 29, 2025

jenkins test api

@athanatos
Copy link
Contributor Author

athanatos commented Apr 29, 2025

jenkins test api

@athanatos athanatos merged commit 76dcf47 into ceph:main Apr 29, 2025
12 checks passed
@Matan-B Matan-B moved this from Needs QA to Merged in Crimson Apr 30, 2025
@Matan-B Matan-B mentioned this pull request Apr 30, 2025
Merged
@Matan-B Matan-B moved this from Merged Pre Tentacle Freeze to Merged (Main) in Crimson May 7, 2025
@Matan-B Matan-B mentioned this pull request Jul 27, 2025
Merged
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Matan-B Matan-B Matan-B approved these changes

Assignees

No one assigned

Labels

build/ops core crimson backport tentacle crimson

Projects

Crimson
Status: Merged (Main)

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@athanatos @Matan-B