[LujhCoconut]

Description

Related RFC #3397

Decouple Mooncake Store's NoF (NVMe-oF) data path from SPDK behind two neutral interfaces, and fix #3131 along the way.

Before: all NoF traffic funneled into the SpdkWrapper Meyers singleton (env init + qpair cache + I/O + DMA alloc + probe in one class). SPDK types leaked through its header, and 38 SPDK/DPDK static libs propagated as PUBLIC link requirements to client/master/bench.

After:

• Two role interfaces under include/nof/: NVMeoFInitiator (byte-oriented I/O behind an opaque NofSegmentHandle) and DmaBufferAllocator.
• SPDK implementations live in the new mooncake_nof_spdk static lib (pimpl — SPDK types confined to one .cpp, SPDK/DPDK libs linked PRIVATE).
• Instances come from CreateNofRuntime() — the only #ifdef USE_NOF site naming SPDK types — and are injected via shared_ptr. The SPDK env initializes lazily on first use and is owned by a refcounted guard.
• register_buffer now also registers user buffers with the SPDK RDMA translation table (process-global registry, per-2MB-page refcount, TE rollback on failure) — fixes [Bug]: NoF 写入失败：spdk_rdma_get_translation: No translation for ptr 是否是我配置的不对导致的这个问题？ #3131. Requires SPDK ≥ 22.05; a CMake probe warns on older versions.

No wire-protocol / RPC / Python ABI changes. Non-NoF builds keep the exact original behavior (allocator injected as nullptr → original protocol → VRAM → aligned_alloc order).

flowchart TB
  subgraph B["Before — everything funnels into one singleton"]
    TS1["TransferSubmitter / worker pool"] -->|GetInstance ×6| SW["SpdkWrapper (Meyers singleton):<br/>env init · qpair cache · IO · DMA alloc · probe"]
    RC1["RealClient::setup"] -->|eager InitializeEnv| SW
    CB1["ClientBufferAllocator / utils"] -->|use_spdk_dma flag| SW
    MS1["Master heartbeat probe"] --> SW
    SW -.->|SPDK types in header,<br/>38 SPDK/DPDK libs PUBLIC| L1["leaks into client / master / bench"]
  end
  subgraph A["After — interface split + dependency injection"]
    RC2["RealClient::setup"] -->|CreateNofRuntime:<br/>only USE_NOF ifdef| F["NofRuntime factory"]
    F --> NI["NVMeoFInitiator «interface»"]
    F --> DA["DmaBufferAllocator «interface»"]
    NI -.pimpl.- SI["SpdkInitiator::Impl — SPDK confined<br/>to one .cpp, libs PRIVATE"]
    DA -.-> SA["SpdkDmaAllocator /<br/>SystemDmaAllocator"]
    TS2["TransferSubmitter / NofWorkerPool"] -->|injected shared_ptr| NI
    CB2["ClientBufferAllocator"] -->|injected; nullptr when NoF absent| DA
    MS2["Master heartbeat probe"] --> NI
  end

Loading

Lifetime and registration fixes riding along:

flowchart LR
  subgraph LIB["mooncake_nof_spdk — the only SPDK-aware code"]
    G["SpdkEnvGuard:<br/>lazy, refcounted,<br/>init/fini serialized"]
    R["NofPageRegistry: process-global,<br/>per-2MB-page refcount,<br/>-EBUSY → external page"]
    Q["ctrlr/ns/qpair cache:<br/>same endpoint → same qpair,<br/>connect outside mutex"]
    G --- R
    G --- Q
  end
  RB["RealClient::register_buffer"] -->|1. TE register OK| R
  R -->|2. RegisterMemory fail → roll back TE| RB
  W["NofWorkerPool"] -->|SubmitIO bytes /<br/>PollCompletion| Q

Loading

Also fixed in passing: io_count was a raw pointer to a worker-thread stack variable (now shared_ptr<atomic<int64_t>>); a failed SubmitIO leaked its sub-task pool slot (now returned); the probe context recycle race; OpenSegment blocked a global mutex during NVMe-oF connect.

Upgrade notes for existing NoF users

No breaking configuration or API changes: build flags (-DUSE_NOF=ON), existing env vars (MC_NOF_*, MC_NVME_*, MC_STORE_NUMA_SOCKET_ID), transport strings, and the Python ABI are all unchanged. One optional env var is added. Behavioral differences to be aware of:

• New optional MC_NOF_BACKEND (spdk by default, case-insensitive): selects the NoF backend pair. Set it to none to disable NoF at runtime in a USE_NOF build — one binary can be deployed on nodes without a working SPDK/hugepage environment. Unrecognized values fall back to spdk with a warning. Note the master process honors it too (with none, heartbeat probes fail and NoF segments are unmounted after the threshold).
• SPDK env init is now lazy. RealClient::setup used to initialize the SPDK env eagerly and fail when hugepages were missing — even for clients that never touch NoF. Setup now always succeeds; a broken SPDK env surfaces as an error on the first NoF operation instead. Keep configuring hugepages before NoF traffic, as the deployment guide says.
• register_buffer registers with SPDK automatically — no extra call or parameter. The buffer must be block-size aligned (existing hard gate) and must live in hugepage-backed memory (2MB hugetlb). The pointer itself need not be 2MB-aligned — a non-aligned buffer is registered by its containing 2MB pages (logging a RegisterMemory warning); but 4KB-backed malloc memory fails inside SPDK's spdk_mem_register with paddr-misalignment -EINVAL.
• The [Bug]: NoF 写入失败：spdk_rdma_get_translation: No translation for ptr 是否是我配置的不对导致的这个问题？ #3131 fix needs SPDK ≥ 22.05 (spdk_mem_register). Older SPDK still builds (with a CMake warning) and behaves exactly as before — i.e. NoF-over-RDMA writes from unregistered user buffers keep failing with "No translation".

Use cases

1. Pure-NoF write to the SSD pool (the #3131 scenario — now works over RDMA):

from mooncake.store import MooncakeDistributedStore, ReplicateConfig

store = MooncakeDistributedStore()
store.setup(...)  # protocol="rdma", local_buffer_size > 0 — see deployment guide

# Now also registered with SPDK's RDMA translation table (automatic):
store.register_buffer(buf_ptr, buf_size)

config = ReplicateConfig()
config.replica_num = 0      # no DRAM replica
config.nof_replica_num = 1  # NoF SSD replica only
store.batch_put_from(keys, buffer_ptrs, sizes, config)

Requirements: USE_NOF=ON build against SPDK ≥ 22.05; the buffer must be block-size aligned (existing hard gate) and must be hugepage-backed (2MB hugetlb). The pointer itself need not be 2MB-aligned — register_buffer normalizes it to its containing 2MB pages, logging a RegisterMemory warning — but 4KB-backed malloc memory fails inside SPDK's spdk_mem_register with paddr-misalignment -EINVAL.

2. One binary, deployed everywhere — nodes without hugepages/SPDK no longer need a separate build:

export MC_NOF_BACKEND=none  # NoF disabled at runtime; the rest works as usual

3. Standard SSD-pool deployment (master + store service + spdk_tgt_create + mooncake_ssd_register): unchanged — see docs/source/deployment/ssd/nvmf-ssd-deployment-guide.md.

4. Custom/test initiator injection (C++): any NVMeoFInitiator implementation can be injected — directly into NofWorkerPool, or via the new tail parameter of Client::Create(...). The alignment gate, QoS, and worker loop run unchanged against it; nvmeof_initiator_test exercises the full pool this way without SPDK.

What if the backend is not SPDK later?

That swap is the point of the split. A non-SPDK backend implements NVMeoFInitiator (and optionally DmaBufferAllocator) and plugs into CreateNofRuntime() — the single place that names SPDK types. Callers (NofWorkerPool, RealClient, master probe) do not change. Backend differences are advertised through NofCapabilities: e.g. a backend without an RDMA translation table returns requires_memory_registration = false and its RegisterMemory is a no-op OK. The new test suite already runs the worker pool against a non-SPDK MockInitiator, proving the seam.

Backend selection exists from day one as one env var choosing a matched Initiator+Allocator pair: MC_NOF_BACKEND=spdk (default) or none (runtime-disable NoF in a USE_NOF build). Adding a backend is one enum value + one factory branch + one env value — deliberately no plugin machinery before a second backend exists.

Module

• Mooncake Store (mooncake-store)
• Docs

Type of Change

• Bug fix ([Bug]: NoF 写入失败：spdk_rdma_get_translation: No translation for ptr 是否是我配置的不对导致的这个问题？ #3131)
• Refactor

How Has This Been Tested?

Test commands:

cmake --build build -j            # USE_NOF=OFF: 0 errors, 0 warnings
./build/mooncake-store/tests/nvmeof_initiator_test   # new mock-initiator suite
./build/mooncake-store/tests/transfer_task_test
./build/mooncake-store/tests/client_buffer_test

Test results:

• Unit tests pass (nvmeof_initiator_test 4/4, transfer_task_test 11/11, client_buffer_test 16/16)
• Integration tests pass (if applicable)
• Manual testing done (describe below)

⚠️ USE_NOF=ON build and runtime NoF validation still pending — requires a machine with SPDK + an NVMe-oF target. Will run before marking ready (incl. non-2MB-aligned hugepage-slice registration and NoF-over-RDMA pure-NoF write from #3131's repro).

Checklist

• I have performed a self-review of my own code
• I have formatted my code using ./scripts/code_format.sh
• I have run pre-commit on the files changed in this PR and all hooks pass (local pre-commit 2.17 too old for the repo config; ran the equivalent hooks manually: clang-format via code_format.sh --staged, whitespace/EOF checks)
• I have updated the documentation (if applicable)
• I have added tests to prove my changes are effective
• For changes >500 LOC: I have filed an RFC issue (design doc exists; will file/link the RFC issue before undrafting)

[fcczzz]

I have not finished reviewing the overall design yet. For now, I only helped run a few NoF experiments in Docker. The following are the test results and the issues I encountered while setting up the environment.

Test environment

• SPDK v23.01.1
• 2 MiB hugepages
• SPDK Malloc0: 64 MiB with a 4 KiB block size

1. NoF worker-pool smoke test

I ran nof_worker_pool_bench with 4 KiB writes, iodepth 8, a 32 MiB range, and a 10-second runtime.

completed_ops=635921
failed_ops=0
bw=248.41 MiB/s
iops=63592.10
fail_iops=0.00
clat_mean=124.14 us
clat_p99=135.00 us
benchmark_rc=0

The SPDK initiator successfully connected to the RDMA target, and the workload completed without I/O failures.

2. User-buffer registration test

I ran a focused test for the #3131 path using a pure NoF configuration:

replica_num=0
nof_replica_num=1

The test used two adjacent 4 KiB buffers inside the same 2 MiB hugepage. Both addresses were intentionally not 2 MiB aligned.

The test performed the following operations:

1. Register both buffers.
2. Write from the source buffer with put_from.
3. Unregister the source buffer.
4. Read into the destination buffer with get_into.
5. Compare the returned data byte-for-byte.
6. Unregister the destination buffer and close the client.

I did not observe the previous No translation for ptr failure. This experiment covered non-aligned registration ranges and the case where two registered buffers share the same 2 MiB page.

3. Store-level end-to-end test

I also ran the repository's store_kv_bench.py as a more end-to-end functional test.

The test used:

• the public Python Store API;
• metadata server and master;
• a pure NoF configuration: memory=0, nof=1;
• the zcopy put_from/get_into path;
• four concurrent lanes;
• batches of eight objects.

It wrote 1,024 4 KiB objects and then read back and verified all 1,024 objects.

summary.ok=true
write.successful_kvs=1024
read.successful_kvs=1024
overall.successful_kvs=2048
failed_kvs=0
failed_requests=0
misses=0
verify_failures=0
journal_records=0

This exercised the following path:

Python Store API
→ RealClient
→ metadata server
→ master NoF allocation
→ NoF worker pool
→ SPDK initiator
→ RDMA target
→ readback verification

Issues encountered

Missing yalantinglibs dependency

A clean USE_NOF=ON build failed with:

fatal error: ylt/struct_json/json_reader.h: No such file or directory

spdk_initiator.cpp indirectly includes types.h, which requires yalantinglibs. However, the new mooncake_nof_spdk target does not currently declare a dependency on yalantinglibs::yalantinglibs.

I temporarily injected the yalantinglibs include path globally to continue testing. It looks like the target should declare the dependency directly:

target_link_libraries(
  mooncake_nof_spdk
  PRIVATE
    ${SPDK_STATIC_LIBS}
    glog::glog
    yalantinglibs::yalantinglibs)

IOVA mode inside the container

The initiator initially failed with:

EAL: FATAL: Cannot use IOVA as 'PA' since physical addresses are not available

For these experiments, I used the following local test-only change:

opts.iova_mode = "va";

The previous spdk_wrapper.cpp implementation appears to have the same default behavior, so this looks like an existing container-compatibility limitation rather than a regression introduced by this PR.

Scope and limitations

These are experimental results only, not a complete design review or approval of the PR.

The tests used an SPDK memory bdev and a same-host RDMA connection. They did not cover:

• physical NVMe devices;
• cross-host RDMA traffic;
• multiple NoF endpoints or worker scaling;
• reconnect, target restart, or error recovery;
• long-running stability;
• all code paths and design decisions changed by this PR.

The end-to-end run was short and included cold-start costs, so its throughput and latency numbers should be treated as functional evidence rather than representative performance measurements.

[fcczzz]

The abstraction direction looks reasonable, and the new SPDK experiments provide useful evidence for the normal #3131 path. I still see lifecycle gaps around timeout handling and memory registration: a timed-out probe can recycle an in-flight callback context, and client teardown does not release its process-global SPDK registrations. The global registry also needs ownership accounting when multiple initiators register the same address. I think these should be addressed before merge.

[fcczzz]

The interface split looks reasonable, but I see three issues that should be addressed before merge:

1. Registration failure leaves the failing page untracked.

spdk_mem_register() can return an error after marking the page registered or installing translation state. NofPageRegistry::Register() rolls back only previously successful pages, leaving the failing page untouched. A later retry can receive -EBUSY and incorrectly classify that page as externally owned. Please clean up the failing page too, retaining explicit bookkeeping when cleanup cannot be confirmed.

2. Unregistration failure is reported as success.

ReleaseRangeLocked() logs backend errors but removes the page record anyway, and Unregister() subsequently erases the owner record and returns OK. This bypasses the ShmHelper/RealClient quarantine checks and allows munmap() while DMA translation state may remain. Please propagate the failure and preserve enough state for safe cleanup or quarantine.

3. Existing C++ callers silently lose NoF support.

The new Client::Create() parameter defaults to nullptr, and that value is forwarded without creating a default runtime. Existing callers still compile, but no NoF worker pool is created even with USE_NOF=ON, so NoF transfers fail. Please preserve the existing default behavior while keeping an explicit way to disable NoF or inject another initiator.

Please add regression coverage for registration failures after partial state changes, backend unregistration failures, and NoF transfers through the existing C++ creation API.

[LujhCoconut]

Thanks @fcczzz for the thorough review. The remaining findings are addressed in the latest commit:

• Batch unregister retry: ERR_ADDRESS_NOT_REGISTERED from an already-cleaned sibling is now normalized per member, so a partial-failure retry no longer keeps the engine's region records stuck and un-re-registerable. Covered by a new regression test (BatchRetryIgnoresAlreadyCleanedSibling).
• A confirmed-cleanup retry with update_metadata now republishes the local segment desc, closing the stale remote advertisement gap left by the publish-failure path.
• ShmHelper only forces hugepage allocation when a NoF initiator actually exists; MC_NOF_BACKEND=none now keeps the previous allocation behavior.
• Corrected the spdk_env_fini reachability comments (in HAVE_SPDK_MEM_REGISTER builds the leaked NofRegistryBundle pins the env for the whole process, so fini intentionally never runs) and downgraded the optional-ns parse message to WARNING.

The tent CI failure is pre-existing breakage on main: #4314 added GrantLeasePolicy to BatchProbeKey but missed the call site in the master_service DSL scenario. The one-line fix is posted separately on PR #4729.

Could you take another look when you have time?

[LujhCoconut]

This PR has gone through many rounds of iteration and self-review, and the remaining edge cases are now addressed:

• Reverted the engine-level ERR_ADDRESS_NOT_REGISTERED tolerance — it assumed "NOT_REGISTERED ⇒ no live MR", which
  only RdmaTransport guarantees; keeping it would let transports without that gate (e.g. EFA) re-register a VA with a
  live MR. Multi-transport unregister-retry is left as a documented follow-up.
• Fixed the re-init guard to key off nof_buffer_cleanup_failed_ instead of registered_buffer_sizes_.
• Re-seeded pending_dereg_ when the batch's deferred publish fails, so a retry can still finish cleanup + republish.
• Hardened a few boundaries: null/zero-size guard in NofPageRegistry::Register, restored the 256TB address-window check, and a process-wide lock serializing spdk_nvme_probe/detach.

I've re-reviewed the whole diff end-to-end and the remaining open items are pre-existing behaviors (multi-transport
unregister retry, nof_io_complete double-completion, USE_NOF=ON in CI) that I'll track as follow-ups rather than
grow this PR further.

@fcczzz Could you take another look when you have a chance?

[fcczzz]

Validation on head 991f655 confirmed that the NoF-over-RDMA runtime path works with SPDK v23.01.1 and a same-host RDMA malloc-bdev target. Pure-NoF Python API round trips, overlapping hugepage-slice registrations, client recreation, and concurrent batch workloads passed data verification. The focused tests also passed: 84/84, with no skips.

One build blocker remains: the unmodified USE_NOF=ON, BUILD_UNIT_TESTS=ON build fails on the ambiguous construction NofMetric metric({{"client", "test"}}); in nof_metrics_test.cpp. Making the map type explicit fixes the build; this was the only source modification used for validation, and production code was unchanged.

Please commit that fix and confirm the full build on the resulting SHA. With that addressed, I have no remaining blocking concerns within the reviewed and tested scope.

[LujhCoconut]

Thanks @fcczzz for your patience.

Fixed — committed as a381a943. The NofMetric construction now passes an explicit std::map<std::string, std::string>; production code is unchanged (test-only).

[fcczzz]

Build and validation passed on my own machine. No blocking findings from me; please merge the latest main and resolve the conflicts.

[LujhCoconut]

Latest main has been merged and conflicts resolved.

[ykwd]

LGTM

[fcczzz]

@alogfans Hi, could you review the changes in rdma_transport.cpp? They affect generic RDMA unregister behavior—including partial failures, retries, chunked buffers, and batch cleanup—even when NoF is disabled. The focused tests passed, but I'd appreciate TE-owner confirmation of the cleanup and concurrency semantics before merging.

[zxpdemonio]

I do not have a concrete code change to request at this point. However, I am holding approval because the changes in rdma_transport.cpp affect generic Transfer Engine unregister behavior—including partial failures, retries, chunked buffers, batch cleanup, and multi-transport interactions—even when NoF is disabled.

The focused tests and current CI are green, but I would like the Transfer Engine owner to confirm the cleanup and concurrency semantics before this PR is considered ready to merge. If that review finds no issue, no additional change is required from my side.

[alogfans]

LGTM in TE

[LujhCoconut]

Hi @ykwd. This PR seems to be good to all the reviewers involved. If you have a moment, could we get this merged? Thanks!

[ykwd]

Thanks for this great work!