crimson/os/pg_map: allow multiple shards to create new pg mappings at the same time by cyx1231st · Pull Request #56332 · ceph/ceph

cyx1231st · 2024-03-20T08:02:45Z

Also:

Better detections in case of inconsistent racings, such as:
- The new mapping is creating towards different cores.
- Mapping creation is racing with its eracing.
- Multiple shards are eracing the same mapping at the same time.
Add more logs to debug in case of unexpected issues.

Fixes: https://tracker.ceph.com/issues/64934
Fixes: https://tracker.ceph.com/issues/64009

Signed-off-by: Yingxin Cheng yingxin.cheng@intel.com

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. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

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
jenkins test rook e2e

Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>

cyx1231st · 2024-03-20T08:03:58Z

Need/going to validate it in sepia.

cyx1231st · 2024-03-22T01:21:27Z

Doesn't see the issue in the tests.

https://pulpito.ceph.com/yingxin-2024-03-21_05:50:24-crimson-rados-ci-yingxin-fix-racing-get-or-create-pg-mapping-2-distro-default-smithi/

7614699, 7614702: https://tracker.ceph.com/issues/62162
The others are scrub failures

athanatos · 2024-03-22T22:08:22Z

src/crimson/osd/pg_map.cc

+    DEBUG("calling primary to add mapping for pg {} to the expected core {}",
+          pgid, core_expected);
+    return container().invoke_on(
+        0, [pgid, _core_expected=core_expected, FNAME](auto &primary_mapping) {


[pgid, core_expected, ...] should just work?

I wanted to update core_expected but this lambda doesn't worth to become mutable.

Refer to the following logic:

auto core_expected = _core_expected; ... core_expected = core_found; ... assert(core_expected != NULL_CORE);

Probably I can rename to make this cleaner.

Preserved core_expected and renamed to use core_to_update to store the updates.

athanatos · 2024-03-22T22:10:10Z

jenkins test windows

liu-chunmei · 2024-03-24T05:12:02Z

@cyx1231st it is also create pg on shard 0 and broadcast to others, why you name the commit : allow multiple shards to create new pg mappings at the same time

Matan-B

nit, commit message typo: s/eracing/erasing/

Can you please separate the actual fix from the additional asserts/logs added? If not, perhaps a comment which emphasizes where the actual concurrent new mapping occur.

Matan-B · 2024-03-24T13:06:41Z

src/crimson/osd/pg_map.cc

+        auto [insert_iter, inserted] =
+          primary_mapping.pg_to_core.emplace(pgid, core_expected);
+        assert(inserted);


The fix here is to emplace the new mapping only if it wasn't already mapped, right? (under find_iter == primary_mapping.pg_to_core.end())

@cyx1231st it is also create pg on shard 0 and broadcast to others, why you name the commit : allow multiple shards to create new pg mappings at the same time

@liu-chunmei

Right, because primary_mapping.core_to_num_pgs also needs to keep correct.

Matan-B · 2024-03-24T13:06:44Z

src/crimson/osd/pg_map.cc

+        0, [pgid, _core_expected=core_expected, FNAME](auto &primary_mapping) {
+      auto core_expected = _core_expected;
+      auto find_iter = primary_mapping.pg_to_core.find(pgid);
+      if (find_iter != primary_mapping.pg_to_core.end()) {


// this pgid was already mapped within primary_mapping, assert that the // mapping is consistent and avoid emplacing once again. if (find_iter != primary_mapping.pg_to_core.end()) {

cyx1231st · 2024-03-25T01:36:03Z

Can you please separate the actual fix from the additional asserts/logs added? If not, perhaps a comment which emphasizes where the actual concurrent new mapping occur.

This fix allows concurrent and consistent updates, meaning that it needs to prevent/assert the inconcistent updates at the same time. So I don't feel that they are splitable.

I'll add more comments.

cyx1231st · 2024-03-25T01:38:03Z

@cyx1231st it is also create pg on shard 0 and broadcast to others, why you name the commit : allow multiple shards to create new pg mappings at the same time.

This PR is allowing multiple shards to create new and consistent pg mappings at the same time, and the design needs these creation to be aligned in shard 0.

cyx1231st · 2024-03-25T06:41:20Z

Changeset: minor renames and added some comments.

I'll proceed to merge if no further concerns.

… the same time Also: * Better detections in case of inconsistent racings, such as: * The new mapping is creating towards different cores. * Mapping creation is racing with its eracing. * Multiple shards are erasing the same mapping at the same time. * Add more logs to debug in case of unexpected issues. Fixes: https://tracker.ceph.com/issues/64934 Fixes: https://tracker.ceph.com/issues/64009 Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>

cyx1231st · 2024-03-26T03:40:00Z

Changeset: minor renames and added some comments.

I suppose there is no impact to the logic already implemented, merging based on the previous test results.

Matan-B · 2024-03-26T08:42:48Z

Doesn't see the issue in the tests.

https://pulpito.ceph.com/yingxin-2024-03-21_05:50:24-crimson-rados-ci-yingxin-fix-racing-get-or-create-pg-mapping-2-distro-default-smithi/

7614699, 7614702: https://tracker.ceph.com/issues/62162

The others are scrub failures

Can you please attach the failed runs to the tracker next time? This may help with investigating :)

cyx1231st · 2024-03-26T09:06:52Z

@Matan-B Sure! Thanks for updating the tracker.

cyx1231st added 2 commits March 19, 2024 11:05

crimson/osd/pg_map/PGShardMapping: cleanups

b35ed71

Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>

crimson/osd/shard_services: fix log

de2e667

Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>

cyx1231st requested review from Matan-B and athanatos March 20, 2024 08:02

github-actions bot added the crimson label Mar 20, 2024

cyx1231st added the bug-fix label Mar 20, 2024

cyx1231st marked this pull request as ready for review March 22, 2024 01:18

cyx1231st requested a review from a team as a code owner March 22, 2024 01:18

athanatos approved these changes Mar 22, 2024

View reviewed changes

Matan-B reviewed Mar 24, 2024

View reviewed changes

cyx1231st added the TESTED label Mar 25, 2024

cyx1231st force-pushed the wip-fix-racing-get-or-create-pg-mapping branch from a43f07d to 895c7b9 Compare March 25, 2024 06:38

cyx1231st force-pushed the wip-fix-racing-get-or-create-pg-mapping branch from 895c7b9 to 2422817 Compare March 25, 2024 06:42

Matan-B approved these changes Mar 25, 2024

View reviewed changes

cyx1231st merged commit 9555086 into ceph:main Mar 26, 2024

cyx1231st deleted the wip-fix-racing-get-or-create-pg-mapping branch March 26, 2024 03:41

Matan-B mentioned this pull request May 8, 2024

squid: crimson/os/pg_map: allow multiple shards to create new pg mappings at the same time #57348

Merged

Conversation

cyx1231st commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

cyx1231st commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyx1231st commented Mar 22, 2024

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!

athanatos commented Mar 22, 2024

Uh oh!

liu-chunmei commented Mar 24, 2024

Uh oh!

Matan-B left a comment

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!

cyx1231st commented Mar 25, 2024

Uh oh!

cyx1231st commented Mar 25, 2024

Uh oh!

cyx1231st commented Mar 25, 2024

Uh oh!

cyx1231st commented Mar 26, 2024

Uh oh!

Matan-B commented Mar 26, 2024

Uh oh!

cyx1231st commented Mar 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cyx1231st commented Mar 20, 2024 •

edited

Loading

cyx1231st commented Mar 20, 2024 •

edited

Loading