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

rgw: add radosclient finisher to perf counter #13535

Merged
merged 1 commit into from Mar 20, 2017

Conversation

Projects
None yet
3 participants
@dongbula

dongbula commented Feb 20, 2017

Signed-off-by: lvshuhua lvshuhua@cmss.chinamobile.com

@liewegas liewegas added the rgw label Feb 20, 2017

@dongbula

This comment has been minimized.

dongbula commented Mar 1, 2017

@cbodley please help review this, thank you :)

@cbodley

This comment has been minimized.

Contributor

cbodley commented Mar 1, 2017

hi @dongbula, looks okay to me. but this is a change that could effect the performance of all rados clients, not just radosgw. are you seeing problems in the rados client that require this instrumentation?

@dongbula

This comment has been minimized.

dongbula commented Mar 7, 2017

@cbodley hi, I want to estimate the proportion of time spent by radosclient finisher thread(radosgw) under high loads, that will be difficulty without perf counter. Does it really have an strong effect on performance?

@cbodley

This comment has been minimized.

Contributor

cbodley commented Mar 8, 2017

@liewegas what is your intuition here? is there any risk to adding perf counters to the librados client finisher?

@dongbula i don't think this warrants a change to the librados api (breaking compatibility with existing applications). i think we'll just want to enable it for all librados clients, instead of making it optional

@@ -406,7 +406,8 @@ CEPH_RADOS_API int rados_create2(rados_t *pcluster,
* @returns 0 on success, negative error code on failure
*/
CEPH_RADOS_API int rados_create_with_context(rados_t *cluster,
rados_config_t cct);
rados_config_t cct,
const char *name);

This comment has been minimized.

@liewegas

liewegas Mar 8, 2017

Member

we can't change the ABI here; need to define a new variant of this function to add the arg.

@@ -1221,7 +1221,7 @@ namespace librados
int init(const char * const id);
int init2(const char * const name, const char * const clustername,
uint64_t flags);
int init_with_context(config_t cct_);
int init_with_context(config_t cct_, const std::string& name = "");

This comment has been minimized.

@liewegas

liewegas Mar 8, 2017

Member

same here. it has to be init_with_context_name or something.. don't overload the same name as gonig from non-overloaded to overloaded breaks the C++ ABI

@liewegas

This comment has been minimized.

Member

liewegas commented Mar 8, 2017

I don't see where the perfcounter comes in here. Does Finisher take a different code path simply because it has a name or something?

I have some general concerns about perfcounter overhead, but I think they're orthogonal (and likely fixable). Still, I would be careful about adding counters to very hot paths (e.g., mutex lock/unlock). I don't see that here though?

@cbodley

This comment has been minimized.

Contributor

cbodley commented Mar 8, 2017

@liewegas if the Finisher constructor is given a name parameter, it initializes perf counters. the normal librados::RadosClient constructor wasn't passing one. the first draft of this pr just added a name argument there. the second draft of the pr made that finisher argument optional, by adding another librados::RadosClient constructor and extending the librados api. so i think the first draft is what we want

@liewegas

This comment has been minimized.

Member

liewegas commented Mar 8, 2017

lvshuhua
rgw: add radosclient finisher to perf counter
Fixes: http://tracker.ceph.com/issues/19011
Signed-off-by: lvshuhua <lvshuhua@cmss.chinamobile.com>
@dongbula

This comment has been minimized.

dongbula commented Mar 9, 2017

@cbodley

cbodley approved these changes Mar 9, 2017

@cbodley

This comment has been minimized.

Contributor

cbodley commented Mar 20, 2017

@liewegas this passed the rgw suite, does it need to run against others?

@liewegas liewegas merged commit decb9d0 into ceph:master Mar 20, 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