feat: IO QoS - #461
Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds per-shard IO QoS budgets for page reads and writes, exposes QoS statistics, updates options and tests, and introduces interference, calibration, and GET2 benchmarking tools with supporting documentation. ChangesPer-Shard IO QoS Budgeting
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant ReadClients
participant EloqStore
participant IoBudget
participant IouringMgr
participant QoSStats
ReadClients->>EloqStore: Issue foreground or background reads
EloqStore->>IoBudget: Acquire page-IO budget
IoBudget->>IouringMgr: Admit page IO or park task
IouringMgr->>IoBudget: Release matching cost on CQE
IouringMgr->>QoSStats: Update counters and timing
QoSStats-->>ReadClients: Report phase QoS results
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/async_io_manager.cpp (1)
171-234: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePotential unsigned underflow in the foreground
reservedcomputation ifbg_inflight_ever exceedsbg_cap_.
reserved = bg_cap_ - bg_inflight_(line 200) assumesbg_inflight_ <= bg_cap_. Given the oversized-request escape hatch (bg_inflight_ != 0guard), a background request withcost > bg_cap_admitted whilebg_inflight_ == 0would pushbg_inflight_abovebg_cap_. With today's call sites this is unreachable (Acquire(1, background)is always called withcost == 1, andbg_cap_ >= 1), so no live bug — but if a future caller ever passescost > 1for a background read,bg_cap_ - bg_inflight_wraps to a huge value,cap_ - reservedthen also wraps, andmust_wait()for foreground silently stops enforcing any cap.Based on learnings,
IoBudget-style admission counters warrant defensive guards against this class of unsigned-arithmetic assumption.🛡️ Proposed defensive fix
const uint32_t reserved = - (bg_cap_ != 0 && !bg_waiting_.Empty()) ? bg_cap_ - bg_inflight_ : 0; + (bg_cap_ != 0 && !bg_waiting_.Empty() && bg_inflight_ < bg_cap_) + ? bg_cap_ - bg_inflight_ + : 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/async_io_manager.cpp` around lines 171 - 234, The foreground reservation calculation in IoBudget::Acquire can underflow if bg_inflight_ ever exceeds bg_cap_, so make the admission logic defensive against oversized background costs. Update the reserved computation inside IoBudget::Acquire and its must_wait() lambda to clamp the background reservation at zero whenever bg_inflight_ >= bg_cap_, and ensure the foreground cap check cannot wrap on unsigned subtraction. Keep the fix localized to IoBudget::Acquire and preserve the existing oversized-request escape behavior for both foreground and background paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmark/CMakeLists.txt`:
- Around line 62-65: The interference_bench target is missing an explicit glog
dependency even though interference_bench.cpp uses LOG/CHECK and eloqstore only
links glog privately. Update the benchmark/CMakeLists.txt target_link_libraries
entry for interference_bench to also link glog::glog directly, alongside
eloqstore and ${GFLAGS_LIBRARY}, so the executable has its own public link to
glog.
In `@docs/design/io_qos.md`:
- Around line 231-239: The paragraph in the io_qos design doc is stale because
it still says max_inflight_write’s real default awaits commit 4, which
contradicts the option block above and the implemented Commit 4 status in the
companion io_qos_impl_plan doc. Update this sentence in the affected paragraph
to reflect that max_inflight_write is already redefined to 512 and shipped, so
the wording matches the current defaults and implementation status. Reference
the max_inflight_write discussion in this section and keep the surrounding
calibration notes unchanged.
In `@include/eloq_store.h`:
- Around line 963-971: GetIoQosStats currently exposes shard IO QoS counters
while the shard thread may still be mutating them, so the plain integer fields
in IoQosStats are being read without synchronization. Update the IoQosStats data
path in eloq_store.h and the GetIoQosStats implementation to use atomics or a
locked snapshot so cross-thread reads are safe, while preserving the existing
invalid-shard/unsupported-manager zero behavior.
In `@src/kv_options.cpp`:
- Around line 166-174: The startup validation still rejects deprecated
max_write_batch_pages values, including 0, even though KvOptions::Load now
treats max_write_batch_pages as deprecated and ignored. Update the check in
EloqStore::InitConfig or the related config validation path in
src/eloq_store.cpp to stop failing on max_write_batch_pages == 0, either by
removing the guard entirely or converting it to a warning so deprecated configs
continue to load.
---
Nitpick comments:
In `@src/async_io_manager.cpp`:
- Around line 171-234: The foreground reservation calculation in
IoBudget::Acquire can underflow if bg_inflight_ ever exceeds bg_cap_, so make
the admission logic defensive against oversized background costs. Update the
reserved computation inside IoBudget::Acquire and its must_wait() lambda to
clamp the background reservation at zero whenever bg_inflight_ >= bg_cap_, and
ensure the foreground cap check cannot wrap on unsigned subtraction. Keep the
fix localized to IoBudget::Acquire and preserve the existing oversized-request
escape behavior for both foreground and background paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3633cce0-44a6-4115-8ad0-053a4d547bf4
📒 Files selected for processing (32)
benchmark/CMakeLists.txtbenchmark/interference_bench.cppbenchmark/opts_interference.inidocs/architecture/02-runtime-and-lifecycle.mddocs/architecture/04-execution-model.mddocs/architecture/07-io-stack.mddocs/architecture/08-data-lifecycle.mddocs/design/io_qos.mddocs/design/io_qos_impl_plan.mdinclude/async_io_manager.hinclude/eloq_store.hinclude/eloqstore_metrics.hinclude/kv_options.hinclude/storage/shard.hinclude/tasks/task.hpython/pyproject.tomlrust/eloqstore-sys/Cargo.tomlrust/eloqstore/Cargo.tomlscripts/io_calibration_sweep.shsrc/async_io_manager.cppsrc/eloq_store.cppsrc/kv_options.cppsrc/storage/shard.cppsrc/tasks/task.cppsrc/tasks/write_task.cpptests/CMakeLists.txttests/batch_write.cpptests/cloud.cpptests/common.cpptests/common.htests/data_page_cache.cpptests/io_qos.cpp
💤 Files with no reviewable changes (1)
- tests/batch_write.cpp
|
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed4fbba94e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
liunyl
left a comment
liunyl
left a comment
There was a problem hiding this comment.
Reviewed the IO-QoS budget core across five lenses (correctness, error handling, test coverage, type design, comment/doc accuracy), with each finding independently verified against the diff. CodeRabbit's four items were re-checked and are folded in.
Bottom line: the production budget accounting is correct. Every acquire/release exit path — success, short-read/write retry, IO error, Abort/AbortWrite, and shutdown — traces as balanced: no leak, double-release, lost-wakeup, or shutdown hang. No merge-blocking correctness bug. The inline comments cluster on one theme — the acquire/release symmetry that keeps the budget correct is guarded only by convention + debug-only asserts + happy-path tests, and a slip is a shard hang, not a cosmetic error — plus one real metrics-wiring defect.
Suggested order:
- Register the three new gauges — they're
Collected but neverRegistered (real defect in theELOQSTORE_WITH_TXSERVICEbuild; OSS CI can't catch it). - Harden + test the acquire/release symmetry: always-on
Releaseguard, structural merged-write cost, and a write-completion-error test. - Add a concurrent FG+BG interference test — the headline anti-starvation claim is currently unpinned (set
reserved = 0and all 11 cases still pass). - Resolve the deprecated-option startup rejection.
Adjudicated — no action needed: the "interference_bench missing glog link" concern is a false positive (eloqstore is a static lib linking glog PRIVATE, which CMake propagates transitively to consumers). The GetIoQosStats cross-thread read is a technical data race but benign, and the production metrics path reads it same-thread (only the benchmark / public API read cross-thread) — so it's not the production race it looks like.
Nice work on the design docs and the exact-counter (not "no-crash") test assertions. Posting as comments — leaving approve/request-changes pending the items above.
Review assisted by automated multi-agent analysis; every finding verified against the code.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/kv_options.h (1)
76-101: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe these budgets in configured page units, not fixed 4KB units.
data_page_sizeis configurable, and merged writes are charged using it. With a non-4KB page size, these comments give operators the wrong unit for calibration.- * `@brief` Per-shard cap on in-flight page-write IO, in 4KB-page units + * `@brief` Per-shard cap on in-flight page-write IO, in data-page units ... - * `@brief` Per-shard cap on in-flight page-read IO, in 4KB-page units + * `@brief` Per-shard cap on in-flight page-read IO, in data-page units🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/kv_options.h` around lines 76 - 101, The max_inflight_write and max_inflight_read documentation incorrectly describes budgets as fixed 4KB-page units. Update both comments to state that limits are measured in configured data_page_size page units, and adjust the merged-write wording to consistently reference that configured size while preserving the existing QoS sizing guidance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmark/eloq_store_bm.cc`:
- Around line 657-667: Update the GET2 request lifecycle around the submission
loop and its completion-drain logic to track successfully submitted requests
independently of issue failures: increment the outstanding count after each
successful submission and decrement it for every completion. Stop issuing new
requests once shutdown begins, and do not return from the benchmark until
outstanding reaches zero, or cancel requests and await callback quiescence so
the local ops objects remain valid.
- Around line 608-635: Update the shard-selection logic before ExecAsyn so
per_shard_cap is enforced against the actual routed shard, using
TableId().ShardIndex(...) rather than modulo arithmetic. Replace the fixed
eight-retry fallback with capacity waiting or resampling until an uncapped
target is available, then assign op->shard_ from the canonical routing result
and increment the matching shard_out entry.
- Around line 651-655: Update the GET2 completion path around
ReadRequest::Error() to check the completed read before recording latency or
incrementing completed_. Exclude failed and missing-key reads from successful
results, and handle them through the existing failure/reporting path while
preserving shard accounting and request reissue behavior.
In `@benchmark/main.cpp`:
- Around line 39-42: Ensure the GET2 benchmark rejects zero values for
client_threads and inflight_per_client instead of running with no work. Add
gflags validation for both DEFINE_uint32 options, or add equivalent fail-fast
checks at the start of RunGet2(), while preserving normal behavior for positive
values.
In `@src/storage/shard.cpp`:
- Around line 214-237: Adjust the SLOWROUND threshold calculation around
dequeue_requests() so it excludes time spent waiting for queued requests.
Measure or derive the active processing interval using timestamps surrounding
the processing work, while preserving the existing SLOWROUND breakdown and
logging for genuinely slow rounds. Update the threshold check in the enclosing
shard loop, not dequeue_requests() itself.
---
Outside diff comments:
In `@include/kv_options.h`:
- Around line 76-101: The max_inflight_write and max_inflight_read documentation
incorrectly describes budgets as fixed 4KB-page units. Update both comments to
state that limits are measured in configured data_page_size page units, and
adjust the merged-write wording to consistently reference that configured size
while preserving the existing QoS sizing guidance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcd5fcc6-6e54-417b-bd10-5394f3e6ebd1
📒 Files selected for processing (16)
benchmark/eloq_store_bm.ccbenchmark/eloq_store_bm.hbenchmark/main.cppdocs/design/io_qos.mdinclude/async_io_manager.hinclude/eloq_store.hinclude/kv_options.hinclude/storage/shard.hinclude/tasks/task.hinclude/utils.hsrc/async_io_manager.cppsrc/eloq_store.cppsrc/kv_options.cppsrc/storage/object_store.cppsrc/storage/shard.cppsrc/tasks/read_task.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- src/kv_options.cpp
- include/tasks/task.h
- docs/design/io_qos.md
- include/eloq_store.h
- include/async_io_manager.h
- src/async_io_manager.cpp
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/io_qos_impl_plan.md`:
- Around line 151-167: Remove or revise the wake-policy bullet near the
`WaitingZone::WakeN` description so it no longer prescribes waking foreground
waiters first; document the demand-gated reservation behavior instead, including
that queued background demand reserves unused entitlement and wake credits are
forwarded appropriately.
In `@docs/design/io_qos.md`:
- Around line 354-360: Update the “Segment IO (zero-copy large values) is not
budgeted” section to replace the undefined max_inflight_io reference with the
documented max_inflight_read and max_inflight_write budgets, clearly stating
that segment IO is exempt from both while preserving the existing compaction
bounds.
- Around line 137-145: Add language identifiers to the fenced Markdown blocks in
the IO QoS design document: mark the pseudocode around acquire/release with text
and the option-declaration block at the referenced additional section with cpp,
preserving their contents unchanged.
- Around line 186-209: Update the read admission and wake-up logic described in
the IO QoS design so background demand remains visible from wake time until the
waiter successfully acquires its budget unit. Charge or reserve the unit when
waking a background waiter, or track that waiter as pending until admission
completes, ensuring foreground admission cannot consume the reserved capacity
during this interval; preserve FIFO ordering and the existing re-wait behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81af2c5a-f57a-4cc9-b399-cfa2036d0e20
📒 Files selected for processing (36)
benchmark/CMakeLists.txtbenchmark/eloq_store_bm.ccbenchmark/eloq_store_bm.hbenchmark/interference_bench.cppbenchmark/main.cppbenchmark/opts_interference.inidocs/architecture/02-runtime-and-lifecycle.mddocs/architecture/04-execution-model.mddocs/architecture/07-io-stack.mddocs/architecture/08-data-lifecycle.mddocs/design/io_qos.mddocs/design/io_qos_impl_plan.mdinclude/async_io_manager.hinclude/eloq_store.hinclude/eloqstore_metrics.hinclude/kv_options.hinclude/storage/shard.hinclude/tasks/task.hinclude/utils.hscripts/io_calibration_sweep.shsrc/async_io_manager.cppsrc/eloq_store.cppsrc/kv_options.cppsrc/storage/object_store.cppsrc/storage/shard.cppsrc/tasks/read_task.cppsrc/tasks/task.cppsrc/tasks/write_task.cpptests/CMakeLists.txttests/batch_write.cpptests/cloud.cpptests/common.cpptests/common.htests/data_page_cache.cpptests/io_qos.cpptests/large_value_benchmark.cpp
🚧 Files skipped from review as they are similar to previous changes (28)
- docs/architecture/08-data-lifecycle.md
- src/kv_options.cpp
- src/storage/object_store.cpp
- src/tasks/read_task.cpp
- benchmark/CMakeLists.txt
- include/eloqstore_metrics.h
- include/tasks/task.h
- tests/common.cpp
- docs/architecture/02-runtime-and-lifecycle.md
- include/eloq_store.h
- src/tasks/task.cpp
- src/storage/shard.cpp
- benchmark/opts_interference.ini
- src/eloq_store.cpp
- tests/batch_write.cpp
- scripts/io_calibration_sweep.sh
- src/tasks/write_task.cpp
- tests/data_page_cache.cpp
- tests/large_value_benchmark.cpp
- include/storage/shard.h
- include/kv_options.h
- tests/cloud.cpp
- docs/architecture/07-io-stack.md
- benchmark/eloq_store_bm.cc
- include/async_io_manager.h
- tests/io_qos.cpp
- benchmark/interference_bench.cpp
- src/async_io_manager.cpp
|
Rebased onto current Latest verification:
Local-NVMe performance campaign (
Important limitation: control and candidate used the same Release binary and both had Against the clarified acceptance target of read p99.9 below 10 ms during concurrent GC, this result is FAIL. The PR description now records the exact workload, results, limitations, unrun checks, compatibility risk, and rollback. |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmark/eloq_store_bm.cc`:
- Around line 633-661: Keep key_index and part consistent in the GET2 selection
flow: when capacity forces a different partition, resample a key mapped to the
selected partition or regenerate a key that belongs there before generate_key.
Ensure op->shard_, TableIdent, and op->key_ all represent the same partition,
while preserving the existing per-shard-cap selection behavior.
- Around line 738-746: Correct the percentile index calculation in the pct
lambda so percentile ranks use the intended zero-based position rather than p *
all.size(), ensuring p99 for 100 samples selects the 99th sample while
preserving empty-input handling and bounds protection.
In `@docs/design/io_qos_impl_plan.md`:
- Around line 120-121: The WriteReqPool sizing description in the IouringMgr
constructor section incorrectly equates the pool bound with the
max_inflight_write QoS limit. Rewrite it to state that the pool is
conservatively allocated using max_inflight_write while counting request
objects, whereas QoS is measured in page units and merged writes may consume
multiple pages.
- Around line 318-330: Update the “Performance acceptance” section to state the
supplied objective of read p99.9 below 10 ms during concurrent GC, and record
the campaign’s reported failure status. Replace the generic idle-relative
criterion while preserving the throughput regression guards and sweep
definitions, and ensure the release-readiness wording does not imply the target
passed.
In `@src/storage/shard.cpp`:
- Around line 264-268: Update the CPU duration calculation in the shard timing
code around cpu0, cpu1, and cpu_us to normalize the timespec subtraction before
converting to uint64_t: borrow one second when cpu1.tv_nsec is less than
cpu0.tv_nsec, then compute the normalized seconds and nanoseconds totals so
cross-second measurements produce the correct cpu_us value.
In `@tests/eloq_store_test.cpp`:
- Line 50: Update the fixture setup around CreateTestDir in the relevant test to
use a helper that creates the directory beneath the canonical /tmp/eloqstore or
/tmp/test-data root, preserving the existing "_qos_options" fixture name.
In `@tests/io_qos.cpp`:
- Around line 163-166: Bound the completion wait loop in the asynchronous I/O
test by adding a deadline or timeout, then assert that done reaches num_parts
before the limit; preserve the existing short sleep while waiting and fail the
test when completion does not occur in time.
- Around line 595-604: Update the wait_until predicate in the foreground priming
setup to remove the fg_done.load(std::memory_order_relaxed) == 0 requirement.
Keep the fg_started, inflight-read, and blocked-count checks unchanged so
priming still requires saturated contention without assuming no reads have
completed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f3e8445-ecf1-493e-b4ba-2a6692452c8b
📒 Files selected for processing (40)
CLAUDE.mdbenchmark/CMakeLists.txtbenchmark/eloq_store_bm.ccbenchmark/eloq_store_bm.hbenchmark/interference_bench.cppbenchmark/main.cppbenchmark/opts_interference.inidocs/architecture/02-runtime-and-lifecycle.mddocs/architecture/04-execution-model.mddocs/architecture/07-io-stack.mddocs/architecture/08-data-lifecycle.mddocs/design/io_qos.mddocs/design/io_qos_impl_plan.mdinclude/async_io_manager.hinclude/eloq_store.hinclude/eloqstore_metrics.hinclude/fail_point.hinclude/kv_options.hinclude/storage/shard.hinclude/tasks/task.hinclude/types.hinclude/utils.hscripts/io_calibration_sweep.shsrc/async_io_manager.cppsrc/eloq_store.cppsrc/kv_options.cppsrc/storage/object_store.cppsrc/storage/shard.cppsrc/tasks/read_task.cppsrc/tasks/task.cppsrc/tasks/write_task.cpptests/CMakeLists.txttests/batch_write.cpptests/cloud.cpptests/common.cpptests/common.htests/data_page_cache.cpptests/eloq_store_test.cpptests/io_qos.cpptests/large_value_benchmark.cpp
💤 Files with no reviewable changes (1)
- tests/batch_write.cpp
🚧 Files skipped from review as they are similar to previous changes (23)
- tests/CMakeLists.txt
- tests/common.h
- src/storage/object_store.cpp
- docs/architecture/07-io-stack.md
- include/eloqstore_metrics.h
- benchmark/opts_interference.ini
- docs/architecture/04-execution-model.md
- benchmark/eloq_store_bm.h
- include/utils.h
- src/kv_options.cpp
- src/tasks/task.cpp
- src/tasks/read_task.cpp
- include/tasks/task.h
- include/kv_options.h
- src/tasks/write_task.cpp
- tests/cloud.cpp
- tests/common.cpp
- tests/data_page_cache.cpp
- benchmark/interference_bench.cpp
- scripts/io_calibration_sweep.sh
- src/eloq_store.cpp
- include/eloq_store.h
- src/async_io_manager.cpp
Local NVMe IO-QoS test reportThis report records the local performance campaign run on 2026-07-16 UTC for Executive resultThe candidate improved mixed-workload read p99 consistently, but it did not
The candidate improved p99 in 5/5 paired runs and p99.9 in 4/5, but max latency The control was the same tested binary with read QoS disabled Provenance
The runner checked the commit and binary hash before every workload. Configure, Preservation caveat: the exact tested binary was not copied into the artifact Machine setupThe following OS/mount facts were captured before the campaign on July 16.
No CPU pinning, IRQ isolation, explicit governor setting, cooldown, or thermal Local NVMe and filesystem
The ext4 mount did not use continuous discard. One guarded Disk calibrationThe disk curve was measured after the EloqStore campaign to characterize the Exact command: TMPDIR=/mnt/local_nvme1/eloqstore-pr461-task7-20260716T094039Z/fio \
timeout 600 bash scripts/io_calibration_sweep.sh \
--dir /mnt/local_nvme1/eloqstore-pr461-task7-20260716T094039Z/fio/calibration \
--size 8G \
--step-secs 20 \
--read-qd 32 \
--rates "0 50 100 200 400 800 1200" \
--preconditionBefore measuring, each separate 8 GiB read and write file received two full Each 20-second measurement used:
The script's CSV header says MB/s, but it divides bytes by
The compact curve shows a read-p99/IOPS knee between 100 and 153 actual This is not a standalone pure-write ceiling because the QD32 random reader The separate io_uring runtime probe used 4 KiB random mixed I/O, QD4, one job,
EloqStore configurationEach run used a unique fresh store on [run]
num_threads = 4
buffer_pool_size = 400MB
fd_limit = 5000
num_retained_archives = 0
skip_verify_checksum = true
max_inflight_read = 0 # control
# max_inflight_read = 64 # candidate
bg_read_ratio = 25
max_inflight_write = 512
[permanent]
store_path = /mnt/local_nvme1/eloqstore-pr461-task7-20260716T094039Z/runs/<run>/store
data_page_size = 4KB
data_file_size = 8MB
data_append_mode = trueThis means the candidate always enforced both the read and write caps, while Exact primary workloadulimit -n 65536
timeout --signal=TERM --kill-after=30s 600 \
/usr/bin/time -v interference_bench \
--kvoptions=<unique-config> \
--partitions=8 \
--keys_per_partition=20000 \
--val_size=3000 \
--read_concurrency=32 \
--baseline_secs=60 \
--storm_secs=180 \
--storm_ratio=5 \
--storm_span=3 \
--storm_batch_keys=2048 \
--write_read_ratio=0 \
--load=true \
--logtostderr=trueWorkload details:
The campaign also ran a candidate smoke test (4,096 keys/partition, QD32, Primary per-run resultsAll latencies are microseconds. Rows are in execution order.
Additional median I/O evidence:
Candidate p99 improved in 5/5 pairs; p99.9 improved in 4/5; max improved in Control per-class read counters are zero by design when QD128 sensitivity diagnosticOne candidate-then-control pair used the same data/write geometry with QD128,
This was single-shot, candidate-first, and the control idle p99 was anomalous, Validity checks
Limitations and interpretation
Retained artifactsAll raw logs and analysis outputs are retained under: Important files include 2026-07-17 follow-up: does the 512-page write cap actually constrain writes?The first campaign compared two conditions that both used Provenance and setup
Common workload: The only condition changes were:
Validity
Per-run resultsLatency is in microseconds. Device rates are the final 120 one-second
Condition medians:
Median relative changes:
InterpretationThe 512-page cap is transiently binding (HWM pinned at 512 and positive At the same write cap, enabling read/BG admission improved median p99 by 9.68% Because the measured benefit does not justify an unconditional write cap, the This remains an |
liunyl
left a comment
liunyl
left a comment
There was a problem hiding this comment.
Re-reviewed the latest PR head (7bf9d16) against merge-base 8963b7c with the full review toolkit. The main remaining risks are incorrect write-IOPS accounting, a default policy that cannot satisfy the documented pure-write no-regression guard, token-bucket edge cases, and acceptance tooling that can produce misleading results. All inline findings below were revalidated on the current diff; this is a COMMENT review and does not change the PR approval state.
| // background buckets — the foreground debit is unreachable by | ||
| // construction, not by luck. | ||
| assert(Positive(true)); | ||
| bg_ops_bal_ -= ops_cost; |
There was a problem hiding this comment.
[P1] Do not debit disabled token dimensions
The default bytes rate is zero, so Positive() ignores the bytes balance and refill never updates it, but every background I/O still subtracts bytes_cost here (and the foreground/borrow paths do the same). Because balances are scaled by 1e6, a normal long-running shard reaches signed int64_t overflow after roughly 9.2 TB in that class, which is undefined behavior. The symmetric problem exists for the ops balance in bytes-only mode. Debit a balance only when its corresponding rate is enabled, including lender-bucket debits.
| // background gets ratio percent, foreground the rest. Clamped so both | ||
| // classes always have a nonzero share when the budget is enabled. | ||
| const uint32_t ratio = std::clamp<uint32_t>(bg_ratio_pct, 1, 99); | ||
| fg_ops_rate_ = ops_per_sec * (100 - ratio) / 100; |
There was a problem hiding this comment.
[P1] Keep nonzero configured rates from becoming disabled classes
Integer truncation contradicts the comment that both classes retain a nonzero share: at 1 op/s per shard both rates become zero, and at 2 op/s with the default ratio the background rate is zero. A zero rate means “bucket disabled,” so low limits, many shards, or an extreme ratio can silently disable QoS or leave background writes unlimited. Reject configurations whose derived rate cannot fund both classes, or allocate nonzero shares while preserving a well-defined total; add boundary tests around the smallest supported per-shard rates.
| if (reader.HasValue(sec_run, "rate_limit_burst_ms")) | ||
| { | ||
| rate_limit_burst_ms = | ||
| reader.GetUnsigned(sec_run, "rate_limit_burst_ms", 4); |
There was a problem hiding this comment.
[P2] Preserve the declared defaults for malformed M4 options
An invalid rate_limit_burst_ms falls back to 4 although the member default is 2, while malformed rate_limit_io_unit parses to 0 and later behaves as a 4 KiB unit instead of the documented 2 KiB default. A typo therefore silently changes the QoS policy. Use the existing member/default value as the fallback (or reject the INI), assign the I/O unit only after a successful nonzero parse, and extend the malformed-default test to all new M4 knobs.
| static_cast<size_t>(p * static_cast<double>(all.size() - 1)); | ||
| return all[idx]; | ||
| }; | ||
| LOG(INFO) << "GET2 finished: clients=" << client_threads |
There was a problem hiding this comment.
[P1] Fail the latency benchmark when requests fail
RunGet2 only logs read_failures/issue_failures and returns normally; percentiles are calculated from surviving reads, and an empty sample set is reported as all-zero latency. Request rejection, I/O errors, or zero successful samples can therefore make a broken run look faster while the process still exits successfully. Return a failure status when either counter is nonzero or no samples were collected, and propagate it to a nonzero benchmark exit.
| skip_verify_checksum = true | ||
|
|
||
| # --- IO QoS knobs under test --- | ||
| max_inflight_read = 64 |
There was a problem hiding this comment.
[P1] Make the committed interference benchmark exercise M4 QoS
This acceptance config still sweeps max_inflight_read and bg_read_ratio, which are deprecated no-ops, plus max_inflight_write, which now only sizes request pools. interference_bench.cpp also logs only those old fields. Running the repository-provided command can therefore claim a QoS A/B comparison without changing the M4 limiter. Replace these with disk_rate_limit_iops/mbps, rate_bg_ratio, rate_limit_burst_ms, rate_limit_io_unit, and max_inflight_io, and print the exact effective values at startup.
| // Positive() and the large-cost debt path (a merged write can exceed | ||
| // one burst of byte tokens). | ||
| eloqstore::KvOptions opts = append_opts; | ||
| opts.disk_rate_limit_mbps = 8; |
There was a problem hiding this comment.
[P2] Actually disable IOPS in the bytes-only test
append_opts inherits the nonzero default disk_rate_limit_iops = 275000; setting only disk_rate_limit_mbps means this test enables both buckets despite claiming to cover the ops-disabled branch. A broken bytes-only configuration could still pass these admitted-counter assertions. Set opts.disk_rate_limit_iops = 0 and assert that the deliberately small byte budget actually blocks background writes.
| // The M1/M2 count budgets (and their per-class in-flight gauges) are | ||
| // retired in favor of the M4 rate budget (docs/design/io_qos.md). The | ||
| // closest surviving instantaneous-depth gauge is the class-blind | ||
| // in-flight device-command window; report it under the read-pages |
There was a problem hiding this comment.
[P2] Do not publish class-blind command depth as read pages
The PR registers read, background-read, and write in-flight page gauges, but only collects the “read” gauge and fills it with io_window_inflight_, which includes writes and uses device-command units for merged I/O. Dashboards will show background/write permanently zero and attribute writes to reads; with the window disabled the metrics are all zero even while rate QoS is active. Replace these names with accurate window/rate metrics or remove the obsolete gauges until real per-class values exist.
| - **Page I/O** — `ReadPage`/`ReadPages` (batched, into pool buffers, fixed | ||
| reads when the buffer is registered), `WritePage`. `ConvFilePageId` splits a | ||
| `FilePageId` into `(file_id, offset)` by `pages_per_file_shift`. | ||
| - **In-flight page-IO budgets** (`IoBudget`, see `docs/design/io_qos.md` |
There was a problem hiding this comment.
[P2] Keep the maintained architecture docs synchronized with M4
This new current-state section documents the removed IoBudget, deprecated no-op read knobs, completion-driven releases, and retired per-class gauges as active behavior. The same stale model appears in 02-runtime-and-lifecycle.md, 04-execution-model.md, and 08-data-lifecycle.md, while the design document's opening summary also still calls M1/M2 current. That sends maintainers toward ineffective configuration and the wrong wake/release invariants. Update the maintained docs to describe refill-driven RateBudget, asymmetric class shares, and the optional class-blind max_inflight_io window; keep M1/M2 only as clearly labeled history.
…d isolation Background tasks (batch writes, compaction, GC) degraded foreground read tail latency even after the CPU-side yield work (eloqdata#455): a background task scheduled for one 20us slice could still burst 128 page reads plus writes into io_uring, and nothing distinguished them from foreground reads at the ring or device. Reads had no budget at all; writes only a per-task one. CPU priority was lost at the IO boundary. Design: docs/design/io_qos.md (+ implementation/testing record in docs/design/io_qos_impl_plan.md). Mechanisms (per shard, all in IouringMgr): - IoBudget: in-flight page-IO counters with independent read/write caps (max_inflight_read / max_inflight_write, 4KB-page units; merged append writes cost bytes/page_size). Acquired immediately before SQE prep — after every other blocking acquisition — and released per CQE in PollComplete via new user-data types (KvTaskPageRead/BaseReqPageRead) that distinguish budgeted page reads from metadata ops. Batched reads acquire per page, so a 128-page compaction batch cannot deadlock against a small budget. A request costlier than the cap is admitted alone once the budget drains (merged writes vs small caps). Metadata, manifest, and segment IO are exempt. - Background read sub-budget (bg_read_ratio, KvTask::IsBackground()): background page reads are additionally bounded so compaction/GC/batch- write read bursts cannot crowd foreground point reads out of the device queue. While background waiters queue, their unused entitlement is reserved from new foreground admissions — sustained foreground saturation cannot starve compaction (and vice versa); release wakes background first and forwards unused wake credits (WaitingZone::WakeN now returns the count woken). The ratio parameterization is deliberate: with the total cap sized to the device's bandwidth-delay product, relative tail inflation is device-independent (see Sizing contract in the design doc). - Retired: the per-task max_write_batch_pages drain-to-zero throttle (option deprecated, parsed with a warning); max_inflight_write is now the write QoS cap (default 32768 -> 512, calibrated: natural demand ceiling was 2048 = 8 concurrent 1MB merged writes; 512 binds marginally at equal-or-best throughput and tails). Defaults max_inflight_read = 32, bg_read_ratio = 25 from interference sweeps (bg cap lands on the measured background demand line; write-throughput knee only below it). - Observability: IoQosStats (in-flight/high-watermark/blocked/admitted per budget + bg slice, fdatasync count/latency), EloqStore::GetIoQosStats, TXSERVICE gauges. Benchmarks/tooling: benchmark/interference_bench (baseline vs 90/10 write/read mixed phase; rotating strided partial-overwrite storm so compaction continuously relocates pages — full overwrites generate no move reads; exact percentiles; per-shard QoS deltas) and scripts/io_calibration_sweep.sh (fio read-tail-vs-write-rate curve). On the dev box the sub-budget cut mixed-phase read p99 ~2.5x and p99.9 3-5x at identical write throughput, with flat p50. Tests: tests/io_qos.cpp (11 cases: accounting invariants, oversized batches through tiny caps, BG sub-budget bounds under compaction, concurrent merged-write admission, failed-write budget drain, shutdown while blocked on a budget). Full suite green; db_stress 120/120 with tiny caps in both write modes; ASAN (flag-injection mode) clean. Two OOM tests that implicitly relied on the retired per-task drain to bound write-promotion pins now set explicit write budgets. Also: test harness now enforces one started EloqStore per process (InitStore gained cleanup=false for warm-restart tests; cloud tests no longer construct stores directly) — fixes a flaky teardown SIGSEGV where a test-local store Stop nulled the global eloq_store pointer that the lingering singleton prewarm shutdown later dereferenced. Deployment notes: max_inflight_write semantics changed for configs that set it explicitly (was pool sizing only, now a queue-depth cap); max_write_batch_pages is ignored. Real-device calibration should re-derive max_inflight_read (QD sweep); bg_read_ratio is policy and transfers. The M3 background write rate limiter remains deferred pending real-device evidence.
…skips them; select the [benchmark] tag to run them explicitly.
read budget to 64; add IO stage-timing instrumentation and GET2 bench Two behavioral fixes found during the Azure NVMe calibration campaign (2026-07-11), one retuned default, and supporting instrumentation: - IoBudget::Release: always wake the foreground class, not only with leftover credits. Under a write storm, background forms a saturated treadmill whose wait-queue never empties, so every released credit re-donated to background; once the last foreground in-flight's credit landed in a background dip, the foreground class had no remaining wake source until background drained — observed as multi-second foreground gate stalls (p999 120–312 ms, gate waits up to 3.7 s). Over-waking is safe: woken tasks re-check the admission condition and re-wait. - IouringMgr::IsIdle: track in-flight SQEs (prepared minus reaped) and report non-idle while any remain. The base-class IsIdle let shards sleep up to the 100 ms request-wait timeout with CQEs pending, which stalls delivery under DEFER_TASKRUN when the pending IO is not owned by an active task (observed during prewarm). CloudStoreMgr::IsIdle now composes the base check. - Raise default max_inflight_read 32 -> 64 (header + INI fallback). Azure local-NVMe calibration put the knee at c ~= 5-7 x BDP: 64–128 indistinguishable, while 32 showed genuine foreground queueing under 128 concurrent readers — the budget must also cover peak per-shard foreground concurrency, not only the BDP. bg_read_ratio stays 25; docs/design/io_qos.md records the calibration and warns that ratio x cap combinations yielding bg_cap < ~8 throttle ingest itself (batch-write RMW page fetches ride the background class). - Opt-in stage-timing instrumentation, gated by ELOQ_IO_STATS=1 at runtime (off: one cached-bool branch per site): per-request enqueue/dequeue stamps, read-path stage breakdown (budget gate, SQE->CQE, CQE->resume, queue wait, start lag, index walk), per-loop phase timing with SLOWROUND reports for >1 ms rounds, and 5 s opstages/loopstats VLOG(1) summaries with CQE reap-batch histogram. - benchmark: new GET2 mode — dedicated client threads each holding --inflight_per_client async reads, optional --per_shard_cap to bound a stalled shard's blast radius; reports QPS and latency percentiles. - object_store: suppress libcurl's default form-urlencoded Content-Type on presigned-URL uploads; it is outside the signature (SignedHeaders=host) and strict validators (s3proxy) reject it. Also: Shard::ReadTimeMicroseconds is now static (callers need no instance).
Preserve a fully reserved background read slice across the wake-to-admit gap and cover all page-read CQE error paths. Expose stable QoS stats, harden option parsing and benchmark validity, and add deterministic accounting and scheduler regressions.
InitializeTscFrequency divided elapsed cycles by the REQUESTED 1ms sleep, but sleep_for reliably oversleeps by scheduler latency (~60us for a 1ms request). That inflated cycles-per-microsecond by ~6%, making every TSC-derived duration run ~6% slow: all timing gauges (budget blocked_us, IO stage timings) underreported by that factor, and the M4 rate budget's refill delivered exactly 94.1% of its configured rate — measured as a constant deficit across every load level, ratio, and budget until the cause was found. The systematic overshoot also defeats the calibration's stability check: consecutive measurements agree with each other while both being wrong. Divide by CLOCK_MONOTONIC-measured elapsed time instead. After the fix, delivered rate is within ~1% of configured. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ing (M4) Cloud local NVMe is a provisioned rate limit, not a device: the test disk (Azure v2 direct disk) enforces ~275K IOPS and holds overflow IOs in quantized multi-ms delays (measured: read-only p99.9 = 3.3ms AT the ceiling with a bimodal 90us/3ms distribution and an empty middle; deep-queue fio reproduces the plateau at the same 275,328 ceiling, while sub-ceiling operation gives ~400us tails even under 400 MB/s of concurrent writes — the clean/throttled boundary sits at 100.5% of the fitted budget). Concurrency caps cannot express this: the count that holds the rate at the ceiling is rate x latency, a knife-edge that drifts with the workload mix. The engine must own the queue in the rate dimension so waiting happens in user space — FIFO, class-aware — instead of in the hypervisor's limiter. RateBudget: per-shard token buckets (ops + bytes) refilled lazily from the shard TSC clock once per event-loop iteration; debt admission (wait until positive, charge the full cost, let the balance go negative) makes the long-run rate exact and admits any single IO larger than the bucket without deadlock. The budget is PARTITIONED by class — foreground refills at (100 - rate_bg_ratio)%, background (background-task reads and all write-path IO) at rate_bg_ratio% — a shared balance was tried and rejected (one merged write's debit stalled every foreground read ~0.5ms/MB). Foreground alone may borrow background's surplus while background has no waiters and a positive balance, with the debit landing on the lender; symmetric borrowing was tried and reverted (storm-driven background skimmed ~2M ops/shard through microsecond foreground-idle windows: foreground 183K -> 116K QPS, p99.9 720us -> 5.6ms). Admission is peek-and-grant: waiters record their cost, the refill charges the FIFO head on its behalf and only then wakes it — exact wake counts for heterogeneous costs, no over-waking, no re-queue churn. New options: disk_rate_limit_iops (default ON at 275,000 per disk — a cloud-NVMe starting point; per-shard budget = iops x store paths / num_threads, multiple paths assumed identical devices; set ~95% of the fio-measured ceiling for precision; 0 disables), rate_bg_ratio (25), rate_limit_io_unit (2KB: the measured hypervisor accounting currency — a written 4KB costs two read units; >=16KB units measurably leak throttling under write load), rate_limit_burst_ms (2: the window only reshapes the latency distribution — smaller flattens median-up/ tail-down; 1/2/4ms cost no throughput), disk_rate_limit_mbps (0: only with a measured write-bandwidth ceiling), and max_inflight_io (0 = off: a single class-blind in-flight device-command window kept as a safety bound — measured inert on Azure, whose limiter charges rate, not instantaneous depth). The M1/M2 count budgets are retired: IoBudget is deleted, max_inflight_read and bg_read_ratio are parse-only deprecation no-ops, and max_inflight_write reverts to write request-pool sizing (32768). Under the partitioned rate budget the tuned count caps changed storm p99.9 by nothing measurable (709 vs 722us), and the write cap could never bind below one merged write buffer anyway. Validation (Azure Standard-L VM, single local NVMe, 4 shards, interference_bench, full ladders in docs/design/io_qos.md): read-only at QD32 budget sweep 200-260K all give p99.9 418-578us vs 3,392us uncapped (8.1x at 95% of ceiling, -11% QPS); with the storm, foreground reads hold ~205K QPS at p99.9 875us vs ~7ms unmanaged; at QD128 the unmanaged 6.9-7.7ms hypervisor plateau becomes an orderly ~2.6ms fair queue; rate_bg_ratio is exactly linear in both foreground QPS and background write MB/s at both depths with no floor down to 10%. docs/design/io_qos.md records the full design, calibration procedure, and the measured reasons each rejected alternative was rejected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…docs) Review findings on the M4 commit, verified against the code and fixed: - Write currency is the physical 4KB page quantum, not 2KB. The old SubmitMergedWrite clamp (max(rate_limit_io_unit, data_page_size)) silently floored the 2KB default at 4KB, so the "2KB accounting" never ran; the read-tail target was met at the effective 4KB. Default and docs now say 4KB, the clamp is removed, and one WriteRateOps helper meters WritePage and SubmitMergedWrite (unit-tested 4KB->1, 1MB->256). - RateBudget::Charge debits a balance only when its dimension is enabled. Previously a disabled dimension (e.g. the bytes bucket at the default disk_rate_limit_mbps=0) was debited every IO but never refilled, driving it to int64 overflow (~9TB) on a long-lived shard. - SetRates splits via SplitRate, keeping both class rates nonzero for any enabled dimension so integer truncation at low per-shard rates cannot silently disable a class (unbudgeted background writes / lost read protection). - Pure-write workloads run at rate_bg_ratio of the device rate by design (all writes are background, no reverse lending); documented as an accepted product decision in io_qos.md and 08-data-lifecycle.md. - benchmark/opts_interference.ini and the committed acceptance path now drive the M4 knobs, not the deprecated count knobs (which changed nothing). GET2 exits nonzero on request failures or zero samples. - Malformed M4 options fall back to their member defaults (burst_ms) and reject a zero io_unit rather than silently changing policy. - The retired per-class in-flight page gauges are unregistered rather than reporting the class-blind command window mislabeled as read pages. - io_qos.cpp: bytes-only test disables IOPS to actually cover the ops-disabled path; the borrow test forces demand with a concurrent overflow read instead of a device-speed-dependent sequential stream. - Architecture docs (07-io-stack, 02-runtime-and-lifecycle, 04-execution-model, 08-data-lifecycle) and the io_qos.md summary now describe the M4 RateBudget instead of the retired M1/M2 count budgets. Full local suite green (325/325). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…take Perf testing uncovered a scheduling fairness gap: new KV requests and resumed tasks flowed through two different paths. StartTask ran each dequeued request's first coroutine segment inline during intake (up to 128 per loop iteration, unconditionally), while tasks resumed by IO completions or rate-budget grants waited in ready_tasks_ for the scheduler's bounded window (max_processing_time_microseconds). Under sustained load the loop therefore preferred starting new work over finishing in-flight work; the ready queue backlog grew and mid-flight tasks - including peek-and-grant wakes already holding charged rate tokens - were progressively deferred, inflating tail latency. StartTask now creates the coroutine SUSPENDED: the body hands control straight back to the creator (the same continuation handoff Yield uses) before touching the request lambda, and the task is enqueued into ready_tasks_. ExecuteReadyTasks becomes the single scheduling point, so new and resumed work runs in true arrival order under one time budget. The request lambda stays captured in the coroutine frame - no type erasure, no KvTask API change; a ready task is uniformly "just resume". Consequences kept intact by construction: - ProcessReq's false-return contract is unchanged (task-acquisition failures happen before StartTask); per-table write serialization and the reopen paths are untouched. - The idle wait cannot false-trigger: a created task counts in TaskManager::NumActive(). - The TXSERVICE request-latency clock is captured at creation (it used to be read inside the body, which was the same instant only because creation and first execution coincided), so measured durations still include the ready-queue wait. - cur_resume_start_us_ stamping moves to ExecuteReadyTasks' resume, which already does it; the two-instruction prologue needs no mark. Cost: one extra continuation switch per request and one loop iteration of added first-segment latency, in exchange for arrival-order fairness under load. Full suite green (327/327). Tail-latency validation on the perf VM pending (offline). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The shard loop ran ExecuteReadyTasks() last, so SQEs it prepared were not handed to the kernel until the NEXT round's Submit(). In module mode (the embedding runtime drives Process/HasTask) the next round is an external scheduling decision, so every IO hop paid a full scheduling quantum of dead device time. The fairness fix (7376b7e) made this worse for new requests: StartTask now only enqueues, so a request crossed two round boundaries before its first IO reached the device instead of one. Reorder every round implementation to Submit -> PollComplete -> Promote -> intake -> ExecuteReadyTasks -> Flush so all four producers into ready_tasks_ (rate-budget grants from Submit's RefillAndWake, IO completions, delayed reopens, new requests) land before the single ExecuteReadyTasks, and the SQEs it prepares are issued before the thread is handed back. WorkOneRound already dequeued first, so module mode needed only the trailing flush. Submit() keeps the top-of-round slot deliberately: it owns RefillAndWake (the rate budget's only wake source) and the kernel entry that delivers CQEs, which PollComplete cannot do itself under IORING_SETUP_DEFER_TASKRUN. So the flush is a separate FlushSubmit() rather than a second Submit(), keeping the refill at exactly one per round; it no-ops when the round prepared nothing and leaves consecutive_skipped_submits_ untouched so the DEFER_TASKRUN forced-enter safety net stays owned by Submit. Measured (Azure L-series, 8 partitions / 4 shards, QD128 storm, 4 interleaved rounds per arm): a wash, as expected for standalone mode where rounds are microseconds apart and foreground is pinned to the rate-budget cap. Median 205,851 vs 205,853 QPS, p99 2285 vs 2281 us, p99.9 2636 vs 2590 us (ranges overlap), writes 73 MB/s in every run. QD32 likewise. The extra kernel entry did not materialize: io_uring_enter over matched 50s runs fell 5.7% (106.0M vs 112.4M), because Submit's no-op path now usually finds nothing prepared while intake-before-execute batches more SQEs behind one flush. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WorkOneRound called OnReceivedReq before Submit/PollComplete, so within a round new arrivals were enqueued on ready_tasks_ ahead of the IO completions PollComplete pushes — the precedence the fairness fix (7376b7e) removed from WorkLoop but which module mode still had. Move admission to step 4, matching WorkLoop exactly: Submit -> PollComplete -> Promote -> OnReceivedReq -> Execute -> Flush The dequeue itself stays above because is_idle_round depends on nreqs; only admission (and its req_queue_size_ accounting) moves. Over-reporting req_queue_size_ in the window between the two is safe: only Shard::IsIdle reads it, from the runtime thread via HasTask, where over-reporting is the conservative direction. Compile-verified with -DELOQ_MODULE_ENABLED=ON. WorkOneRound sits inside that ifdef, so the default build (and the 327-test suite, which has no module coverage) never compiles this path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 participants
Summary
Add per-shard page-I/O admission budgets at the io_uring boundary so foreground reads and background compaction/GC reads no longer share an unbounded read queue. This branch is rebased onto current
main(8963b7c).The implementation is functionally complete, but the local NVMe performance acceptance target is not met. The lower write cap remains opt-in rather than becoming the default.
Implementation
IouringMgr.bg_read_ratioof the read budget for background page reads while demand exists, including the wake-to-admission gap.IoQosStatssnapshots and TXSERVICE gauges.max_write_batch_pagesdrain-to-zero throttle; keep it as a deprecated, ignored compatibility option.interference_bench, GET2 benchmarking support, and an NVMe calibration helper.Final defaults:
max_inflight_read = 64bg_read_ratio = 25max_inflight_write = 32768(effectively unbounded; calibrated lower values such as 512 are explicit opt-ins)The write default was restored from 512 because the measured read-tail benefit did not justify throttling every write-heavy workload.
Review and CI fixes
db_stressCLI compatibility while no longer randomizing the retired no-op option.Local NVMe validation
This was tested in local-store mode, not Cloud Store, on
/mnt/local_nvme1(/dev/nvme1n1, ext4, schedulernone) on an AzureStandard_L8s_v4host (8 vCPU, 62.8 GiB RAM). Every run used a fresh store.Three balanced-order repetitions per condition used 4 store threads, 8 partitions, 20,000 keys/partition, 3,000-byte values, QD32 reads, a 30 s read-only baseline, and a 120 s concurrent write/compaction/GC storm.
The 512-page cap reached HWM 512 and recorded write blocks, but reduced sustained logical write throughput by only 0.70% and device writes by only 0.51%. It was transiently binding, not sustained-throughput-discriminating. Full QoS improved p99.9 by 7.95% versus 32768, but 15.751 ms is still 57.5% above the required 10 ms.
Performance acceptance status: FAIL / not release-ready on the basis of these results. The pure-read ±3% guard remains unmeasured. Pure-write throughput is explicitly not a release guard: writes intentionally remain within the background rate share even when foreground is idle.
Detailed machine, disk, fio, workload, per-run, iostat, validity, limitation, and evidence-provenance report: #461 (comment)
Verification
Current rebased tree:
cmake --build build --parallel 8: passed../build/tests/io_qos: 18 cases / 130 assertions passed../build/tests/eloq_store_test '[eloq_store]': 8 cases / 58 assertions passed.db_stress --help: deprecated option is reported as ignored.clang-format --dry-run --Werroron final modified C/C++ files: passed.git diff --check,bash -n scripts/io_calibration_sweep.sh, andpython3 -m py_compile db_stress/crash_test.py: passed.CreateBucket failed for eloqstore: http status -1errors because this host has no local MinIO/S3 service; all non-cloud tests passed.GitHub CI passed on both AMD64 and ARM64 for C++, Python, and Rust. The previous ARM64 failure was the architecture-sensitive wake-gap test oracle fixed in this revision. The external
license/clastatus remains pending and requires the repository's CLA workflow for commits authored by the local automation account.Compatibility, risk, and rollback
max_inflight_writeconfigurations now mean a per-shard page-unit admission cap, not only request-pool sizing.max_write_batch_pagesremains accepted but is deprecated and ignored.max_inflight_read=0disables read budgeting; leavingmax_inflight_write=32768preserves effectively-unbounded write admission.Reviewer focus
src/async_io_manager.cpp: admission, wake, CQE release, and error symmetry.src/storage/shard.cpp/src/eloq_store.cpp: retry integration and metric collection.benchmark/eloq_store_bm.cc/interference_bench.cpp: routing and evidence validity.tests/io_qos.cpp: wake-gap, negative-CQE, shutdown, and accounting regressions.Checklist