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

mon/OSDMonitor: use up set instead of acting set in reweight_by_utilization #13802

Merged
merged 1 commit into from May 31, 2017

Conversation

Projects
None yet
5 participants
@LiumxNL
Contributor

LiumxNL commented Mar 6, 2017

Signed-off-by: Mingxin Liu mingxin@xsky.com

@LiumxNL

This comment has been minimized.

Contributor

LiumxNL commented Mar 6, 2017

@tchaikov @liewegas pls review, thanks!

@LiumxNL

This comment has been minimized.

Contributor

LiumxNL commented Mar 8, 2017

@tchaikov would you mind to have a look? TY

@jecluis jecluis added core mon labels Mar 8, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Mar 20, 2017

@LiumxNL sorry for the latency, i will take a look tmr

@tchaikov tchaikov self-assigned this Mar 20, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 27, 2017

rebsae this please

@LiumxNL

This comment has been minimized.

Contributor

LiumxNL commented May 19, 2017

@tchaikov rebased.

for (unsigned ps = 0; ps < pi->get_pg_num(); ++ps) {
pg_t pgid(ps, pool_id, -1);
total_pg += pi->get_size();
pg_to_up_acting_osds(pgid, &up, &up_primary,
&acting, &acting_primary);
pg_to_up_acting_osds(pgid, &up, &up_primary, NULL, NULL);

This comment has been minimized.

@tchaikov

tchaikov May 19, 2017

Contributor

nit, use nullptr if you please.

for (int osd : up) {
if (osd >= 0 && osd < get_max_osd())
++base_by_osd[osd];
}
if (newmap) {
newmap->pg_to_up_acting_osds(pgid, &up2, &up_primary,
&acting, &acting_primary);
newmap->pg_to_up_acting_osds(pgid, &up2, &up_primary, NULL, NULL);

This comment has been minimized.

@tchaikov

tchaikov May 19, 2017

Contributor

ditto.

@@ -2886,7 +2886,7 @@ int reweight::by_utilization(
for (const auto& pg : pgm.pg_stat) {
if (pools && pools->count(pg.first.pool()) == 0)
continue;
for (const auto acting : pg.second.acting) {
for (const auto acting : pg.second.up) {

This comment has been minimized.

@tchaikov

tchaikov May 19, 2017

Contributor

i wonder, if it is the acting set who is serving the pg and they are where the objects of this PG are currently stored. why shall we reweight the up set based on the utilization of the up set who is not holding the objects of the PG? this does not sound reasonable to me.

This comment has been minimized.

@LiumxNL

LiumxNL May 19, 2017

Contributor

IMHO, acting set is temporarily response for a PG if encountered some osdmap changes, up set is the destination. we expect PG eventually can be placed in up set, so maybe user will prefer to a final distribution of weight. pg number of a osd act as acting which reweight mechanism relies on will fluctuate during rebalance, the result maybe not that accurate.
anyway, i agree that it might be not an absolute advantage. let it go or keep here?

This comment has been minimized.

@LiumxNL

LiumxNL May 19, 2017

Contributor

this change is for "reweight by pg". mostly amount of used will be in accordance with pg number of this osd.

This comment has been minimized.

@tchaikov

tchaikov May 19, 2017

Contributor

as is said, i don't think this change is a pure win unless you can prove it. and i think acting is a safer bet.

This comment has been minimized.

@LiumxNL

LiumxNL May 19, 2017

Contributor

ok, cleared.

osd: haven't take acting into account here, remove unnecessary variable
Signed-off-by: Mingxin Liu <mingxin@xsky.com>

@tchaikov tchaikov added the needs-qa label May 19, 2017

@tchaikov tchaikov removed their assignment May 19, 2017

@liewegas liewegas merged commit 2c231f9 into ceph:master May 31, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment