[DNM] nvme: fused keepalive (#3811)#3995
Open
babayet2 wants to merge 3 commits into
Open
Conversation
Backport of the nvme_workaround feature (squashed commit 28c70c99) onto release 1.7.2511. Includes: - Fused NVMe keepalive device detection and handling (mod.rs, device.rs, manager.rs, driver.rs, queue_pair.rs). - Concurrent NVMe device restore in NvmeManagerWorker::restore via try_join_all. Backport deviations from the feature branch (due to mainline drift absent from the release branch): - driver.rs restore uses the unconditional eager sorted_io path (the feature's actual runtime behavior, since allow_lazy_restore is always Some(false)); the allow_lazy_restore saved-state field and the lazy proto_io restore split are not introduced. - Unstable Vec::push_mut is replaced with push + last_mut. - The phantom-completion admin-CQ diagnostic is dropped, as it depends on the mainline request_diagnostic_dump plumbing not present on the release branch. - The added VMM servicing tests in openhcl_servicing.rs are dropped, as they depend on a newer fault-injection framework (HardwareConfigFaultConfig, TDISP, run_cpu_pinned_io, nvme_test resolver changes) absent from the release branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 002f00db-6e37-4182-b06a-57deb3e7c0a5
github-actions
Bot
added
the
release_1.7.2511
Contributor
|
Couple things:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a “fused keepalive” compatibility mode to the NVMe driver to handle devices whose admin queue becomes unusable after servicing/restore, and wires device detection + flag plumbing through Underhill’s NVMe manager/spawner stack.
Changes:
- Extend
NvmeDriverinitialization/restore paths with afused_keepalive_deviceflag and adjust admin/AER + IO-queue handling accordingly. - Add enforcement/diagnostics for “no admin commands after restore” behavior (AER disabled, async-event task skipped, command-forbidden panic path).
- Propagate the new mode through tests, fuzzing, and Underhill device detection/restore creation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| vm/devices/storage/storage_tests/tests/scsidvd_nvme.rs | Updates test driver construction to match new NvmeDriver::new signature. |
| vm/devices/storage/disk_nvme/nvme_driver/src/tests.rs | Adjusts unit tests to pass the new fused-keepalive flag. |
| vm/devices/storage/disk_nvme/nvme_driver/src/queue_pair.rs | Adds device-id context to pending commands and introduces “commands forbidden” + disabled AER handler support. |
| vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs | Implements fused-keepalive mode behavior across init, restore, async events, and IO queue restore/claiming. |
| vm/devices/storage/disk_nvme/nvme_driver/fuzz/fuzz_nvme_driver.rs | Adds fuzz coverage for the new fused-keepalive flag. |
| openhcl/underhill_core/src/nvme_manager/save_restore_helpers.rs | Updates saved-state test helper to include the new unmapped field. |
| openhcl/underhill_core/src/nvme_manager/mod.rs | Adds sysfs-based detection for fused-keepalive devices. |
| openhcl/underhill_core/src/nvme_manager/manager.rs | Threads fused-keepalive detection into create/restore flows; restores devices concurrently via try_join_all. |
| openhcl/underhill_core/src/nvme_manager/device.rs | Propagates fused-keepalive flag into VFIO driver creation and driver-manager state. |
Comment on lines
+1039
to
1042
| worker.io = sorted_io | ||
| .into_iter() | ||
| .flat_map(|q| -> Result<IoQueue<D>, anyhow::Error> { | ||
| let qid = q.queue_data.qid; |
mattkur
reviewed
Jul 22, 2026
| pub queue_data: QueuePairSavedState, | ||
| #[mesh(4)] | ||
| /// When `true`, the queue has not yet been affinitized to its cpu. | ||
| pub unmapped: bool, |
Contributor
There was a problem hiding this comment.
Note: this adds a new field to save state that we have already shipped. This is safe, since we cannot support keepalive (and thus this save state) to or from a version of OpenHCL without these changes.
If we did support such servicing, then this would be unsafe, since it is possible that a queue "loses" its unmapped status.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad2f5a78-7441-43d3-9679-9de10f68a9c3
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
openhcl/underhill_core/src/nvme_manager/mod.rs:136
- On sysfs read failure,
is_nvme_fused_keepalive_device()currently returnstrue, which makes all devices fall into fused keepalive mode (disabling AER/admin activity) even though the device IDs are unknown. This can mask environment/setup problems and unnecessarily degrade functionality for non-fused devices.
pci_id = %pci_id,
error = err.as_ref() as &dyn std::error::Error,
"failed to read PCI vendor/device IDs; treating device as a fused keepalive device"
);
true
Comment on lines
+1025
to
+1029
| // Restore all IO queues eagerly. Queues are sorted by interrupt vector | ||
| // for ordered VPci allocation (MSI-X ordering workaround). | ||
| // | ||
| // Devnote: Safety of inline new_self_drained(): This loop is fully | ||
| // synchronous (no .await). Although IoQueue::restore() spawns queue |
babayet2
changed the title
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.
This is a backport of #3811 to 1.7. This backport has in-progress scale testing, but should be reviewed as an independent PR, it is an AI-assisted manual backport, not a clean cherry pick
This version contains an unintentionally contains eager-restore for IO queues. Leaving the PR, for now, but marking as DNM