mgr, mon, osdc: pass complex parameters by rvalue reference by MaxKellermann · Pull Request #65014 · ceph/ceph

MaxKellermann · 2025-08-13T13:24:33Z

A non-trivial type is always passed by reference, even if the parameter is declared as a value; in that case, the compiler must allocate stack space and move the value there, and then really passes a reference to this stack allocation. This only adds overhead for no reason.

A side effect of this API change is that all callers that accidently created temporary copies are now revealed by throwing a compiler error. This commit fixes all callers and reduces a good amount of useless overhead by wrapping those parameters in std::move() instead of copying temporaries.

More temporaries are avoided by emplacing the std::string&& into the std::vector.

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 test classic perf Jenkins Job | Jenkins Job Definition
jenkins test crimson perf Jenkins Job | Jenkins Job Definition
jenkins test signed Jenkins Job | Jenkins Job Definition
jenkins test make check Jenkins Job | Jenkins Job Definition
jenkins test make check arm64 Jenkins Job | Jenkins Job Definition
jenkins test submodules Jenkins Job | Jenkins Job Definition
jenkins test dashboard Jenkins Job | Jenkins Job Definition
jenkins test dashboard cephadm Jenkins Job | Jenkins Job Definition
jenkins test api Jenkins Job | Jenkins Job Definition
jenkins test docs ReadTheDocs | Github Workflow Definition
jenkins test ceph-volume all Jenkins Jobs | Jenkins Jobs Definition
jenkins test windows Jenkins Job | Jenkins Job Definition
jenkins test rook e2e Jenkins Job | Jenkins Job Definition

MaxKellermann · 2025-08-13T20:01:39Z

jenkins test make check arm64

MaxKellermann · 2025-08-14T05:36:47Z

jenkins test make check arm64

MaxKellermann · 2025-08-14T08:31:45Z

arm64 keeps crashing with SIGBUS, but I don't think that's related.... so I keep repeating the test run! Sigh.

MaxKellermann · 2025-08-14T08:31:47Z

jenkins test make check arm64

MaxKellermann · 2025-08-14T11:08:51Z

jenkins test make check arm64

MaxKellermann · 2025-08-14T12:14:41Z

so I keep repeating the test run!

That strategy has worked, yet again.

github-actions · 2025-08-21T09:46:00Z

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

ronen-fr · 2025-08-28T10:58:16Z

src/common/mclock_common.cc

          dout(10) << __func__ << " Removing Key: " << *key
                   << " for " << osd << " from Mon db" << dendl;
-                   monc->start_mon_command(vcmd, {}, nullptr, nullptr, nullptr);
+	  monc->start_mon_command({std::move(cmd)}, {}, nullptr, nullptr, nullptr);


Aren't you changing more than the value category here (i.e. by moving from a vector to a single string)?

I don't understand. What do you think is changed here? What is a "value category"?

(value category: lvalue, rvalue, etc.)
I was ref'ing the change that replaced the vector of strings with one string (which seemed to be unrelated)

And - a general question: as you are changing some 'const A&' to 'A&': did you run a static analyzer to verify that no 'use after move' instances were created?

that replaced the vector of strings with one string (which seemed to be unrelated)

I did not replace a vector or strings with one string. Maybe you missed the curly braces?

And - a general question: as you are changing some 'const A&' to 'A&': did you run a static analyzer to verify that no 'use after move' instances were created?

No, I don't have a static analyzer. But I did not change const A& to A&, but const A& to A&&.

No, I don't have a static analyzer. But I did not change const A& to A&, but const A& to A&&.

Sure - that's what I meant. Sorry. The question is this: the original signature of a function such as that had guaranteed that the argument won't be changed. Now - it was moved-from. Are you sure there are no cases where the client did use the argument? I tried to follow some of the cases, but a static analyzer is better at that.

I did not replace a vector or strings with one string. Maybe you missed the curly braces?

Yes - I did miss that, but now I wonder what will happen there. Going to check.

Are you sure there are no cases where the client did use the argument?

I checked all changes (by code review, not by running them - I don't have any Ceph installation that I can use to test this), and I'm confident that they are correct. Minus human mistakes, as always.

(I assume you have this working, but - if you need help running a vstart configuration - let me know)

src/rgw/driver/rados/rgw_tools.cc

ronen-fr · 2025-08-28T11:48:09Z

a final comment for now: as it is now, this PR spans 3 teams, which might make its approval into a lengthy process.

MaxKellermann · 2025-08-28T11:53:24Z

a final comment for now: as it is now, this PR spans 3 teams, which might make its approval into a lengthy process.

Everything with Ceph is a lengthy process! At the current rate, it will take a decade or two until all my finished patches are merged. I have hundreds on my stgit stack, only waiting for dependent PRs to get merged.

github-actions · 2025-09-08T11:34:06Z

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

MaxKellermann · 2025-09-18T07:08:29Z

Thanks @anrao19 - there are now conflicts with newly merged commits. I can rebase & force-push - do you want me to?

adamemerson · 2025-09-19T18:56:33Z

want me to

Yes, please.

adamemerson · 2025-09-19T18:57:22Z

@ljflores Would you be willing to have someone schedule RADOS QA?

MaxKellermann · 2025-09-19T19:52:30Z

Yes, please.

done

github-actions · 2025-09-26T18:01:38Z

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

MaxKellermann · 2025-09-27T21:43:13Z

Rebased again to fix conflict with newly merged PRs.

MaxKellermann · 2025-09-28T18:14:54Z

jenkins test make check arm64

github-actions · 2025-09-29T09:26:11Z

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

A non-trivial type is always passed by reference, even if the parameter is declared as a value; in that case, the compiler must allocate stack space and move the value there, and then really passes a reference to this stack allocation. This only adds overhead for no reason. A side effect of this API change is that all callers that accidently created temporary copies are now revealed by throwing a compiler error. This commit fixes all callers and reduces a good amount of useless overhead by wrapping those parameters in std::move() instead of copying temporaries. More temporaries are avoided by emplacing the std::string&& into the std::vector. Signed-off-by: Max Kellermann <max.kellermann@ionos.com>

anrao19 · 2025-10-30T07:31:09Z

RGW PR testing completed and approved by @ivancich. Details : https://tracker.ceph.com/issues/73623

… rvalue refs PR ceph#65014 added std::move() inside the while loop, which required recreating the command string on each iteration. Add const std::string& overload that forwards to the existing std::string&& version, allowing the command to be declared outside the loop for better efficiency. Fixes: https://tracker.ceph.com/issues/73838 Signed-off-by: Nitzan Mordechai <nmordech@ibm.com>

… rvalue refs PR ceph#65014 added std::move() inside the while loop, which required recreating the command string on each iteration. Add const std::string& overload that forwards to the existing std::string&& version, allowing the command to be declared outside the loop for better efficiency. Fixes: https://tracker.ceph.com/issues/73838 Signed-off-by: Nitzan Mordechai <nmordech@ibm.com> (cherry picked from commit 3e107ec)

… rvalue refs PR ceph#65014 added std::move() inside the while loop, which required recreating the command string on each iteration. Add const std::string& overload that forwards to the existing std::string&& version, allowing the command to be declared outside the loop for better efficiency. Fixes: https://tracker.ceph.com/issues/73838 Signed-off-by: Nitzan Mordechai <nmordech@ibm.com>

MaxKellermann requested review from a team as code owners August 13, 2025 13:24

github-actions bot added cephfs Ceph File System common core mgr mon rbd rgw tests labels Aug 13, 2025

adamemerson self-assigned this Aug 15, 2025

github-actions bot added the needs-rebase label Aug 21, 2025

MaxKellermann force-pushed the mgr_mon_osdc__references branch from 070663f to 3c4512f Compare August 28, 2025 10:41

github-actions bot removed the needs-rebase label Aug 28, 2025

ronen-fr reviewed Aug 28, 2025

View reviewed changes

src/rgw/driver/rados/rgw_tools.cc Show resolved Hide resolved

ronen-fr requested review from adamemerson, badone and rzarzynski August 28, 2025 13:03

anrao19 added TESTED and removed wip-anrao5-testing labels Sep 18, 2025

MaxKellermann force-pushed the mgr_mon_osdc__references branch from 5203141 to e3cbd0a Compare September 19, 2025 19:52

github-actions bot added needs-rebase and removed needs-rebase labels Sep 19, 2025

MaxKellermann force-pushed the mgr_mon_osdc__references branch from e3cbd0a to 419231b Compare September 27, 2025 21:42

github-actions bot removed the needs-rebase label Sep 27, 2025

github-actions bot added the needs-rebase label Sep 29, 2025

MaxKellermann force-pushed the mgr_mon_osdc__references branch from 419231b to f82ab1c Compare September 29, 2025 11:10

github-actions bot removed the needs-rebase label Sep 29, 2025

anrao19 added the wip-anrao1-testing label Oct 22, 2025

anrao19 removed the wip-anrao1-testing label Oct 30, 2025

MaxKellermann merged commit a9b8933 into ceph:main Nov 10, 2025
13 checks passed

MaxKellermann deleted the mgr_mon_osdc__references branch November 10, 2025 17:49

NitzanMordhai mentioned this pull request Nov 17, 2025

test/lazy-omap-stats: overload get_output() to accept both lvalue and… #66277

Merged

14 tasks

Conversation

MaxKellermann commented Aug 13, 2025

Checklist

Uh oh!

MaxKellermann commented Aug 13, 2025

Uh oh!

MaxKellermann commented Aug 14, 2025

Uh oh!

MaxKellermann commented Aug 14, 2025

Uh oh!

MaxKellermann commented Aug 14, 2025

Uh oh!

MaxKellermann commented Aug 14, 2025

Uh oh!

MaxKellermann commented Aug 14, 2025

Uh oh!

github-actions bot commented Aug 21, 2025

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!

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!

Uh oh!

ronen-fr commented Aug 28, 2025

Uh oh!

MaxKellermann commented Aug 28, 2025

Uh oh!

github-actions bot commented Sep 8, 2025

Uh oh!

MaxKellermann commented Sep 18, 2025

Uh oh!

adamemerson commented Sep 19, 2025

Uh oh!

adamemerson commented Sep 19, 2025

Uh oh!

MaxKellermann commented Sep 19, 2025

Uh oh!

github-actions bot commented Sep 26, 2025

Uh oh!

MaxKellermann commented Sep 27, 2025

Uh oh!

MaxKellermann commented Sep 28, 2025

Uh oh!

github-actions bot commented Sep 29, 2025

Uh oh!

anrao19 commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anrao19 commented Oct 30, 2025 •

edited

Loading