os/bluestore: improve usability for bluestore/bluefs perf counters#41557
os/bluestore: improve usability for bluestore/bluefs perf counters#41557ifed01 merged 8 commits intoceph:masterfrom
Conversation
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
20aa9ae to
565c342
Compare
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
565c342 to
a1ae753
Compare
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
ljflores
left a comment
There was a problem hiding this comment.
Had a few comments about perf counter nicks since I worked on a PR recently that addressed updating/adding nicks.
Improves both general usability and daemonperf output Signed-off-by: Igor Fedotov <ifedotov@suse.com>
Print slow writing and reading volumes Signed-off-by: Igor Fedotov <ifedotov@suse.com>
Signed-off-by: Igor Fedotov <ifedotov@suse.com>
Signed-off-by: Igor Fedotov <ifedotov@suse.com>
Better naming and description. Signed-off-by: Igor Fedotov <ifedotov@suse.com>
Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>
a1ae753 to
9b19e60
Compare
|
rebase and some cleanup are done. @ljflores mind reviewing? |
Sure! I want to take some time to understand the rest of the logic, but I will leave another review soon. Cleanup looks good. |
ljflores
left a comment
There was a problem hiding this comment.
Everything looks okay to me! I went through and checked that none of the counters got lost in your re-arrangement, which looks really good by the way.
The one thing I'll say is, if there are any counters in BlueStore or BlueFS that are necessary for analyzing performance, make sure they're set to "PRIO_USEFUL" or higher if they're not already. In that case, they would be accessible to the Manager Module, and we could collect them in Telemetry.
| //**************************************** | ||
| b.add_time_avg(l_bluestore_kv_flush_lat, "kv_flush_lat", | ||
| "Average kv_thread flush latency", | ||
| "kfsl", PerfCountersBuilder::PRIO_INTERESTING); |
There was a problem hiding this comment.
Thanks for modifying this nick! I accidentally introduced this duplication in #43405. kfsl makes more sense anyway.
Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>
Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>
Thanks for reviewing! Just pushed a couple more commits to adjust the priority as per your comment. Would you take another look, please (previous commits are untouched). |
ljflores
left a comment
There was a problem hiding this comment.
Looks good! I tested your branch on the Telemetry Perf Channel (ceph telemetry show perf), and this is a sample of the perf counters that would be picked up:
"osd.0": {
"bluefs": {
"bytes_written_slow": {
"value": 0
},
"bytes_written_sst": {
"value": 8192
},
"bytes_written_wal": {
"value": 19091456
},
"db_total_bytes": {
"value": 1073733632
},
"db_used_bytes": {
"value": 10485760
},
"log_bytes": {
"value": 12279808
},
"logged_bytes": {
"value": 11980800
},
"max_bytes_db": {
"value": 13631488
},
"max_bytes_slow": {
"value": 0
},
"max_bytes_wal": {
"value": 24117248
},
"num_files": {
"value": 13
},
"read_bytes": {
"value": 353642
},
"read_count": {
"value": 89
},
"read_disk_bytes": {
"value": 1134592
},
"read_disk_bytes_db": {
"value": 86016
},
"read_disk_bytes_slow": {
"value": 0
},
"read_disk_bytes_wal": {
"value": 1052672
},
"read_disk_count": {
"value": 12
},
"read_prefetch_bytes": {
"value": 3650
},
"read_prefetch_count": {
"value": 3
},
"read_random_buffer_bytes": {
"value": 3835
},
"read_random_buffer_count": {
"value": 18
},
"read_random_bytes": {
"value": 3835
},
"read_random_count": {
"value": 18
},
"read_random_disk_bytes": {
"value": 0
},
"read_random_disk_bytes_db": {
"value": 0
},
"read_random_disk_bytes_slow": {
"value": 0
},
"read_random_disk_bytes_wal": {
"value": 0
},
"read_random_disk_count": {
"value": 0
},
"slow_total_bytes": {
"value": 107374182400
},
"slow_used_bytes": {
"value": 0
},
"wal_total_bytes": {
"value": 1048571904
},
"wal_used_bytes": {
"value": 24117248
}
},
"bluestore": {
"allocated": {
"value": 1228800
},
"bluestore_onode_hits": {
"value": 7126
},
"bluestore_onode_misses": {
"value": 787
},
"clist_lat": {
"count": 0,
"value": 0
},
"compress_lat": {
"count": 0,
"value": 0
},
"compressed": {
"value": 0
},
"compressed_allocated": {
"value": 0
},
"compressed_original": {
"value": 0
},
"csum_lat": {
"count": 20,
"value": 24709
},
"decompress_lat": {
"count": 0,
"value": 0
},
"kv_commit_lat": {
"count": 2910,
"value": 1575397719
},
"kv_final_lat": {
"count": 2909,
"value": 308077646
},
"kv_flush_lat": {
"count": 2910,
"value": 70159641
},
"kv_sync_lat": {
"count": 2910,
"value": 1645557360
},
"omap_get_keys_lat": {
"count": 0,
"value": 0
},
"omap_get_values_lat": {
"count": 0,
"value": 0
},
"omap_lower_bound_lat": {
"count": 0,
"value": 0
},
"omap_next_lat": {
"count": 10,
"value": 7791
},
"omap_seek_to_first_lat": {
"count": 1,
"value": 5450
},
"omap_upper_bound_lat": {
"count": 2,
"value": 29323
},
"read_lat": {
"count": 151,
"value": 5570709
},
"read_onode_meta_lat": {
"count": 346,
"value": 684319
},
"read_wait_aio_lat": {
"count": 195,
"value": 2229213
},
"reads_with_retries": {
"value": 0
},
"remove_lat": {
"count": 138,
"value": 95701449
},
"state_aio_wait_lat": {
"count": 6439,
"value": 942738659
},
"state_deferred_aio_wait_lat": {
"count": 83,
"value": 14069449
},
"state_deferred_cleanup_lat": {
"count": 83,
"value": 16819863825
},
"state_deferred_queued_lat": {
"count": 83,
"value": 101468726323
},
"state_done_lat": {
"count": 6439,
"value": 14971041652
},
"state_finishing_lat": {
"count": 6439,
"value": 29091760
},
"state_io_done_lat": {
"count": 6439,
"value": 75082238
},
"state_kv_commiting_lat": {
"count": 6439,
"value": 8062662094
},
"state_kv_done_lat": {
"count": 6439,
"value": 1079209
},
"state_kv_queued_lat": {
"count": 6439,
"value": 6181699062
},
"state_prepare_lat": {
"count": 6439,
"value": 1331529051
},
"stored": {
"value": 679686
},
"txc_commit_lat": {
"count": 6439,
"value": 16594306655
},
"txc_submit_lat": {
"count": 6439,
"value": 1580534219
},
"txc_throttle_lat": {
"count": 6439,
"value": 112576261
}
},
The BlueStore counters you marked as PRIO_DEBUGONLY will not be accessible to the Manager module, but that makes no difference to me since we don't currently collect those counters in Telemetry anyway.
|
@ifed01 is this backportable to O/P ? |
I asked because it might have been useful to see a plot of aio read lat to debug an io scheduler issue (bfq looks broken in latest CentOS kernel). Maybe an O/P specific fix for just that would be simple enough. We can look on our side. |
|
so in fact you need just part of this PR which introduces new perf counters.. That should be even easier... |
During phase of log fnode switch from new_log to actual log it is necessary to hold lock. Added that locking. Modified procedure of transporting extents from old_log to new_log. Now new_log gets additional extents, instead of removing from old_log; this shortens time when we need to hold log.lock. Signed-off-by: Adam Kupczyk <akupczyk@redhat.com> (cherry picked from commit f2acf52) Conflicts: src/os/bluestore/BlueFS.cc ( - Lacking ceph#41557 - misc stuff with vselector )
Add function tags and comments related to locks. Fixed locking graph documentation. Signed-off-by: Adam Kupczyk <akupczyk@redhat.com> (cherry picked from commit a091ea7) Conflicts: src/os/bluestore/BlueFS.h (Lacking backport for ceph#41557)
Requires in pacific backport only due to the lack of ceph#41557 Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>
Requires in pacific backport only due to the lack of ceph#41557 Resolves: rhbz#2264054 Signed-off-by: Igor Fedotov <igor.fedotov@croit.io> (cherry picked from commit 027d414)
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 apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume tox