Skip to content

Handle transient SPDK qpair descriptor exhaustion with deferred submissions - #9

Merged
thweetkomputer merged 5 commits into
mainfrom
fix/spdk-submit-backpressure-20261009
Oct 9, 2026
Merged

thweetkomputer merged 5 commits into
mainfrom
fix/spdk-submit-backpressure-20261009

Conversation

@thweetkomputer

@thweetkomputer thweetkomputer commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Problem and behavior

When an SPDK qpair exhausts its request descriptors, an otherwise valid asynchronous read/write/flush returns -ENOMEM. Bycorf currently turns this transient pressure into an application I/O failure. This existed with the old Lavik read-buffer default; larger individual read slots happened to limit how many ordinary reads could reach SPDK.

Retain these rejected operations in a worker-local queue for their qpair and retry after processing completions. Keep their DMA buffers and completion tags alive, and count deferred operations as outstanding for close, shutdown and worker-idle handling. Other errors, including -ENOMEM without any in-flight operation that could release resources, still fail.

Implementation and bounds

  • Reuse the existing bounded 4,096-entry request pool; no hot-path allocation, cross-worker lock, or enlarged SPDK queues.
  • Store the operation and submission arguments in the request and maintain a FIFO of deferred requests per controller channel.
  • Bound each retry pass using the polling completion budget. Never poll inline from submission: the caller has not installed its coroutine continuation yet.
  • Document ownership, lifetime and failure behavior near the code and in docs/architecture/io.md.

Reproduction and validation

bycorf_spdk_backpressure_stub_check is registered with CTest in BYCORF_KERNEL_BYPASS=ON builds, so all four bypass CI jobs run it without NVMe hardware or hugepages. Link-time stubs replace EAL initialization and NVMe device calls while exercising the production Worker, request pool, submission, polling and completion paths. It covers independent qpairs, shared-controller namespaces, FIFO read/write/flush retries, persistent and nonrecoverable ENOMEM, other submission/completion errors, exactly-once callbacks, two full 4,096-request pool cycles, and shutdown with deferred I/O. Production request code gains no testing hooks.

Local Clang 18 Debug and GCC 13 Release CTest runs pass. Temporarily disabling ENOMEM deferral makes the test fail on the first queued write; restoring it passes again. Hosted CI results are tracked by this PR's checks.

A read-only hardware check submits 1,024 4 KiB reads to each of two qpairs without polling between submissions, for four rounds:

BYCORF_DPDK_MEMORY_MB=8192 \
BYCORF_EAL_ARGS='-a fa20:00:00.0 -a cc2f:00:00.0' \
./build/bycorf/bycorf_spdk_backpressure_check \
  spdk://fa20:00:00.0/1 spdk://cc2f:00:00.0/1

On the explicitly provisioned test devices, the old backend rejects 1,024 of 2,048 operations each round (exit 1); the fixed backend completes all 2,048 (exit 0), with matching read bytes and exactly one callback per operation. The check covers rejecting close while I/O is outstanding and recycling request objects. It is intentionally not an automatic CTest because it requires explicitly supplied SPDK hardware.

Lavik integration: native SPDK, six NVMe namespaces, 16 workers, 640 memtier connections, pipeline 1, defrag enabled. All 12 primary and six supplemental benchmark points completed without client errors; GETs had no misses. Large-string/collection boundary tests passed before and after restart, including native SPDK.

Diagnostic tracing at the same connection count found worker preemption explaining 3–12 ms gaps between SPDK polls (95.7–99.4% of the examined long gaps was off-CPU time). Other workers continue returning requests while one owner is delayed, concentrating the closed-loop workload at that owner. With 32 KiB read slots, the traced run encountered 138 initial transient submission failures, all recovered, and finished with 66,031,238 accepted/released completions. The same fixed binary with 1 MiB slots peaked at 218 outstanding requests and had no descriptor exhaustion. No foreign block-owner routing or completion-accounting leak appeared in these traces. Diagnostic timings are not performance benchmarks.

The combined Lavik slot-size/runtime change measured GET +0.11% and SET −2.04% in three alternating runs. That comparison changes both variables and does not establish this runtime fix as the SET cause. The slot-size proposal remains a separate draft; this PR fixes correctness independently.

Compact request metadata

AsyncRequest is 48 bytes on the tested x86-64 build, compared with the initial 72-byte retry descriptor. A fixed-file index replaces the cached namespace/qpair pointers, with fields grouped by alignment. Initial submission passes the namespace/qpair already resolved for validation, without an extra lookup; only deferred retries resolve the namespace from the file table. Outstanding-I/O accounting protects that entry from close/reuse.

Completion callbacks obtain the sole storage backend from the existing worker TLS. Requests carry no backend pointer, and polling neither installs a second TLS context nor saves/restores one. Initialization enforces that the backend belongs to the supplied worker; completion polling and shutdown drains require that worker's context to remain bound. The hardware check now drives the actual Worker submission/polling path and also verifies an accepted read completes during shutdown.

The Release/SPDK build and four-round, two-qpair hardware check pass (2,048 accepted/completed each round, zero rejected). The throughput and diagnostic results above describe the initial layout; there is no completed throughput comparison for these metadata-only changes.

Risk and rollback

The existing request-pool bound still applies. Deferring temporarily resource-constrained operations changes error timing and must preserve caller buffer/tag lifetimes. Revert this commit to restore immediate error behavior. No disk-format change.

@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 5d5fa949-bd5e-4615-ac8f-964e1ab5a81a

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thweetkomputer
thweetkomputer merged commit 85d8c9c into main Oct 9, 2026
10 checks passed
@thweetkomputer
thweetkomputer deleted the fix/spdk-submit-backpressure-20261009 branch October 9, 2026 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant