-
Notifications
You must be signed in to change notification settings - Fork 167
Fix partial cache tail latency by correcting the cache chunk size calc #2176
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
Conversation
…culation Undo temporary chunk size changes Fix temporary variable name change
392a81a
to
f54d16b
Compare
Update existing unit test to have suffix GiB Add comment to clarify cache size is always in GiB
38b8836
to
648f174
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cemakd, mattcary The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Waiting for e2e testing edge cases confirmation /hold |
@cemakd: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
/unhold e2e verification complete, the new chunk size is 2,368 KiB (or ~2.31 MiB) on a cluster with data-cache-count=6 |
/cherry-pick release-1.21 |
@cemakd: new pull request created: #2177 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
The GKE Data Cache was exhibiting extreme tail latency on partially
cached volumes, performing significantly worse than non-cached volumes.
Initial investigation into LVM cache policies and
atime
-inducedwrite-back thrashing did not reveal the root cause.
A series of
fio
benchmarks comparing cached and uncached volumesuncovered a bug where the LVM cache was being configured with a
massive 1 GB chunk size. This was traced to a failure in the
fetchPvSizeGiB
function incache.go
. Thepvs
command syntax wasmalformed, and a subsequent string parsing error on its empty output
caused the driver to fall back to the default
maxChunkSize
of 1 GB.This 1 GB chunk size led to extreme "read and write amplification," where a
single 4KB cache miss would trigger an inefficient 1 GB read from the
backing disk, explaining the catastrophic per-miss penalty.
This change corrects the
pvs
command syntax and the string parsinglogic within the driver. With this fix, the cache is now correctly
configured with a reasonable chunk size (e.g., 384 KiB), which resolves
the extreme read and write amplification and restores performance to expected
levels.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: