Repository navigation
fix(storage): preserve IOPS throttle quota across re-probes - #18
Open
detail-app[bot] wants to merge 2 commits into
Open
detail-app[bot] wants to merge 2 commits into
detail-app[bot] wants to merge 2 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Detail bug report: View on Detail
Bug
The IOPS throttle in
Statistics(foyer-storage/src/io/device/statistics.rs) did not enforce configuredread_iops/write_iops.Metric::throttle()stored the recomputedf64quota asquota as isize, which in Rust truncates toward zero. With per-IO counting, a single IO produces a small fractional negative debt (-0.999…) that rounds to0, wiping the debt on every probe.The production read path (
engine.rs:630) and the write admission path (store.rsviafilter.rs) discard the returnedDurationand re-poll, so the debt was cleared in O(number of probes) instead of O(time/throttle). A configuredread_iops=10admitted ~500/sec under a 1000/sec lookup rate — a ~50× violation — degrading the throttle to a coin-flip gate. Throughput (byte) throttling was largely unaffected because byte debts are large. This regression was introduced when the lock-freeMetric/Statisticsrefactor (PR foyer-rs#1086) replaced the priorMutex-protected,f64-quotaIoThrottler.Fix
AtomicU64off64::to_bits()instead of truncating toAtomicIsize.record()andthrottle()with CAS loops (integerfetch_subis meaningless on bit-encoded floats), keeping the float-quota arithmetic atomic under concurrent probes/records.fetch_maxso it never moves backward under contention.This restores the token-bucket semantics the deleted
IoThrottlerhad.Testing
Committed unit tests (run by default):
test_fractional_negative_debt_preserved_losslessly— a-0.999quota survives a probe and stays negative (guards the root cause directly).test_iops_single_io_debt_survives_rapid_probes— one IO under a 1 iops/s limit stays throttled across 2000 rapid re-probes (the production poll pattern).test_iops_debt_releases_after_refill_window— the throttle releases after the refill window elapses (guards against over-throttle).test_concurrent_record_and_throttle_is_consistent— 8 threads interleavingrecord()/throttle()keep the quota finite and the cumulative counter exact (guards the new CAS loops).test_production_pattern_admission_respects_configured_limit— simulating probe→latency→record, aread_iops=1limit admits far fewer than 1000 rapid attempts.Committed E2E tests (
#[ignore]d wall-clock rate tests against a realFsDevice+ psyncMonitoredIoEngine, following the repo's existing convention of ignoring throttle rate tests):test_e2e_real_read_path_low_limit_enforcedandtest_e2e_real_read_path_high_limit_scales. With the fix, a 10 iops/s limit caps admitted reads to ≤30 over 0.5s and a 100 iops/s limit admits ~50 — the configured rate governs. They also assertdisk_read_ios()equals admitted reads, exercising the real read-completion-callback recording path.Regression-guard check (dev-only, not versioned): I temporarily reintroduced the
as isizetruncation and re-ran the suite — the five unit tests and both E2E tests failed (the admission test reproduced the report's exactgot 500; the E2E 10 iops/s case admitted ~7400 reads in 0.5s). Restoring the fix returned all of them to green, confirming the tests actually catch the bug.Routine checks: unit tests,
cargo check,cargo clippy, andcargo fmt --checkpass for the changed file with no new warnings. The fullio::*suite, thefoyer-storagesuite (includingstorage_fuzzy_test), and the umbrellafoyercrate tests pass with no regressions.cargo clippy -p foyer-storage --all-targets -- -D warningsfails both before and after this change on a pre-existing, unrelateddead_codefield instore.rs; I confirmed viagit stashthat it reproduces on the clean baseline.Automatic Fixes PRs can be configured here.