[Luisuantech]

Redesign of NVMe‑oF High‑Speed Read‑Write Layer

Description

Redesign the Mooncake Store NVMe-oF I/O subsystem from single-qpair to multi-qpair pipeline architecture. Achieves 5.4× throughput improvement (616 MiB/s → 3.26 GiB/s) on FORINN targets.

Module: Store — mooncake-store/src/spdk/
Type: New feature + bug fixes (existing SpdkWrapper API preserved, new methods added)

---

Problem Statement

Issue	Impact
Single I/O qpair per namespace	Write perf capped at ~1,820 MB/s (60% of target max)
No pipeline — sub-I/Os serialized on one qpair	Cannot exploit multi-qpair parallelism
keep_alive_timeout_ms=0 (legacy ApplyCtrlrOptsFromEnv)	Stale connections permanently consume target queue slots after crash

Architecture

NofConfig (MC_NVME_* / MC_NOF_* env var configuration)
    │
NofConnection (multi-qpair connection factory, keep_alive > 0)
    │  └─ NofQpairPool (N qpairs, round-robin dispatch + inflight tracking)
    │
NofSegment (contiguous LBA range on one connection)
    ├─ SubmitRead/Write   (async single-request, backwards-compatible)
    ├─ PipelineRead/Write  (blocking pipeline, high-throughput — not yet wired into hot path)
    └─ PollCompletion      (poll all qpairs)
    │
SpdkWrapper (compatibility layer — existing API unchanged)
    ├─ QidPressureGauge   (sliding-window QID pressure sensing, mutex-protected)
    ├─ OpenNofSegment     (connect with backoff retry + adaptive degradation + TryGrow recovery)
    ├─ CloseNofSegment    (no-op on stale handle — prevents double-free during static destruction)
    └─ ProbeNofSegment    (heartbeat probe, independent temp connection, connect_mutex_-serialized)

Files Changed

New (5)

File	Purpose
include/spdk/nof_config.h	NofConfig struct + env var parsing (FromEnv, ForRead, ForWrite, ForProbe)
include/spdk/nof_connection.h	NofQpairPool + NofConnection class declarations
src/spdk/nof_connection.cpp	Connect() factory, qpair allocation, TryGrow recovery
include/spdk/nof_segment.h	NofSegment class + PipelineCtx
src/spdk/nof_segment.cpp	PipelineIO core loop, SubmitRead/Write, PollCompletion

Modified (7)

File	Changes
include/spdk/spdk_wrapper.h	QidPressureGauge, new segment APIs, internal state private
include/transfer_task.h	SpdkNofTask/SubTask/Qos, SpdkNofWorkerPool, nof_cache_mutex_
src/spdk/spdk_wrapper.cpp	Adaptive connect + backoff retry, heartbeat probe serialized, dead code removed
src/transfer_task.cpp	Worker thread with flow-control backpressure, periodic TryGrow rebalance, OOM-safe
src/client_buffer.cpp	NoF DMA buffers via unified free_memory()
src/spdk/CMakeLists.txt	Add new source files
src/spdk/README.md	Architecture, component reference, benchmark data, FAQ (English)

Core Implementation

PipelineIO — high-throughput path (nof_segment.cpp)

1. max_inflight = pool.Size() × max_inflight_per_qpair
2. chunk_blocks = config.chunk_blocks (default 512 = 2 MiB @ 4 KiB)
3. Guard: reject lba + total_blocks > num_blocks_
4. Guard: reject chunk_blocks == 0 || max_inflight == 0
5. while (next_block < total || inflight > 0):
   a. Submit: while (inflight < max_inflight && next_block < total):
      - chunk = min(remaining, chunk_blocks)
      - qp = pool.GetNextQpair()    // round-robin
      - spdk_nvme_ns_cmd_read/write(ns, qp, ptr, abs_lba + next_block, chunk, cb)
      - next_block += chunk, inflight++
   b. poll_rc = pool.PollAll(0)     // harvest all qpairs
   c. if (poll_rc < 0) return -1    // qpair fault → fail fast
6. return total_blocks × block_size

NofConnection::Connect() — multi-qpair factory (nof_connection.cpp)

probe_cb:
    opts->num_io_queues        = config.num_io_queues;
    opts->keep_alive_timeout_ms = config.keep_alive_timeout_ms;
    // Unique hostnqn per connection (nqn.2024-08.mooncake:c<N>)

attach_cb: ctx->ctrlr = ctrlr;

// Sequential allocation — take as many as available, fail if < min_io_queues
for (i = 0; i < requested; i++) {
    qp = spdk_nvme_ctrlr_alloc_io_qpair(ctrlr, nullptr, 0);
    if (!qp) break;
    qpairs.push_back(qp);
}

Key Design Decisions

Category	Decision	Rationale
Correctness	LBA bounds check + start_lba_ translation	Prevents silent cross-segment data corruption
Correctness	CloseNofSegment no-op on stale handle	Prevents double-free during static destruction
Correctness	OOM-safe: set_completed(FAIL) before discard	Prevents submitter hang on allocation failure
Thread safety	Double-checked locking for nof_handle_cache_	Cache hits brief; blocking OpenNofSegment outside lock
Thread safety	connect_mutex_ in ProbeNofSegment too	spdk_nvme_probe() not thread-safe; heartbeat + I/O must serialize
Thread safety	Worker pool reset before handle close in ~TransferSubmitter	Workers joined → no inflight tasks → handles safe to free
Performance	Round-robin qpair dispatch (fetch_add)	Uniform load distribution, lock-free
Performance	Adaptive inflight = nq × inflight_per_qpair × blocks_per_chunk	I/O count → block count conversion; shrinks on degradation
Robustness	Exponential backoff capped at 30 s	Prevents multi-hour hangs with extreme config
Robustness	errno reset before strtoull, #include <cerrno>	Self-contained parser, no stale-errno false failures
Transport	Trtype passed through, not hard-coded RDMA	MC_NOF_TRTYPE=TCP works end-to-end
API	Pipeline API implemented, not yet wired	PipelineRead/Write available for future hot-path integration

Benchmark Results

Test: FORINN HWE62P447T6L00LN target, single SSD, 8 MiB random read, MC_NVME_MAX_INFLIGHT_PER_QPAIR=4.

for qp in 1 2 4 8 16 32 64; do
    MC_NVME_NUM_IO_QUEUES=$qp MC_NVME_MAX_INFLIGHT_PER_QPAIR=4 \
    ./nof_worker_pool_bench \
        --endpoints "traddr:10.10.10.100 trsvcid:4420 subnqn:... trtype:RDMA adrfam:IPv4 ns:1" \
        --rw randread --io_size 8388608 --iodepth 64 --duration_sec 20 --warmup_sec 5
    sleep 5
done

Req qp	Actual qp	Per-qp inflight	Total inflight	BW	vs qp=1	Efficiency
1	1	4	4	616 MiB/s	1.00×	—
2	2	4	8	1.15 GiB/s	1.91×	96%
4	4	4	16	2.11 GiB/s	3.50×	88%
8	8	4	32	3.25 GiB/s	5.40×	68%
16	8¹	4	32¹	3.28 GiB/s	5.45×	—
32	8¹	4	32¹	3.26 GiB/s	5.43×	—
64	8¹	4	32¹	3.26 GiB/s	5.43×	—

¹ Target per-controller I/O Queue cap (nn) limits allocation to ~8 qpairs. Throughput flatlines from qp=8 through qp=64 — the target, not client code, is the bottleneck.

• Multi-qpair scaling is near-linear up to the target's nn cap
• Low per-qpair inflight (=4) avoids target-side completion congestion
• Recommended config: MC_NVME_NUM_IO_QUEUES=8, MC_NVME_MAX_INFLIGHT_PER_QPAIR=4

[zxpdemonio]

I left a few inline comments focused on correctness and lifecycle issues in the NVMe-oF hot path. The multi-qpair/pipeline direction looks useful, but these should be fixed before this can be considered ready.

[zxpdemonio]

This pipeline API is implemented in the wrapper/segment layer, but I do not see callers from the real Mooncake NoF transfer path (TransferSubmitter still submits individual SpdkNofTasks through SubmitRequest/poll). If the PR is claiming pipeline optimization for Mooncake, please wire this into the hot path or narrow the PR description/docs to say the pipeline API is currently unused.

[zxpdemonio]

One more community-facing cleanup request: please keep code, docs, and comments in English. This PR currently contains Chinese text in source comments / docs, which makes the upstream code harder for the broader community to review and maintain.

[zxpdemonio]

I did another pass over the lifecycle/error paths. In addition to the inline notes below: please also check the submit-failure path in SpdkNofWorkerPool::workerThread(): after a SpdkNofSubTask is popped from sub_task_pool, a SubmitRequest() failure does not return it to the pool, so repeated submit failures can drain the pool because no callback will recycle that subtask.

[zxpdemonio]

nof_handle_cache_ is accessed without synchronization. If the same TransferSubmitter receives concurrent NoF submissions, the first-use path can race on this std::map, open duplicate connections for the same endpoint, and violate the QID budget this cache is meant to protect. Please guard the cache or make the single-threaded submitter contract explicit and enforced.

[zxpdemonio]

Adding a few more comments for points that were not covered in the earlier pass.

[zxpdemonio]

One higher-level suggestion: this PR mixes several separable concerns: multi-qpair connection management, worker-pool lifecycle/backpressure, pipeline API/docs, QID degradation/rebalance, client-buffer allocator cleanup, and documentation. It would be much easier to review and validate if this were split into smaller PRs, starting with the minimal hot-path change that wires multi-qpair NoF into the existing worker path with tests. Also, spdk_wrapper.cpp still has an ApplyCtrlrOptsFromEnv helper that forces keep_alive_timeout_ms to 0; if it is dead code now, please remove it, and if it is still used, it conflicts with the positive keep-alive behavior this PR wants.

[ykwd]

Thanks for the contribution!

One small request regarding the copyright notices: would it be possible to avoid adding or changing copyright statements? Since the changes in this PR are not specific to any single vendor or company, we’d prefer to keep the existing copyright headers unchanged and not introduce additional company-specific copyright notices.

Also, could you please use English for comments in the code, so they remain accessible to the broader community?

Thanks again for your contribution and understanding!

[zxpdemonio]

This PR adds Copyright (c) 2026 绿算技术 headers across multiple files. I do not think we should merge this as-is.

For an upstream Mooncake contribution, source headers should follow the repository existing copyright/license convention. Adding a company-specific and non-English copyright line to community code creates unnecessary ownership/licensing ambiguity, and it is also inconsistent with the rest of the project.

Please remove these custom copyright headers, or replace them with the standard Mooncake/KVCache.AI Apache-2.0 header only where the project normally requires a file header. For existing files, please avoid unrelated copyright/header churn.

[zxpdemonio]

Thanks for the update. I reviewed the latest head (f0cc5c3) again. A few previously raised issues are fixed now, but I still see several required items before this should be considered ready.

Required stability issues:

1. mooncake-store/src/spdk/spdk_wrapper.cpp:453-483

   ProbeNofSegment() still has a timeout-path lifetime bug. If the probe read does not complete before the deadline, probe_ctx is recycled and the temporary NofConnection/qpair is destroyed while the request may still be in flight. A later CQE can write into a recycled context, or SPDK teardown can race outstanding I/O. Please only recycle the context and destroy the connection after the submitted request has reached a terminal state, e.g. by draining/cancelling/proving completion on timeout.

2. mooncake-store/src/spdk/nof_segment.cpp:142-145

   PipelineIO() returns immediately on PollAll() error while previously submitted I/O may still hold &ctx as callback state. Since ctx is stack allocated, this can become a stack-use-after-return. Please stop new submissions and drain/cancel outstanding requests before returning from this path.

3. mooncake-store/src/transfer_task.cpp:637-644

   A worker PollAll() error is only logged, but outstanding subtasks/counters are not unwound. This can leave TransferFuture waiters hanging forever and can also make TransferSubmitter teardown block while joining the worker pool. Please treat poll failure as terminal for affected in-flight tasks: complete their states with failure and release outstanding_sub_io, total_outstanding_io, and inflight_blocks.

4. mooncake-store/src/spdk/nof_segment.cpp:42,56,86

   The range checks still use lba + num_blocks, which can overflow and pass validation for very large lba. Please use an overflow-safe form such as lba > num_blocks_ || num_blocks > num_blocks_ - lba, and also guard the start_lba_ + lba translation.

5. mooncake-store/src/transfer_task.cpp:1538-1547

   The nof_handle_cache_ map data race is fixed, but first-open is still not single-flight. Multiple callers for the same endpoint can concurrently open real SPDK connections; if one loses QIDs and fails while another succeeds and populates the cache, this path can still return failure unnecessarily. Please make the first-open path single-flight per endpoint, or re-check/wait on the cache before reporting an open failure.

Correctness / configuration:

6. mooncake-store/src/ssd_register_client.cpp:37-45

   Please normalize MC_NOF_TRTYPE before validation. With the current order, tcp is rejected before toupper() runs and silently falls back to RDMA. This file also still has a Chinese source comment; code comments in this PR should be English and match the rest of Mooncake.

Performance-path / documentation consistency:

7. mooncake-store/src/spdk/spdk_wrapper.cpp:488-492, mooncake-store/src/transfer_task.cpp:1584-1587

   PipelineRead/Write are implemented, but they are not wired into the Mooncake NoF transfer hot path. The hot path still goes through SpdkNofTask -> submitTask() -> SubmitRequest(). Please either wire the pipeline path into the actual transfer path, or narrow the PR/README claims so pipeline is described as a lower-level API that is not currently used by Mooncake transfers.

8. mooncake-store/src/transfer_task.cpp:397-411, mooncake-store/src/transfer_task.cpp:503-566

   The current backpressure only bounds the entrance task_queue_. Once a worker wakes up, it drains tasks into the per-segment seg_to_qos linked lists, which are not bounded. The comment/README should not claim this prevents unbounded accumulation across task_queue_ + seg_to_qos unless the internal queues are also bounded.

9. mooncake-store/include/transfer_task.h:420-442

   The comment says this should be min(absolute limit, qpair pool I/O capacity), but the implementation uses max(blocks_per_chunk, pool_limit) and does not cap against the original absolute limit. Under partial degradation this can increase the limit instead of shrinking it. Please make the formula match the documented intent.

Style / community-readiness:

10. Hardware-specific benchmark numbers are useful, but they should not be tied to public header comments. For example, mooncake-store/include/spdk/nof_config.h:14-16,38-40 references a specific device/model and benchmark result. Please keep headers focused on behavior and move hardware-specific benchmark data into the README or a benchmark report section with the test environment clearly labeled.

11. Date-based changelog comments such as Added 2026-..., Migrated, Removed, and this PR are process history rather than durable code documentation. Please move that history to the PR description/commit log and keep source comments focused on invariants and behavior. This pattern appears in several places, for example:

• mooncake-store/include/spdk/nof_config.h:43-44,102,161,212
• mooncake-store/include/spdk/spdk_wrapper.h:27-28,152,219-231,239,255
• mooncake-store/src/spdk/spdk_wrapper.cpp:30-32,198-220,385-391,486-492
• mooncake-store/include/transfer_task.h:429-430,497-498,513-514,585-586,654

12. Please remove company-specific copyright headers / All rights reserved blocks and Chinese history comments, e.g.:

• mooncake-store/include/spdk/nof_connection.h:1-12
• mooncake-store/include/spdk/nof_segment.h:1-7
• mooncake-store/src/ssd_register_client.cpp:1-9
• mooncake-store/src/CMakeLists.txt:1-7

Positive confirmations:

• start_lba_ translation was added to the main segment read/write path.
• TCP/RDMA is no longer hard-coded in NofConnection::Connect(), although the register-side env parsing bug above remains.
• QidPressureGauge now protects sample reads/writes with a mutex.
• TransferSubmitter now resets the NoF worker pool before closing cached handles.
• queue_not_full_cv_ is notified during shutdown.
• nof_handle_cache_ map access is now protected by a mutex.

CI note: the latest checks still show Check code format failing, so the build/test jobs are skipped. Please run the repository format script before the next update.

[zxpdemonio]

Blocking stability finding: the poll-error path still has a task/callback lifetime bug, and I can reproduce a process crash on the current head ef41b601222d51154840727a08e41233acac3e1e.

The problematic sequence is:

1. PollAll() returns as soon as one qpair reports a transport error.
2. SpdkNofWorkerPool::workerThread() calls FailAllInflightTasks() and completes/deletes every task tracked by that segment.
3. Other qpairs in the same pool may still have pending CQEs. A late CQE can therefore call nvmf_io_complete() with a freed SpdkNofSubTask/SpdkNofTask and decrement already-unwound counters. The qpair destructor also logs that it frees qpairs after a timed-out quiesce, while acknowledging that late callbacks may access freed memory.

Reproduction on 60.205.250.70:

• SPDK v26.05 TCP target, one 128 MiB Malloc0 bdev, 4 KiB blocks, listener 192.168.22.70:4420.
• Client and target were run with the same setup and the same benchmark binary/configuration; only the PR/base binary was changed.
• PR command:

env LD_LIBRARY_PATH=/opt/spdk-26.05/build/lib:/usr/local/lib \\
  MC_NVME_NUM_IO_QUEUES=1 MC_NVME_MAX_INFLIGHT_PER_QPAIR=4 \\
  MC_NOF_WORKERS=1 MC_NVME_KEEP_ALIVE_TIMEOUT_MS=60000 \\
  /mnt/vdb/build-pr3292/mooncake-store/benchmarks/nof_worker_pool_bench \\
  --endpoints 'traddr:192.168.22.70 trsvcid:4420 subnqn:nqn.2026-08.io.spdk:cnode1 trtype:TCP adrfam:IPv4 ns:1' \\
  --rw randwrite --io_size 131072 --iodepth 64 \\
  --range_bytes 67108864 --duration_sec 5 --warmup_sec 1 \\
  --report_interval_ms 1000 --nof_workers 1

Observed on two runs: rc=139 (SIGSEGV). The log contains repeated poll completion error, submit io fail, and:

total outstanding io < 0
task outstanding io < 0
task inflight_block_count < 0
nvme_tcp.c: Failed to flush ... (14): Bad address

The community/base binary completed the same command successfully (for example, 1.89 GiB/s, failed_ops=0). Please fix the transport-error lifecycle so all qpairs are stopped/drained (or explicitly cancelled/fenced) before task/context memory and counters are released, and add a regression test covering one failed qpair with pending CQEs on another qpair. Until then this should block merge.

[zxpdemonio]

Performance result from a like-for-like run on 60.205.250.70 (current PR head ef41b601222d51154840727a08e41233acac3e1e versus the community/base build):

• SPDK v26.05 TCP target, one 128 MiB Malloc0 software bdev, 4 KiB blocks.
• One endpoint, one worker, same target restarted for each run.
• Random read, 128 KiB I/O, iodepth 64, 64 MiB range, 1 s warm-up + 5 s measurement, MC_NVME_MAX_INFLIGHT_PER_QPAIR=4, no failed operations.
• Bandwidth uses GiB/s (1 GiB = 2^30 bytes); latency is submit-to-future completion latency.

I/O qpairs	PR	Base	PR delta
1	2.77 GiB/s, 2.82 ms mean	2.76 GiB/s, 2.83 ms mean	+0.4%
2	2.29 GiB/s, 3.41 ms mean	2.80 GiB/s, 2.79 ms mean	-18.2%
4	2.16 GiB/s, 3.61 ms mean	2.80 GiB/s, 2.79 ms mean	-22.9%
8	2.18 GiB/s, 3.58 ms mean	2.82 GiB/s, 2.77 ms mean	-22.7%

The PR's PipelineRead/Write API is not used in this benchmark or in the Mooncake NoF transfer hot path; the path remains SpdkNofTask -> SubmitRequest -> SubmitRead/SubmitWrite. Therefore these numbers measure the current worker-pool path, not the standalone Pipeline API.

Could you please provide the exact methodology behind the reported 616 MiB/s -> 3.26 GiB/s (5.4x) result, including:

• target/device model and whether the target is real NVMe-oF hardware or a software/malloc backend;
• transport (TCP/RDMA), SPDK version, CPU/NUMA pinning, and target/client host topology;
• number of connections, qpairs per connection, worker/submit threads, and total/per-qpair queue depth;
• read/write mix, I/O size, alignment, address range, random/sequential pattern, and data temperature (cold-only or cache-warm);
• warm-up, measurement duration, repetitions, aggregation method, and whether bandwidth is decimal MB/s or binary MiB/GiB/s;
• the precise latency/throughput start and end points.

With the same conditions, please also report the write result. On this setup the PR write run currently crashes before producing a valid throughput number.

[zxpdemonio]

Blocking stability re-review on the latest head 9b2a91adba0dbbc27ddf8d7b163292772ef0fe9b:

I rebuilt this head in a clean directory on 60.205.250.70 and reran the same reproduction as the previous review: SPDK v26.05 TCP target, one 128 MiB Malloc0 bdev with 4 KiB blocks, randwrite, 128 KiB I/O, iodepth 64, one NoF worker, one I/O qpair, and MC_NVME_MAX_INFLIGHT_PER_QPAIR=4.

The original SIGSEGV no longer occurred, but the failure mode has changed into a permanent recovery hang:

• the first CQ transport error immediately moves the pool to DRAINING;
• throughput remains 0 B/s and completed=0;
• after the 5-second measurement, the log reports failed=84252;
• the process does not exit after the benchmark duration and had to be terminated by a 20-second guard (rc=124);
• while stuck, the worker continuously logs submit io fail.

The accounting path explains the hang:

1. nvmf_io_complete()'s DRAINING short-circuit only decrements the pool-level inflight counter. It skips task->io_count, outstanding_sub_io, inflight_block_count, and the per-QoS inflight_blocks counter.
2. SpdkNofQos::FinalizeAfterDrain() marks tasks failed and clears outstanding_sub_io, but deliberately does not release task->io_count or the inflight-block counters.
3. SpdkNofWorkerPool::workerThread() continues polling and cannot satisfy its shutdown condition while total_outstanding_io remains non-zero.
4. WaitForInflightCompletion() has no timeout or terminal recovery operation. Once a dead qpair stops producing CQEs, worker, probe, or connection teardown can wait forever.

The new regression coverage also does not currently validate this fix:

• nof_qpair_drain_protocol_test --gtest_filter=NofQpairDrainSibling.* exits with 255 at the first sibling-qpair case. The test constructs null qpair slots, skips the injected failed qpair, and then still calls SPDK completion processing for a null sibling qpair.
• The real-target nof_qpair_sibling_failure_test passes even when the injected failure is never observed. In my run there was no transition to DRAINING; the test explicitly accepts that no-op outcome. It also exercises concurrent PipelineRead/Write, not the production SpdkNofWorkerPool path that reproduced the failure, and the Pipeline path does not contribute to the pool inflight counter asserted by the test.

Please keep this PR blocked until the transport-error state machine has a terminal outcome. The fix needs to stop new submissions, drain/cancel/reset or retire the affected connection, retain callback state and buffers until a real terminal fence, and release every logical counter exactly once. The regression test should exercise the production worker path, require the fault to trigger, assert all task/QoS/worker counters return to zero, and prove both futures and teardown complete within a bounded time.