New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

osd: kill all remaining MOSDSubOp users #13401

Merged
merged 10 commits into from Apr 3, 2017

Conversation

Projects
None yet
2 participants
@liewegas
Member

liewegas commented Feb 13, 2017

Replace them with new messages. Keep some handling around for the jewel -> luminous upgrade (we can remove MOSDSubOp[Reply] entirely post-luminous).

@liewegas liewegas changed the title from osd: kill most remaining MOSDSubOp users to DNM: osd: kill most remaining MOSDSubOp users Feb 13, 2017

@gregsfortytwo

This comment has been minimized.

Member

gregsfortytwo commented Feb 13, 2017

@liewegas, these don't display well on github with all the rebasing and the hashes aren't consistent across PRs either -- can you call out the new commits (or their starting point)? :)

@liewegas

This comment has been minimized.

Member

liewegas commented Feb 13, 2017

@liewegas liewegas changed the title from DNM: osd: kill most remaining MOSDSubOp users to DNM: osd: kill all remaining MOSDSubOp users Feb 14, 2017

@liewegas liewegas changed the title from DNM: osd: kill all remaining MOSDSubOp users to osd: kill all remaining MOSDSubOp users Feb 15, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Feb 16, 2017

retest this please

@tchaikov tchaikov self-requested a review Mar 10, 2017

dzafman added a commit to dzafman/ceph that referenced this pull request Mar 16, 2017

TEMPORARY CHANGE TO SIMPLIFY CODE DURING MODIFICATIONS
DO NOT MERGE THIS
THIS WILL BE DONE IN ANOTHER PULL REQUEST
ceph#13401
@gregsfortytwo

This comment has been minimized.

Member

gregsfortytwo commented Mar 17, 2017

Retest this please

@liewegas

This comment has been minimized.

Member

liewegas commented Mar 17, 2017

@gregsfortytwo

Few comments for when you come back to this — trying to clear out some of the backlogged cleanups since it's getting hard to keep track of which things I do or don't depend on. ;)

// send ack to acker only if we haven't sent a commit already
if (ack) {
// send ack to acker only if we haven't sent a commit already

This comment has been minimized.

@gregsfortytwo

gregsfortytwo Mar 17, 2017

Member

Can kill this comment now it's all unified in one block?

@@ -0,0 +1,64 @@
// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-

This comment has been minimized.

@gregsfortytwo

gregsfortytwo Mar 17, 2017

Member

Should probably include the copyright/LGPL declaration.

pg_shard_t target = to_remove[i].get<2>();
MOSDPGBackfillRemove *m;
if (reqs.count(target)) {
m = reqs[target];

This comment has been minimized.

@gregsfortytwo

gregsfortytwo Mar 17, 2017

Member

I think we're considering this an anti-pattern now, and should use iterator find()?

spg_t(info.pgid.pgid, get_primary().shard),
msg->map_epoch,
pg_whoami);
::encode(map, reply->get_data());

This comment has been minimized.

@gregsfortytwo

gregsfortytwo Mar 17, 2017

Member

...are you embedding dencode logic for a Message in the PG code?
Please either give MOSDRepScrubMap a type for this purpose, or a real bufferlist that can be internally encoded/appended whose type can be separately versioned.

This comment has been minimized.

@liewegas

liewegas Mar 17, 2017

Member

It already has a type (ScrubMap) that can be versioned. We're just using the data payload instead of a separate bufferlist that's encoded into the front.

We could make the message class do the decode, but then we have to make sure there is an efficient swap() or move ctor for ScrubMap.

I don't think either of those thing would actually help?

This comment has been minimized.

@gregsfortytwo

gregsfortytwo Mar 17, 2017

Member

My main concern is that we're requiring specific OSD paths for this message to decode properly. If we stick the ScrubMap into a bufferlist owned by the message, then even though it's encoded/decoded by the OSD, we can change the order of things without breaking stuff (by, say, sticking new fields on the end of the Message that only one OSD is new enough to know about). Just give it a bufferlist that MOSDRepScrubMap encodes/decodes; that surely isn't noticeable in performance.

This comment has been minimized.

@gregsfortytwo

gregsfortytwo Mar 17, 2017

Member

that's the same thing? this is the data payload (not "front"); it's not used by anything else.

Oh right. I am still discontented but probably just because I was thinking about it wrong.

@liewegas

This comment has been minimized.

Member

liewegas commented Mar 17, 2017

dzafman added a commit to dzafman/ceph that referenced this pull request Mar 25, 2017

TEMPORARY change to simplify code during modifications
and conflict with ceph#13401 due to added arg to handle_push_reply()

No sig by line so that this pull can't be merge

See ceph#13401

dzafman added a commit to dzafman/ceph that referenced this pull request Mar 29, 2017

TEMPORARY change to simplify code during modifications
and conflict with ceph#13401 due to added arg to handle_push_reply()

No sig by line so that this pull can't be merge

See ceph#13401

liewegas added some commits Feb 13, 2017

osd: use new MOSDPGBackfillRemove instead of MOSDSubOp
One of the last remaining MOSDSubOp users!

Signed-off-by: Sage Weil <sage@redhat.com>
osd/ReplicatedBackend: remove legacy push/pull op handling
These are never sent, and thus never recieved.

Signed-off-by: Sage Weil <sage@redhat.com>
osd: use new MOSDRepScrubMap message for ScrubMap instead of MOSDSubOp
This is one of the last remaining users of MOSDSubOp!

Signed-off-by: Sage Weil <sage@redhat.com>
osd: use MOSDScrubReserve instead of MOSDSubOp for scrub reservations
This is the last MOSDSubOp user.

Note that while the next step is to move to AsyncReserver internally,
this isn't quite yet possible since AsyncReserve "blocks" indefinitely
so we wouldn't generate a REJECT.  Changing how we schdule scrubs
internally will take a bit more work.

Signed-off-by: Sage Weil <sage@redhat.com>
osd: drop unnecessary declarations in headers
Signed-off-by: Sage Weil <sage@redhat.com>
osd/ReplicatedBackend: use more __func__
Signed-off-by: Sage Weil <sage@redhat.com>
osd/ReplicatedBackend: no empty MOSDSubOps anymore
Signed-off-by: Sage Weil <sage@redhat.com>
osd/ReplicatedBackend: rename sub_op_modify* do_repop*
Signed-off-by: Sage Weil <sage@redhat.com>

liewegas added some commits Mar 28, 2017

osd: drop unused MOSDSubOp includes
Signed-off-by: Sage Weil <sage@redhat.com>
osd/ReplicatedBackend: remove MOSDSubOp cruft from repop_commit
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas

This comment has been minimized.

Member

liewegas commented Mar 31, 2017

retest this please

@liewegas liewegas merged commit c3d1333 into ceph:master Apr 3, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@liewegas liewegas deleted the liewegas:wip-kill-subop branch Apr 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment