osd: Access/Modify epoch maps under mutex in OSDSuperblock class by mohit84 · Pull Request #62916 · ceph/ceph

mohit84 · 2025-04-22T15:16:21Z

The OSDSuperblock object access/modify epoch maps in multiple threads simultaneously due to that OSD is getting crashed. To avoid the crash access the maps under mutex.

Fixes: https://tracker.ceph.com/issues/66819

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 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

mohit84 · 2025-04-22T15:19:27Z

jenkins test make check

src/osd/osd_types.h

mohit84 · 2025-04-23T10:15:46Z

jenkins test make check

mohit84 · 2025-04-25T09:42:08Z

jenkins test make check

rzarzynski · 2025-05-06T19:29:47Z

src/osd/osd_types.h


 class OSDSuperblock {
+private:
+  mutable std::shared_ptr<std::mutex> map_lock = std::make_shared<std::mutex>();


shared_ptr makes me worried about copy-constructing. Have you tried unique_ptr?
Do we ever copy-construct OSDSuperblock?

Yes i have tried there are multiple places where we do copy the object.

Huh, this aggregates the critical sections between all these instances.

Move the maps to a new class MapContainer and that class is using mutex to get/set the maps.

mohit84 · 2025-05-19T12:53:30Z

jenkins test make check

rzarzynski

Basically LGTM! Just some nits.

rzarzynski · 2025-05-30T20:05:49Z

src/osd/osd_types.h

+  interval_set<epoch_t> maps;

-  epoch_t get_oldest_map() const {
+  epoch_t _get_newest_map() const {


nit: these two methods got flapped got flapped which makes reading the diff a bit harder. Not a blocker, though.

rzarzynski · 2025-05-30T20:06:57Z

src/osd/osd_types.h

+public:
+  MapContainer() = default;
+  MapContainer(const MapContainer& other)
+    : map_lock(ceph::make_mutex("map_lock"))


Hmm, is not giving the name for default-constructed instances intentional?

I thought it is always recommended to use default instead of declaring empty constructor so i use default, please let me know if you want to change it to empty constructor something like MapContainer () {}.

Sorry, I'm taking my previous comment back. There is the initializer:

mutable ceph::mutex map_lock = ceph::make_mutex("map_lock");

rzarzynski · 2025-05-30T20:09:46Z

src/osd/osd_types.h

-  int32_t whoami = -1;    // my role in this fs.
-  epoch_t current_epoch = 0;             // most recent epoch
-  interval_set<epoch_t> maps; // oldest/newest maps we have.
+class MapContainer {


nit: MapContainer is a quite general name while the entity is very closely related to OSDSuperblock. How about renaming to OSDSuperblockGuardedMap? Even better clarity might achieved with a nested class (if possible): OSDSuperblock::GuardedMap.

mohit84 · 2025-06-02T07:45:09Z

jenkins test make check

rzarzynski · 2025-06-02T11:48:27Z

There is an FTBFS when building the file for crimson:

In file included from /home/jenkins-build/build/workspace/ceph-pull-requests/src/crush/CrushWrapper.cc:7:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/osd/osd_types.h:5679:17: error: no type named 'mutex' in namespace 'ceph'
 5679 |   mutable ceph::mutex map_lock = ceph::make_mutex("map_lock");
      |           ~~~~~~^
/home/jenkins-build/build/workspace/ceph-pull-requests/src/osd/osd_types.h:5698:22: error: no member named 'make_mutex' in namespace 'ceph'
 5698 |     : map_lock(ceph::make_mutex("map_lock"))
      |                ~~~~~~^
2 errors generated.
[763/2885] Building CXX object src/crimson/CMakeFiles/crimson-common.dir/__/common/config.cc.o
FAILED: src/crimson/CMakeFiles/crimson-common.dir/__/common/config.cc.o

I think this is rather a trivial issue – most likely the header for make_mutex really hasn't been included; for the classical works because of being included indirectly, from another header.

mohit84 · 2025-06-02T14:06:42Z

There is an FTBFS when building the file for crimson:

In file included from /home/jenkins-build/build/workspace/ceph-pull-requests/src/crush/CrushWrapper.cc:7:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/osd/osd_types.h:5679:17: error: no type named 'mutex' in namespace 'ceph'
 5679 |   mutable ceph::mutex map_lock = ceph::make_mutex("map_lock");
      |           ~~~~~~^
/home/jenkins-build/build/workspace/ceph-pull-requests/src/osd/osd_types.h:5698:22: error: no member named 'make_mutex' in namespace 'ceph'
 5698 |     : map_lock(ceph::make_mutex("map_lock"))
      |                ~~~~~~^
2 errors generated.
[763/2885] Building CXX object src/crimson/CMakeFiles/crimson-common.dir/__/common/config.cc.o
FAILED: src/crimson/CMakeFiles/crimson-common.dir/__/common/config.cc.o

I think this is rather a trivial issue – most likely the header for make_mutex really hasn't been included; for the classical works because of being included indirectly, from another header.

done

mohit84 · 2025-06-02T14:06:52Z

jenkins test make check

Matan-B · 2025-06-03T09:11:06Z

src/osd/osd_types.h

 #include "common/hobject.h"
 #include "common/snap_types.h"
 #include "common/strtol.h" // for ritoa()
+#include "common/ceph_mutex.h" //To use make_mutex() for crimson


nit: I think that the comment is confusing and should be removed. make_mutex is needed for both.

Classic was ok without it because of it being included indirectly (implicitly).

The OSDSuperblock object access/modify epoch maps in multiple threads simultaneously due to that OSD is getting crashed. To avoid the crash access the maps under mutex. Fixes: https://tracker.ceph.com/issues/66819 Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>

mohit84 · 2025-06-03T12:17:07Z

jenkins test make check

rzarzynski · 2025-06-12T17:47:33Z

src/osd/osd_types.h

+public:
+  MapContainer() = default;
+  MapContainer(const MapContainer& other)
+    : map_lock(ceph::make_mutex("map_lock"))


Sorry, I'm taking my previous comment back. There is the initializer:

mutable ceph::mutex map_lock = ceph::make_mutex("map_lock");

NitzanMordhai · 2025-06-23T09:30:28Z

Rados approved: https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrackercephcomissues71712

backport tracker: https://tracker.ceph.com/issues/72070 backport of ceph#62916 parent tracker: https://tracker.ceph.com/issues/66819 Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>

backport tracker: https://tracker.ceph.com/issues/72070 backport of ceph#62916 parent tracker: https://tracker.ceph.com/issues/66819 (cherry picked from commit 8cf896e) Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>

backport tracker: https://tracker.ceph.com/issues/72070 backport of ceph#62916 parent tracker: https://tracker.ceph.com/issues/66819 (cherry picked from commit 8cf896e) Signed-off-by: Mohit Agrawal <moagrawa@redhat.com> (cherry picked from commit 1fdf9cd) Resolves: rhbz#2417015

backport tracker: https://tracker.ceph.com/issues/72070 backport of ceph#62916 parent tracker: https://tracker.ceph.com/issues/66819 (cherry picked from commit 8cf896e) Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>

mohit84 requested a review from a team as a code owner April 22, 2025 15:16

mohit84 requested a review from rzarzynski April 22, 2025 15:16

github-actions bot added the core label Apr 22, 2025

bill-scales reviewed Apr 23, 2025

View reviewed changes

src/osd/osd_types.h Outdated Show resolved Hide resolved

mohit84 force-pushed the tick_without_osd_crash branch from 25f8c8a to 289631e Compare April 23, 2025 10:14

mohit84 requested a review from a team as a code owner April 23, 2025 10:14

github-actions bot added the crimson label Apr 23, 2025

bill-scales approved these changes Apr 23, 2025

View reviewed changes

rzarzynski reviewed May 6, 2025

View reviewed changes

mohit84 force-pushed the tick_without_osd_crash branch from 289631e to cd06003 Compare May 19, 2025 12:52

rzarzynski reviewed May 30, 2025

View reviewed changes

mohit84 force-pushed the tick_without_osd_crash branch from cd06003 to 844e94b Compare June 2, 2025 07:40

mohit84 force-pushed the tick_without_osd_crash branch from 844e94b to 29fe9f4 Compare June 2, 2025 14:06

Matan-B reviewed Jun 3, 2025

View reviewed changes

mohit84 force-pushed the tick_without_osd_crash branch from 29fe9f4 to 8cf896e Compare June 3, 2025 12:14

rzarzynski approved these changes Jun 12, 2025

View reviewed changes

rzarzynski added the needs-qa label Jun 16, 2025

SrinivasaBharath added the wip-bharath4-testing label Jun 17, 2025

NitzanMordhai merged commit c056c9f into ceph:main Jun 23, 2025
12 checks passed

SrinivasaBharath removed the wip-bharath4-testing label Jun 23, 2025

mohit84 mentioned this pull request Jul 10, 2025

tentacle: osd: Access/Modify epoch maps under mutex in OSDSuperblock class #64438

Merged

mohit84 mentioned this pull request Jul 29, 2025

squid: osd: Access/Modify epoch maps under mutex in OSDSuperblock class #64732

Merged

14 tasks

Conversation

mohit84 commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

mohit84 commented Apr 22, 2025

Uh oh!

Uh oh!

mohit84 commented Apr 23, 2025

Uh oh!

mohit84 commented Apr 25, 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!

mohit84 commented May 19, 2025

Uh oh!

rzarzynski 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohit84 commented Jun 2, 2025

Uh oh!

rzarzynski commented Jun 2, 2025

Uh oh!

mohit84 commented Jun 2, 2025

Uh oh!

mohit84 commented Jun 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohit84 commented Jun 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NitzanMordhai commented Jun 23, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mohit84 commented Apr 22, 2025 •

edited

Loading