Skip to content
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

common: fix reset max in Throttle using perf reset command #6300

Merged
merged 1 commit into from Nov 9, 2015

Conversation

XinzeChi
Copy link
Contributor

we could not reset max to 0 in Throttle using perf reset command,
such as throttle-osd_client_messages, throttle-filestore_bytes and so on.

Fixes: #13517
Signed-off-by: Xinze Chi xinze@xsky.com

@XinzeChi
Copy link
Contributor Author

@dachary , please review it.

@ghost
Copy link

ghost commented Oct 17, 2015

@XinzeChi could you please explain in the commit message why and how this patch fixes the problem ?

@ghost ghost added bug-fix core common and removed core labels Oct 17, 2015
@ghost ghost self-assigned this Oct 17, 2015
@XinzeChi
Copy link
Contributor Author

repush based on @dachary 's comment.

@XinzeChi
Copy link
Contributor Author

@dachary , when the data.type is PERFCOUNTER_U64, the reset function would not reset it.

@ghost
Copy link

ghost commented Oct 27, 2015

@XinzeChi right... I got it backwards

@ghost
Copy link

ghost commented Oct 27, 2015

@XinzeChi so, what you want with this patch is to be able to call reset() so that it does not reset l_throttle_max to zero, right ?

@ghost
Copy link

ghost commented Oct 27, 2015

Should this also be done for l_throttle_val ?

we could not reset max to 0 in Throttle using perf reset command,
such as throttle-osd_client_messages, throttle-filestore_bytes and so on.

If we call PerfCountersBuilder::add_u64, the data.type would be PERFCOUNTER_U64.
the reset function would skip it if data.type is PERFCOUNTER_U64.

Fixes: ceph#13517
Signed-off-by: Xinze Chi <xinze@xsky.com>
@XinzeChi
Copy link
Contributor Author

@dachary , Yes, We should also do it for l_throttle_val . Thanks.

@ghost ghost added the needs-qa label Oct 27, 2015
@ghost
Copy link

ghost commented Oct 27, 2015

Reviewed-by: Loic Dachary <ldachary@redhat.com>

after a test suite passes.

@ghost
Copy link

ghost commented Oct 27, 2015

@XinzeChi are you willing to run a rados suite with this patch ? I don't see why it would fail anything but better be safe ;-)

@XinzeChi
Copy link
Contributor Author

I'm afraid I don't have the authority to run a rados suite. @tchaikov would help to run a test suite later. Thanks.

@tchaikov
Copy link
Contributor

@XinzeChi will collect more needs-qa pr, and run them in a batch.

liewegas added a commit that referenced this pull request Nov 9, 2015
common: fix reset max in Throttle using perf reset command

Reviewed-by: Loic Dachary <ldachary@redhat.com>
@liewegas liewegas merged commit b533274 into ceph:master Nov 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants