mds: use regular dispatch for processing beacons by batrick · Pull Request #57469 · ceph/ceph

batrick · 2024-05-15T01:45:52Z

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

src/messages/MMDSBeacon.h

leonid-s-usov

Is there a flow where the dispatchers are added dynamically, which would require a priority queue? Until now I was under the impression that dispatchers are added in a predictable static initialization flow, meaning that simply ordering the add_dispatcher_* calls should provide enough control over the invocation order.

If we have to go for the new priority argument, I think we should define standard high/default/low priorities, evenly spaced in the priority range. The default should be in the middle of the priority range, not on the lowest side.

src/msg/Messenger.h

batrick · 2024-05-15T14:28:19Z

Is there a flow where the dispatchers are added dynamically, which would require a priority queue? Until now I was under the impression that dispatchers are added in a predictable static initialization flow, meaning that simply ordering the add_dispatcher_* calls should provide enough control over the invocation order.

Dispatchers can be added dynamically but it is uncommon.

Adding dispatchers to the head/tail of the list is just too clumsy; there's no reason not to just define the order. It's rather important to get right because some dispatchers acquire highly contentious locks (mds_lock being one example).

If we have to go for the new priority argument, I think we should define standard high/default/low priorities, evenly spaced in the priority range. The default should be in the middle of the priority range, not on the lowest side.

Sure.

leonid-s-usov

OK then let's go with the priorities.

However, we should use the priority constants instead of literal magic numbers and try our best to leave calls unchanged where the default priority should apply.

src/mds/MDSDaemon.cc

src/mds/MDSRank.cc

src/mon/MonClient.cc

batrick · 2024-05-15T15:13:13Z

I've switched Messenger::(fast_)dispatchers to a std::vector from std::deque:

std::vector ensures the elements are contiguous
optimizing for add_dispatcher_head is not worthwhile when the dispatchers list is rarely changed and usually only at startup

leonid-s-usov · 2024-05-15T17:51:20Z

src/mds/MDSRank.cc

  dout(10) << __func__ << ": initializing metrics handler" << dendl;
  metrics_handler.init();
-  messenger->add_dispatcher_tail(&metrics_handler);
+  messenger->add_dispatcher_head(&metrics_handler, 20); // very high but behind Beacon


Still magic numbers. Now, one needs to go find where beacon handler is added and check that number. And if that number changes, how do they know to update this place?

If this should be high but behind beacon, this should be

messenger->add_dispatcher_tail(&metrics_handler, ::priority_high);

and beacon should stay

messenger->add_dispatcher_head(&beacon, ::priority_high);

For order checking. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

So we can ensure that e.g. MDSRank::ms_dispatch is lowest priority so that we do not acquire the mds_lock when looking at beacons. This change maintains the current behavior when the priority is unset: the use of std::stable_sort will ensure that the add_dispatcher_head and add_dispatcher_tail calls will preserve order when dispatcher priorities are equal. Fixes: 7fc04be Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

Similar to the issue with MClientMetrics, beacons should also not be handled via fast dispatch because it's necessary to acquire Beacon::mutex. This is a big no-no as it may block one of the Messenger threads leading to improbable deadlocks or DoS. Instead, use the normal dispatch where acquiring locks is okay to do. Fixes: 7fc04be See-also: linux.git/f7c2f4f6ce16fb58f7d024f3e1b40023c4b43ff9 Fixes: https://tracker.ceph.com/issues/65658 Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

This tries to preserve existing order but uses priorities to make it explicit and robust to future dispatchers being added. Except: - The beacon and metrics dispatcher have the highest priorities. This is to ensure we process these messages before trying to acquire any expensive locks (like mds_lock). - The monc dispatcher also has a relatively high priority for the same reasons. This change affects other daemons which may have ordered a dispatcher ahead of the monc but I cannot think of a legitimate reason to nor do I see an instance of it. Fixes: 7fc04be Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

leonid-s-usov

I love it!

batrick · 2024-05-17T01:25:13Z

This PR is under test in https://tracker.ceph.com/issues/66086.

cbodley · 2024-07-15T13:36:40Z

@batrick does the fs suite run under valgrind against ubuntu?

in https://tracker.ceph.com/issues/66336, the rgw suite is tracking MismatchedFree operator delete[] warnings under Messenger::add_dispatcher_head(). @mkogan1 bisected the regression down to this "msg: add priority to dispatcher invocation order" commit

i'm stumped by the actual valgrind issue though; the new/delete seem to be internal to stable_sort() in the standard library, and only effect ubuntu runs

batrick · 2024-07-15T19:51:53Z

@batrick does the fs suite run under valgrind against ubuntu?

Yes, see fs:verify.

in https://tracker.ceph.com/issues/66336, the rgw suite is tracking MismatchedFree operator delete[] warnings under Messenger::add_dispatcher_head(). @mkogan1 bisected the regression down to this "msg: add priority to dispatcher invocation order" commit

i'm stumped by the actual valgrind issue though; the new/delete seem to be internal to stable_sort() in the standard library, and only effect ubuntu runs

Ya, I'm not seeing an issue with the code. It's either a bug in glibc++ or valgrind.

cbodley · 2024-07-15T20:02:46Z

@batrick does the fs suite run under valgrind against ubuntu?

Yes, see fs:verify.

ok, thanks. i scanned through some recent fs suite results and did spot several occurrances of this MismatchedFree operator delete[] issue. for example, this run complains about it for mds.a, mds.c, mds.e, and mon.a

(pulpito summarizes the failure as valgrind error: Leak_StillReachable calloc calloc _dl_check_map_versions which may be why you're missing this?)

batrick requested a review from a team as a code owner May 15, 2024 01:45

github-actions bot added cephfs Ceph File System core mon labels May 15, 2024

batrick force-pushed the i66014 branch from 7e982e7 to 428dd7b Compare May 15, 2024 01:51

batrick requested a review from a team May 15, 2024 01:51

batrick mentioned this pull request May 15, 2024

mds: use regular dispatch for processing metrics #57081

Merged

14 tasks

rishabh-d-dave reviewed May 15, 2024

View reviewed changes

src/messages/MMDSBeacon.h Outdated Show resolved Hide resolved

leonid-s-usov suggested changes May 15, 2024

View reviewed changes

src/msg/Messenger.h Outdated Show resolved Hide resolved

src/msg/Messenger.h Outdated Show resolved Hide resolved

src/msg/Messenger.h Outdated Show resolved Hide resolved

batrick force-pushed the i66014 branch from 428dd7b to 159985a Compare May 15, 2024 14:25

leonid-s-usov suggested changes May 15, 2024

View reviewed changes

batrick force-pushed the i66014 branch 2 times, most recently from d9794d4 to 8defa5d Compare May 15, 2024 15:11

leonid-s-usov reviewed May 15, 2024

View reviewed changes

mds: note when dispatcher is called

1b93fd5

For order checking. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

batrick force-pushed the i66014 branch from 8defa5d to 515dfe1 Compare May 15, 2024 20:33

batrick added 3 commits May 15, 2024 21:16

batrick force-pushed the i66014 branch from 515dfe1 to 3291f39 Compare May 16, 2024 01:16

batrick requested review from leonid-s-usov and rishabh-d-dave May 16, 2024 01:18

batrick added the needs-review label May 16, 2024

leonid-s-usov approved these changes May 16, 2024

View reviewed changes

batrick added needs-qa wip-pdonnell-testing and removed needs-review labels May 16, 2024

batrick merged commit eb10f3d into ceph:main May 23, 2024

batrick deleted the i66014 branch May 23, 2024 01:16

This was referenced May 23, 2024

squid: mds: use regular dispatch for processing beacons #57682

Merged

reef: mds: use regular dispatch for processing beacons #57683

Merged

quincy: mds: use regular dispatch for processing beacons #57684

Closed

Conversation

batrick commented May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

leonid-s-usov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

batrick commented May 15, 2024

Uh oh!

leonid-s-usov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

batrick commented May 15, 2024

Uh oh!

leonid-s-usov May 15, 2024

Choose a reason for hiding this comment

Uh oh!

batrick May 15, 2024

Choose a reason for hiding this comment

Uh oh!

leonid-s-usov left a comment

Choose a reason for hiding this comment

Uh oh!

batrick commented May 17, 2024

Uh oh!

cbodley commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

batrick commented Jul 15, 2024

Uh oh!

cbodley commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

batrick commented May 15, 2024 •

edited

Loading

cbodley commented Jul 15, 2024 •

edited

Loading

cbodley commented Jul 15, 2024 •

edited

Loading