squid: a series of optimizations for kerneldevice discard#59065
squid: a series of optimizations for kerneldevice discard#59065SrinivasaBharath merged 12 commits intoceph:squidfrom
Conversation
Signed-off-by: Matt Vandermeulen <matt@reenigne.net> (cherry picked from commit 4ae47bd)
Signed-off-by: Matt Vandermeulen <matt@reenigne.net> (cherry picked from commit d8815e1)
Signed-off-by: Matt Vandermeulen <matt@reenigne.net> (cherry picked from commit 671e126)
Signed-off-by: Matt Vandermeulen <matt@reenigne.net> (cherry picked from commit 5c4a234)
…bled Fix calls bdev->discard_drain() before calling store_allocator() to make sure all freed space is reflected in the allocator before destaging it The fix set a timeout for the drain call (500msec) and if expires will not store the allocator (forcing a recovery on the next startup) Fixes: https://tracker.ceph.com/issues/65298 Signed-off-by: Gabriel BenHanokh <gbenhano@redhat.com> (cherry picked from commit 3aa891d)
…toring the allocator. ON fast shutdown we will simply copy the discard queue entries to the allocator Signed-off-by: Gabriel BenHanokh <gbenhano@redhat.com> (cherry picked from commit 4762ffa)
On fast-shutdown take over the main discarded queue copying it to the allocator and only wait for the threads to commit their small private discarded queues Signed-off-by: Gabriel BenHanokh <gbenhano@redhat.com> (cherry picked from commit 1e10a9b)
Signed-off-by: Gabriel BenHanokh <gbenhano@redhat.com> (cherry picked from commit b37080c)
…_change() This fixes two issues that were introduced by 755f3e0: 1. After an OSD boots, discard threads were not stopped when bdev_enable_discard was set to false, whereas that was the intent of that commit. 2. If bdev_enable_discard or bdev_async_discard_threads are configured with a mask that can't be evaluated at OSD boot (e.g. a device class), then async discard won't be enabled until a later config change to bdev_async_discard_threads. Fixes: https://tracker.ceph.com/issues/66817 Signed-off-by: Joshua Baergen <jbaergen@digitalocean.com> (cherry picked from commit 8ffe35e)
1. In _discard_stop(), the wait for !discard_threads.empty() was there from a prior implementation where there could theoretically be a race between _discard_start() and _discard_stop(). If that race does exist, this check won't help; not only is _discard_stop() not called if discard_threads is empty, but discard_threads won't be populated until _discard_start() runs and thus this won't detect such a race. 2. Calling _discard_stop() from handle_conf_change() is a guaranteed deadlock because discard_lock is already held, so don't do that. Use the same flow whether we're stopping a subset of threads or all threads. 3. Asking a subset of discard threads to stop was not guaranteed to take effect, since if they continued to find contents in discard_queue then they would continue to run indefinitely. Add additional logic to _discard_thread() to have threads stop if they have been requested to stop and other threads exist to continue draining discard_queue. 4. Make the flow of _discard_stop() and handle_conf_change() more similar. Fixes: https://tracker.ceph.com/issues/66817 Signed-off-by: Joshua Baergen <jbaergen@digitalocean.com> (cherry picked from commit 3d4a899)
Instead of having _discard_start() and _discard_stop() partially or completely duplicate functionality in handle_conf_change(), have a single _discard_update_threads() that can handle all three. Loops are tidied slightly, the unnecessary target_discard_threads class variable has been removed, and now handle_conf_change() will respect support_discard. Signed-off-by: Joshua Baergen <jbaergen@digitalocean.com> (cherry picked from commit 617c936)
|
jenkins test make check |
|
@YiteGu @ifed01 please see this error found in QA testing that looks related to this PR: |
At first glance, this backport does not modify the code related to ceph-bluestore-tool rehard action. where I can see log file of ceph-bluestore-tool reshard? |
ifed01
left a comment
There was a problem hiding this comment.
Please bring discard_stop initialization back.
I have a feeling we have two issues in this batch. This is definitely to be addressed within this PR's scope. The second one "ceph-bluestore-tool resharding failed" and it needs further investigation. Not sure if it's related to this PR. |
Value discard_stop could be uninitialized. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com> (cherry picked from commit bdcc7da)
done |
|
jenkins test make check |
So after checking the logs and talking to Adam I'm pretty sure that my hypothesis is valid - there were two independent issues with the last QA run. The second one is tracked by https://tracker.ceph.com/issues/67911 |
|
@ljflores The issue in #59065 (comment) is caused by: |
|
@ljflores @SrinivasaBharath This might be the reason for Bluestore osd-exapnsion failing https://github.com/rook/rook/pull/15251#issuecomment-2582895014| cc @travisn |
|
Looks like the revert PR #60641 has not been merged yet. So if this is related, we should be able to see the latest squid devel image failing in the resize testing: |
|
Yes its failing with |
|
Hey Ceph maintainers, this change removed While this is mentioned in the changelog, it only includes the PR title which isn't very descriptive. Can this change be rolled back until v20, or be made compatible with the current |
|
This change causing a pretty major performance regression when
The charge from ~0 to ~12 is when I changed from 1 to 4. I'm not the only one seeing this. Here's a few screenshots from some other users:
I can't file an issue on the official bugtracker because account creation requires manual approval. @YiteGu @ifed01 Sorry for the ping but I don't know how else to get this in front of somebody. |
The purpose of |
|
I completely get that the Unless I am missing something here, ceph admins who have previously set [global]
bdev_async_discard = 1will suddenly, without any notice, have async discards disabled on their clusters. Additionally, running This change is breaking compatibility with v19 stable, and without backwards compatibility should have been delayed until v20. |
I completely understand how you feel. It is indeed a bit troublesome during the period of introducing new configurations and canceling old configurations. |
19.2.1 is missing #59529 , causing excessive CPU load during multi-threaded discard, please keep |
I LOVE BREAKING CHANGES! See ceph/ceph#59065 for details
backport tracker: https://tracker.ceph.com/issues/67139
backport tracker: https://tracker.ceph.com/issues/67404
backport of #55469, #56744, #58409
parent tracker: https://tracker.ceph.com/issues/65298, https://tracker.ceph.com/issues/66817
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