[pjdurden]

NoF qpair recovery after target restart (issue #3863)

1. Root cause

SpdkWrapper::OpenNofSegment() (mooncake-store/src/spdk/spdk_wrapper.cpp)
memoizes exactly one nof_seg_handle per (controller, namespace) pair, and
each handle owns exactly one spdk_nvme_qpair allocated on first use:

auto ns_it = ns_seg.find(tr.ns);
if (ns_it != ns_seg.end()) {
    return ns_it->second.get();   // cached forever, never health-checked
}

Nothing in the tree ever inspected the qpair's health or called
spdk_nvme_ctrlr_reconnect_io_qpair() / spdk_nvme_ctrlr_reset(). When the
NoF target goes away the qpair enters a permanent failure state, so from that
moment on:

• spdk_nvme_ns_cmd_read/write() (via SpdkWrapper::SubmitRequest) returns
  -ENXIO, and SpdkNofWorkerPool::workerThread() just marks the task failed;
• spdk_nvme_qpair_process_completions() returns -ENXIO, and the worker only
  logs poll completion error;
• SpdkWrapper::ProbeNofSegment() reuses the same dead handle, so the master's
  NoF heartbeat fails forever.

Because the handle is cached, this does not clear itself when the target comes
back. The master unmounts the segment after the failure threshold, but once the
segment is registered again the heartbeat still probes the stale qpair and
unmounts it again, and client I/O submitted through
TransferSubmitter::submitSpdkNofOperation() keeps hitting the same dead
qpair. That is the "keeps reusing failed qpairs and cannot automatically
recover I/O after target recovery" behaviour reported in the issue.

2. The fix and why

Keep the cached handle (its address is used as a map key by
SpdkNofWorkerPool::seg_to_worker_ and by the per-worker seg_to_qos map, so
it must stay stable), but heal the qpair inside it instead of reusing a failed
one.

SpdkWrapper gains:

• IsNofSegmentFailed(): spdk_nvme_qpair_get_failure_reason() != SPDK_NVME_QPAIR_FAILURE_NONE. Cheap enough to call on every submit round.
• TryLockNofSegmentIo(): a try-lock on a new per-controller
  std::shared_mutex io_mutex. Every thread that submits to, polls, or probes
  a qpair of that controller holds it shared. It never waits: while a reset is
  queued or running it returns an unowned lock and the caller skips that
  segment for the round.
• TryRecoverNofSegment(): non-blocking, called by the qpair's owner while
  holding the io lock. If the controller has been reset since this qpair last
  connected, it disconnects and reconnects the qpair on the owning thread (SPDK
  requires that, and it is also where aborted requests complete). Otherwise it
  queues a controller reset and returns false.

Controller resets run on a background recovery thread inside SpdkWrapper.
It takes io_mutex exclusively, so spdk_nvme_ctrlr_reset() never runs while
another thread is using the controller, as SPDK requires. After a successful
reset it refreshes every cached ns pointer of that controller (the reset may
free namespace structs) and the cached block size, then bumps a reset
generation that tells each qpair owner to reconnect. GetBlockSize() now
returns the cached value, so callers outside the workers never dereference a
namespace pointer.

Callers:

• SpdkNofWorkerPool::workerThread() takes the io lock per segment for the
  submit round and for polling, and calls TryRecoverNofSegment() for a failed
  segment with queued work, at most once per second per segment. A worker
  never blocks on a dead target, so healthy segments sharing it keep moving.
• SpdkWrapper::ProbeNofSegment() does the same before probing and reports
  controller_resetting or recovering instead of probing a dead qpair.

If recovery has not completed, behaviour is unchanged from today: the submit
fails the task and the probe reports a heartbeat failure, and a later round
tries again. Recovery logs carry the target endpoint (traddr:trsvcid/subnqn)
and namespace ID.

3. Files changed

• mooncake-store/include/spdk/spdk_wrapper.h: declare the new API and the
  recovery thread state.
• mooncake-store/src/spdk/spdk_wrapper.cpp: implement it, give
  nof_seg_handle its owning ctrlr_info *, nsid, cached block size and
  connected generation, and recover in ProbeNofSegment().
• mooncake-store/src/transfer_task.cpp: io lock around submit and poll,
  recovery from the owning worker with per-segment back off.
• mooncake-store/tests/e2e/run_nof_heartbeat_tcp_e2e.sh: extend the existing
  NoF heartbeat e2e with a target recovery phase.

4. Risk and uncertainty

• Every recovery is a full controller reset, even when only one io connection
  dropped. Telling the two apart needs a connect attempt, which blocks while
  the target is down, and that is exactly what the workers must not do. The
  cost is that a reset also makes the other qpairs of that controller
  reconnect once.
• Recovery takes one extra round: the first failure queues the reset, the next
  round after it succeeds reconnects the qpair. For the master heartbeat that
  is one more failed probe after the target is back.
• The owner's reconnect after a successful reset is still a synchronous
  connect. The target answered the reset moments earlier, so it normally takes
  one round trip, but a target that dies again in that window would block that
  one worker for a connect timeout.
• Not verified against real SPDK: this environment has no SPDK, no NVMe-oF
  target and no hugepages, and USE_NOF is OFF by default, so the NoF code
  is not part of the normal build here.

5. How I verified it

• Compiled spdk_wrapper.cpp against the real SPDK v23.01.1 headers (the
  version pinned by dependencies.sh) with
  g++ -std=c++20 -fsyntax-only, and the same for the extracted
  MaybeRecoverSpdkNofSegment() helper plus the worker's lock/poll calls.
  Both pass. The reset and reconnect semantics above were checked against
  SPDK v23.01.1 lib/nvme/nvme_ctrlr.c. A full build was not possible here (no SPDK, and the store's other
  third-party headers are not installed).
• clang-format (the formatter driven by the repo's pre-commit
  mooncake-code-format hook) reports no changes for the three C++ files.
  pre-commit itself is not installed in this environment.
• bash -n mooncake-store/tests/e2e/run_nof_heartbeat_tcp_e2e.sh passes.
• The e2e script now covers the regression. It already killed the NVMe-oF
  target and asserted the master unmounts the segment; it now restarts the
  target, re-registers the segment, and asserts that the segment is not
  unmounted a second time. Before this change the master's heartbeat keeps
  probing the stale qpair, so the segment is unmounted again within
  HEARTBEAT_INTERVAL * HEARTBEAT_FAILURES and the assertion fails. The
  client process is deliberately kept alive across the outage so its cached
  qpair is exercised too, and in NoF-only mode
  (CLIENT_GLOBAL_SEGMENT_SIZE=0) the script additionally requires that the
  same client resumes successful put/get after recovery. Running it needs an
  SPDK build, hugepages and root, none of which are available here.

[Copilot]

🟡 Changes recommended

The new reconnect backoff logic can retry immediately after a long blocking reconnect attempt, defeating the intended throttling during extended outages.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR addresses NVMe-oF (NoF) I/O recovery after a target outage/restart by detecting failed IO qpairs in cached NoF segment handles and attempting reconnection (with controller reset fallback) so both master heartbeat probes and client I/O can resume without restarting the process.

Changes:

• Add NoF qpair health-check (IsNofSegmentFailed) and recovery (ReconnectNofSegment) APIs to SpdkWrapper, including controller-reset fallback.
• Reconnect failed NoF segments from the owning SpdkNofWorkerPool thread before submitting queued I/O, with per-segment throttling.
• Extend the NoF heartbeat TCP E2E to include a target restart + re-registration phase and verify recovery.

File summaries

File	Description
mooncake-store/include/spdk/spdk_wrapper.h	Declares new NoF qpair failure detection and reconnection APIs.
mooncake-store/src/spdk/spdk_wrapper.cpp	Implements qpair failure detection and reconnect/reset logic; probes now reconnect before heartbeat read.
mooncake-store/src/transfer_task.cpp	Adds worker-thread reconnection attempt with a per-segment backoff before submitting queued I/O.
mooncake-store/tests/e2e/run_nof_heartbeat_tcp_e2e.sh	Extends E2E to restart the target and assert the master/client recover without repeated unmount.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[cage-goat]

Thanks for the PR! I have a few comments below:

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

   Could we rename ReconnectNofSegment() to TryRecoverNofSegment()? It also checks health and may reset the controller. The "NoF io qpair reconnected" log should include the target endpoint and namespace ID so we can identify which connection recovered.

2. mooncake-store/src/spdk/spdk_wrapper.cpp:478

   Resetting the controller here is unsafe: ns_mutex does not prevent other workers from accessing it during the reset, since their I/O paths do not take this lock. The reset may also invalidate cached namespace pointers that are still used afterward. Could we coordinate all users of this controller and refresh or invalidate its cached namespace handles as part of recovery?

3. mooncake-store/src/transfer_task.cpp:568–571

   This synchronous reconnect can block the worker while the target is unavailable, preventing it from submitting or polling I/O for healthy segments sharing the same worker. The retry backoff does not limit how long each attempt blocks. Could we make recovery non-blocking so one failed segment does not stall the others?

[pjdurden]

@cage-goat thanks, all three were fair. Pushed a49f294 (rebased on main):

1. Renamed to TryRecoverNofSegment(). Recovery logs now include the target endpoint (traddr:trsvcid/subnqn) and the nsid.

2. Added a per-controller std::shared_mutex io_mutex. The worker submit round, completion polling and the heartbeat probe all hold it shared, and the controller reset holds it exclusively, so the reset no longer runs while another worker is on that controller. After a successful reset the cached ns pointers of every namespace on that controller are refreshed under the same lock (and dropped if the nsid is no longer active), and GetBlockSize() now returns a cached value so callers outside the workers don't touch ns at all. Each worker then reconnects its own qpair on its own thread, since that's where SPDK wants it and where the aborted requests complete.

3. The reset moved to a background recovery thread in SpdkWrapper. Workers only try-lock: if a reset is queued or running they skip that segment for the round and keep serving the rest, and TryRecoverNofSegment() itself never connects to a target that hasn't just answered a reset. The tradeoff is that every recovery is now a full controller reset, because telling a dropped io connection apart from a lost association would need a blocking connect. Updated the description with that and the other caveats.

Built against the SPDK v23.01.1 headers with -fsyntax-only, but I have no NoF target here, so the e2e is the real check.

[cage-goat]

Thanks for working on this issue. I opened an alternative implementation in #4656 for #3863. Linking it here to make the overlap and design considerations clear for reviewers.

The main considerations were:

• Coordinate recovery across all namespaces and workers sharing a controller: wait for each owner worker to drain and free its qpairs before reconnecting the controller, then let the workers recreate their qpairs.
• Minimize locking on the I/O submission path: bind segments to workers during Open and use worker-local recovery state to gate submission, avoiding a controller lock on each submission.

The PR also includes an integration test covering controller recovery and qpair reconnection after a target restart with I/O outstanding.

[pjdurden]

merged current main in (34be261). the conflict was #3424 deleting spdk_wrapper.{h,cpp}, so the recovery moved into SpdkInitiator::Impl: the per-controller io_mutex, reset_generation and background reset thread live there, and ~Impl stops that thread before tearing down the controllers. NVMeoFInitiator gains TryLockSegmentIo, IsSegmentFailed and TryRecoverSegment with defaults that never refuse and never fail, so the test stub initiators are unchanged. NofWorkerPool::workerThread calls them through initiator_ and keeps main's cached nof_qos->block_size, ProbeSegment recovers before probing as before, and register_nof_segment in the e2e uses main's MOONCAKE_PYTHON_BIN / mooncake.store invocation. I could not compile it here (no SPDK), so CI is the first build of this version.