crimson/osd/osd_operation: fix dump_historic_slow_ops command works #56994
Conversation
|
jenkins test make check |
|
jenkins test windows |
|
jenkins test make check arm64 |
|
The commit message "crimson/osd/osd_operation.cc: optimize the calculation and output of slow_ops" suggests that this is a performance optimization. In fact, from the linked bug, this actually fixes a few bugs in how the command works. Please update the commit message to explain what this commit changes and how it fixes the bugs. |
|
As a guideline, "optimize ..." is basically never an acceptable commit message as it really doesn't tell a future reader what the commit is changing. |
* The first 'local_conf()->osd_op_history_slow_op_size' elements in the history_cliend_registry list store the slow ops log, while the last 'local_conf()->osd_op_history_size' elements store the normal history ops log * dump_historic_slow_ops command will output real slow ops log and slow ops numbers Fixes: https://tracker.ceph.com/issues/65531 Signed-off-by: junxiang Mu <1948535941@qq.com>
guojidan
changed the title
| @@ -63,7 +63,7 @@ void OSDOperationRegistry::put_historic(const ClientRequest& op) | |||
| static_cast<size_t>(OperationTypeCode::historic_client_request); | |||
| auto& client_registry = get_registry<client_reg_index>(); | |||
| auto& historic_registry = get_registry<historic_reg_index>(); | |||
| historic_registry.splice(std::end(historic_registry), | |||
| historic_registry.splice(last_of_recents, | |||
There was a problem hiding this comment.
Don't we always want to insert at the end of historic_registry?
| ++num_slow_ops; | ||
| } else { | ||
| ++num_recent_ops; | ||
| --last_of_recents; |
There was a problem hiding this comment.
I'm confused as to what last_of_recents is supposed to indicate. Is it the case that for all ops in the interval [last_of_recents, std::end(historic_registry)), get_duration(op) <= local_conf()->osd_op_complaint_time? If so, I'm not sure this patch actually maintains that invariant.
| @@ -100,6 +100,10 @@ void OSDOperationRegistry::put_historic(const ClientRequest& op) | |||
| ClientRequest::ICRef(&fastest_historic_op, /* add_ref= */false); | |||
| --num_slow_ops; | |||
| } | |||
| if (num_recent_ops > local_conf()->osd_op_history_size) { | |||
| historic_registry.pop_back(); | |||
| --num_recent_ops; | |||
There was a problem hiding this comment.
The last op may not be recent.
Fixes: https://tracker.ceph.com/issues/65531
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an
xbetween the brackets:[x]. Spaces and capitalization matter when checking off items this way.Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windowsjenkins test rook e2e