libnvme: rewrite the discovery walk as a depth-capped, deduped walker - #3837
Merged
Conversation
added 4 commits
August 13, 2026 12:30
Every fabrics hook is optional and every other call site in fabrics.c already guards accordingly. These are public APIs. A caller must be allowed to pass NULL for a hook it doesn't need. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
--force used to do double duty: force persistence, and skip reusing an existing connection. Now that --persistent=force exists as its own explicit option, --force only ever means the latter -- so the name no longer matches what it does. Add --no-reuse as the real name; keep --force as a deprecated alias so existing scripts and discovery.conf files keep working. Rename the backing fctx->force field and its accessors to match, and check_ctrl_owner()'s force parameter, which shares the same concept. Document --no-reuse on nvme-connect-all(1) too, which never had a --force entry at all. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
A DCNQNHOR cleared to 0h is spec-legal (NVMe Boot Specification rev 1.4): it means "no unique NQN, use the well-known Discovery NQN". get_heap_obj() reports that as -ENOENT, but read_discovery() treated any nonzero return as a hard parse failure and silently dropped the whole Discovery Descriptor -- including the common case, since most descriptors have no unique NQN at all. Tolerate -ENOENT for the NQN lookup specifically; a genuinely malformed reference (-EINVAL) still drops the descriptor. Once the descriptor's own nqn survives parsing, libnvmf_discover_nbft() can use it when present instead of always hardcoding the well-known Discovery NQN. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
discover_lookup_ctrl()/discover_lookup_ctrl_by_device() only ever
resolved a controller; libnvmf_discover() then had its own separate
create-fallback and its own already_connected bool to track what it
found. Fold both into dc_open(), returning enum dc_ownership
{DC_OWNED, DC_BORROWED} instead of the bool.
Also stop overwriting fctx->persistent to NO when --device doesn't
resolve to a real controller (not found, or found but not a discovery
controller). fctx->persistent must keep reflecting what the user
actually asked for.
Signed-off-by: Martin Belanger <martin.belanger@dell.com>
igaw
reviewed
Aug 14, 2026
igaw
reviewed
Aug 14, 2026
igaw
reviewed
Aug 14, 2026
igaw
reviewed
Aug 14, 2026
igaw
reviewed
Aug 14, 2026
igaw
reviewed
Aug 14, 2026
igaw
reviewed
Aug 14, 2026
igaw
reviewed
Aug 14, 2026
igaw
reviewed
Aug 14, 2026
igaw
reviewed
Aug 14, 2026
igaw
reviewed
Aug 14, 2026
Collaborator
|
Generally looks good, just a few nitpicks. The only big question is on the interpretation what 'referral entries deeper than 8 levels' means. |
added 2 commits
August 14, 2026 10:08
_nvmf_discover() had four real bugs, all living in code this rewrite already has to touch. An already-connected referral was skipped entirely, without ever walking its own referrals. A DC's self entry was identified by pointer identity (cl == c), which is wrong per the spec's SUBTYPE 03h semantics. A referral's KATO was requested after the connect already happened, too late to take effect. And hooks.connected never fired for anything discovered and connected during the walk itself. Rewrite it as a two-pass, depth-capped (NVMF_MAX_REFERRAL_DEPTH, 8 per spec), TID-deduped walk. Pass 1 finds the self entry and decides this DC's own disconnect fate via the new dc_decide() -- a pure function, unit-tested directly, that gives every DC (primary or referral) the same self-entry-driven decision, falling back to the parent's reported EFLAGS only when a referral has no self entry of its own. Pass 2 walks NVM subsystem entries (connect, never recurse) and referrals (depth-cap check, then visited-set check, then reuse-or-connect, then recurse), fixing all four bugs above along the way. New: dc_decide(), dc_already_connected() (dedups a hook-invocation pattern), the dc_visited TID set, dc_walk() (owns the visited-set lifetime so libnvmf_discover() doesn't need to know it exists). Deleted: dc_should_connect(), superseded by dc_decide() plus the explicit subtype branches in pass 2. Also drops nvmf_connect_disc_entry()'s dead NVME_NQN_CURR handling and its unused discover out-parameter -- self entries never reach it now that pass 1 filters them first. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
nbft_discovery() was a forked copy of _nvmf_discover()'s per-DC walk, missing the depth cap and visited-set the general path just gained. Replace it with dc_open()/dc_walk(), the same machinery discover and connect-all use, via a new private connect_leaf hook so NBFT's leaf-connect quirks (DHCP retry, firing hooks.connected) stay NBFT-only without forking the walk itself. NBFT never honors --no-reuse (dc_open() gains honor_no_reuse) and never respects --persistent (fctx->persistent forced to NO before the walk): boot discovery is a one-shot operation where reusing an existing connection is strictly better, and neither flag was ever consulted by the old code either. Two real, pre-existing bugs found and fixed along the way: host_iface was freed before nbft_discovery()'s walk read it from every per-entry connect during that DC's DLP walk (moot now -- ctrl_params_dup()'s __cleanup_ctrl_params already prevents this by construction); and the Discovery Descriptor connection's own DHCP retry cleared traddr (the destination) instead of host_traddr (the local address), which cannot ever succeed. Also adds lookup_live_ctrl(), deduplicating a lookup_ctrl() + name check repeated at four call sites, one of them new here; and dc_visited_register(), so a cycle back to a DC (including the primary itself) is recognized from its own real connected identity, not just whatever a referring entry happened to report about it. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
martin-belanger
force-pushed
the
discover-rewrite-v2
branch
from
August 14, 2026 14:11
b8518bb to
bc0ffbc
Compare
Collaborator
|
Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rewrites libnvme's discovery walk (
_nvmf_discover()) from scratch, replacing the closed #3828, and breaks it into smaller, logical helpers to make the flow easier to follow.Four main things in here:
--forcerenamed to--no-reuse(kept as a deprecated alias):--forceused to double as both "force persistence" and "skip reusing an existing connection"; now that--persistent=forceexists as its own explicit option,--forceonly means the latter, so its name no longer matches what's left of its job. This is also the widest-reaching commit -- the only one that touches CLI docs, generated accessor bindings, andsrc/fabrics.c, not justlibnvme/src/nvme/fabrics.c.nvme discover/connect-alluse, instead of a second, forked implementation of the same walk.Bugs found and fixed along the way
None of these were part of the original plan; all found while working through the surrounding code.
fctx->hooks.*calls (connected,already_connected,decide_retry) had no NULL check, even though every hook is documented-optional and every other call site in the file guards them. An external caller passing NULL for a hook it doesn't need would crash.libnvmf_discover_nbft()still never read the parsed NQN when present, always connecting with the well-known one instead.--devicedidn't resolve to a real controller,fctx->persistentgot silently forced toNO, discarding whatever the user actually asked for with--persistent.cl == c) instead of its spec-defined SUBTYPE.hooks.connectednever fired for anything discovered and connected during the walk itself --connect-allnever reported per-device progress for its own discovery-driven connects, even though the hook exists and fires correctly everywhere else.host_ifacewas freed before its own walk read it from every connect made during that walk (moot now --ctrl_params_dup()'s__cleanup_ctrl_paramsalready prevents this by construction).traddr) instead of the local one (host_traddr) -- a retry that could never succeed.Commits
--forceto--no-reuse----forceno longer means what its name says now that--persistent=forceexists separately;--forcekept as a deprecated alias.dc_open()-- one path for "reuse or create a discovery controller," used by discover, connect-all, and (later) NBFT._nvmf_discover()as a depth-capped, deduped walk -- the core change. Fixes four real bugs: an already-connected referral was skipped without ever walking its own referrals, a self entry was misidentified by pointer identity instead of SUBTYPE, KATO was requested too late to take effect, andhooks.connectednever fired for controllers discovered and connected during the walk. Adds a referral depth cap and a TID-based visited set for cycle detection.dc_decide()is a new pure, unit-tested function (9 cases intest-fabrics.c) driving every disconnect decision.nbft_discovery()was a forked copy of the walk logic, missing the depth cap and visited-set the general path just gained. Replaced with the samedc_open()/dc_walk()machinery via a new privateconnect_leafhook, fixing two more pre-existing bugs along the way (see commit message).Testing
meson compileclean under default,-Dwerror=true, and-Dfabrics=disabled.meson testpasses, excluding the pre-existing, unrelatedfabrics-mock-cliLD_PRELOAD failure that's also present on a clean master.test_dc_decide()unit test exercisesdc_decide()'s disconnect logic directly.