dmclock server side refactor by athanatos · Pull Request #30650 · ceph/ceph

athanatos · 2019-09-30T21:12:24Z

This PR replaces the OpQueue abstraction with a smaller Scheduler abstraction, creates a new, simpler mclock implementation of that abstraction, and removes the old mclock OpQueue implementations as well as the PriorityQueue implementation leaving the new mClockScheduler and wpq.

athanatos · 2019-09-30T21:13:18Z

Test run: http://pulpito.ceph.com/sjust-2019-09-28_23:02:01-rados-wip-sjust-testing-distro-basic-smithi/
Rerun: http://pulpito.ceph.com/sjust-2019-09-30_17:02:45-rados-wip-sjust-testing-distro-basic-smithi/
Remaining failures appear unrelated.

athanatos · 2019-09-30T21:26:47Z

@ivancich @liyichao This PR has a simpler version of the server side dmclock implementation. I'd like to merge it prior to adding the client side as this bit does not require protocol changes. Note, this patch removes the two existing mclock OpQueue implementations in favor of a simpler one based on a less complicated interface. Moreover, it removes PrioritizedQueue as no one appears to use it anymore. Does this seem reasonable?

https://github.com/athanatos/ceph/tree/sjust/wip-dmclock-cleanup has the client side portions. I'm holding off on it for a bit since it changes the MOSDOp encoding significantly to enable adding the mclock params to the pre-dispatch decode area.

xiexingguo · 2019-09-30T23:45:02Z

I like it.

CC: @Yan-waller

tchaikov · 2019-10-01T01:59:04Z

src/osd/scheduler/mClockScheduler.h

+
+namespace ceph {
+namespace osd {
+namespace scheduler {


nit, could put

namespace ceph::osd::scheduler {

athanatos · 2019-10-01T02:13:07Z

src/osd/scheduler/mClockScheduler.cc

+{
+  auto id = get_scheduler_id(item);
+  // TODO: express cost, mclock params in terms of per-node capacity?
+  auto cost = 1; //std::max(item.get_cost(), 1);


Note, I've specifically punted on cost here. I plan on expressing the client concept of the mClock params in terms of fractions of a single node of capacity. Those values will be transformed into actual params by multiplying and normalizing based on an online estimate from bluestore of the osd's capacity. Future work.

For testing purposes, iops is as good as anything else.

athanatos · 2019-10-01T02:18:15Z

retest this please

athanatos · 2019-10-11T23:46:50Z

retest this please

athanatos · 2019-10-14T21:27:42Z

retest this please

athanatos · 2019-10-15T00:37:35Z

retest this please

athanatos · 2019-10-16T23:09:09Z

Second set of test runs:

http://pulpito.ceph.com/sjust-2019-10-15_05:58:15-rados-wip-sjust-testing-distro-basic-smithi/
http://pulpito.ceph.com/sjust-2019-10-15_23:29:00-rados-wip-sjust-testing-distro-basic-smithi/

The failures seem unrelated. Looked at one dead one, seemed to be an IO error.

athanatos · 2019-10-16T23:09:43Z

@tchaikov I think this is ready for a real review now.

Not all schedulers necessarily depend on a particular strict/non-strict cutoff. Instead, simply shedule at CEPH_MSG_PRIO_HIGH. See also: d346fdc Signed-off-by: Samuel Just <sjust@redhat.com>

Signed-off-by: Samuel Just <sjust@redhat.com>

ivancich

Thanks for the simplifications. It was a bit unwieldy before, and I like the direction this is heading.

ivancich · 2019-10-21T19:25:12Z

src/osd/scheduler/OpSchedulerItem.cc

+namespace ceph {
+namespace osd {
+namespace scheduler {


Since you combined the namespace declarations in other places (e.g., OpSchedulerItem.h) you could do so as well here.

athanatos · 2019-10-21T21:58:33Z

@tchaikov Did you want to take a look at this, or is @ivancich 's review sufficient?

tchaikov · 2019-10-22T14:40:47Z

@athanatos yeah, i did want to review this change. but haven't got a chance to do so. i am skimming through it now.

tchaikov · 2019-10-22T14:39:06Z

src/osd/scheduler/OpSchedulerItem.h

-class OpQueueItem {
+namespace ceph::osd::scheduler {
+
+enum class op_scheduler_class : uint8_t {


can we remove "scheduler" from the names? as we already have "scheduler" in the namespace's name. i think that also the nice thing of namespace -- it has name in it =)

I almost did that, but even with the scheduler namespace, I found names like op_class or OpItem to be insufficiently specific without the scheduler portion. The main problem is that op/operation is a very overloaded concept in the ceph code base. I'd rather leave it like this even at the expense of a little extra verbosity.

src/osd/scheduler/OpSchedulerItem.h

src/test/osd/TestMClockScheduler.cc

src/common/options.cc

OpQueue is overkill for mclock based schedulers. The interface doesn't need to externalize the _strict modifiers, the scheduler can figure that out from the item itself. Introduce simpler Scheduler interface and add an adapter for the existing OpQueue based implementations. Signed-off-by: Samuel Just <sjust@redhat.com>

mClockScheduler schedules items based on op_scheduler_class with configured mclock parameters. Items which should be scheduled immediately (op_scheduler_class::immediate) are placed into a single queue as it's not clear that there's a reason to differentiate among them. A subsequent patch will add support for client provided mclock params and dmclock request state. Signed-off-by: Samuel Just <sjust@redhat.com>

The consensus seems to be that PrioritizedQueue is strictly worse than WeightedPriorityQueue. mClockClientQueue and mClockClassQueue are superceded by mClockScheduler. Signed-off-by: Samuel Just <sjust@redhat.com>

Signed-off-by: Samuel Just <sjust@redhat.com>

mClockPriorityQueue (mClockQueue class) is an older mClock implementation of the OpQueue abstraction. This was replaced by a simpler implementation of the OpScheduler abstraction as part of ceph#30650. The simpler implementation of mClockScheduler is being currently used. This commit removes the unused src/common/mClockPriorityQueue.h along with the associated unit test file: test_mclock_priority_queue.cc. Other miscellaneous changes, - Remove the cmake references to the unit test file - Remove the inclusion of the header file in mClockScheduler.h Signed-off-by: Sridhar Seshasayee <sseshasa@redhat.com>

mClockPriorityQueue (mClockQueue class) is an older mClock implementation of the OpQueue abstraction. This was replaced by a simpler implementation of the OpScheduler abstraction as part of ceph/ceph#30650. The simpler implementation of mClockScheduler is being currently used. This commit removes the unused src/common/mClockPriorityQueue.h along with the associated unit test file: test_mclock_priority_queue.cc. Other miscellaneous changes, - Remove the cmake references to the unit test file - Remove the inclusion of the header file in mClockScheduler.h Signed-off-by: Sridhar Seshasayee <sseshasa@redhat.com>

mClockPriorityQueue (mClockQueue class) is an older mClock implementation of the OpQueue abstraction. This was replaced by a simpler implementation of the OpScheduler abstraction as part of ceph#30650. The simpler implementation of mClockScheduler is being currently used. This commit removes the unused src/common/mClockPriorityQueue.h along with the associated unit test file: test_mclock_priority_queue.cc. Other miscellaneous changes, - Remove the cmake references to the unit test file - Remove the inclusion of the header file in mClockScheduler.h Signed-off-by: Sridhar Seshasayee <sseshasa@redhat.com> (cherry picked from commit 28a26f7)

mClockPriorityQueue (mClockQueue class) is an older mClock implementation of the OpQueue abstraction. This was replaced by a simpler implementation of the OpScheduler abstraction as part of ceph/ceph#30650. The simpler implementation of mClockScheduler is being currently used. This commit removes the unused src/common/mClockPriorityQueue.h along with the associated unit test file: test_mclock_priority_queue.cc. Other miscellaneous changes, - Remove the cmake references to the unit test file - Remove the inclusion of the header file in mClockScheduler.h Signed-off-by: Sridhar Seshasayee <sseshasa@redhat.com> (cherry picked from commit 28a26f7) (cherry picked from commit 6dffc43) Resolves: rhbz#2330755

athanatos added core DNM labels Sep 30, 2019

athanatos requested a review from a team as a code owner September 30, 2019 21:12

athanatos self-assigned this Sep 30, 2019

tchaikov reviewed Oct 1, 2019

View reviewed changes

athanatos commented Oct 1, 2019

View reviewed changes

athanatos mentioned this pull request Oct 1, 2019

Small fixes to dmclock_server and run_every includes ceph/dmclock#74

Merged

athanatos force-pushed the sjust/wip-dmclock-server-only branch 2 times, most recently from 115801b to 27ce332 Compare October 14, 2019 19:14

athanatos changed the title ~~DNM: dmclock server side refactor~~ dmclock server side refactor Oct 16, 2019

athanatos added 2 commits October 16, 2019 16:14

PrimaryLogPG::maybe_kick_recovery: simply use CEPH_MSG_PRIO_HIGH

488625e

Not all schedulers necessarily depend on a particular strict/non-strict cutoff. Instead, simply shedule at CEPH_MSG_PRIO_HIGH. See also: d346fdc Signed-off-by: Samuel Just <sjust@redhat.com>

common/OpQueue.h: fix whitespace and add spaces between methods

e975f69

Signed-off-by: Samuel Just <sjust@redhat.com>

athanatos force-pushed the sjust/wip-dmclock-server-only branch from 27ce332 to caab2d5 Compare October 16, 2019 23:14

sebastian-philipp added dashboard needs-review and removed dashboard labels Oct 17, 2019

athanatos removed the DNM label Oct 17, 2019

athanatos removed the request for review from a team October 17, 2019 22:41

ivancich approved these changes Oct 21, 2019

View reviewed changes

tchaikov reviewed Oct 22, 2019

View reviewed changes

athanatos force-pushed the sjust/wip-dmclock-server-only branch from caab2d5 to 3642cb2 Compare October 22, 2019 22:12

athanatos added 3 commits October 23, 2019 13:33

osd/: remove legacy schedulers

2157ac7

The consensus seems to be that PrioritizedQueue is strictly worse than WeightedPriorityQueue. mClockClientQueue and mClockClassQueue are superceded by mClockScheduler. Signed-off-by: Samuel Just <sjust@redhat.com>

PendingReleaseNotes: note mClockScheduler config changes

6eaf324

Signed-off-by: Samuel Just <sjust@redhat.com>

athanatos force-pushed the sjust/wip-dmclock-server-only branch from 3642cb2 to 6eaf324 Compare October 23, 2019 20:34

gregsfortytwo merged commit e190825 into ceph:master Oct 23, 2019

sseshasa mentioned this pull request Oct 9, 2023

osd: Apply randomly selected scheduler type across all OSD shards #53524

Merged

14 tasks

Conversation

athanatos commented Sep 30, 2019

Uh oh!

athanatos commented Sep 30, 2019

Uh oh!

athanatos commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiexingguo commented Sep 30, 2019

Uh oh!

tchaikov Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

athanatos Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

athanatos Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

athanatos commented Oct 1, 2019

Uh oh!

athanatos commented Oct 11, 2019

Uh oh!

athanatos commented Oct 14, 2019

Uh oh!

athanatos commented Oct 15, 2019

Uh oh!

athanatos commented Oct 16, 2019

Uh oh!

athanatos commented Oct 16, 2019

Uh oh!

ivancich left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivancich Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

athanatos commented Oct 21, 2019

Uh oh!

tchaikov commented Oct 22, 2019

Uh oh!

tchaikov Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

athanatos Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

athanatos commented Sep 30, 2019 •

edited

Loading

ivancich left a comment •

edited

Loading