nvme_driver: update driver to use `Arc` / `Weak` semantics to enforce unique ownership for namespaces by nsid #2657

gurasinghMS · 2026-01-16T06:40:30Z

This PR fixes a couple of problems with the driver.

1. `Namespace` persisting and being reused after namespace removal from the controller

.
It addresses the situation where a namespace is removed from the controller (and therefore from the disk exposed via VTL0), but the driver continues to hold a reference to that namespace. In this state, the driver would return a stale or invalid Namespace object—one that can no longer perform valid I/O.

The approach in this PR is to avoid keeping a strong Arc in the driver. Instead, the driver stores only a Weak for any namespace it hands out. When the disk is dropped, the strong reference disappears, causing the driver’s weak reference to naturally invalidate via normal Arc/Weak semantics. This eliminates the dangling reference issue without requiring additional bookkeeping.

As discussed during office hours, the window for failure with this approach is expected to be extremely narrow, as the driver relies on strict single-ownership of the Namespace object. To further enforce this single-ownership model, this PR introduces a non-cloneable wrapper around Arc. This wrapper exposes the same interface as Namespace and is consumed by the disk, but it prevents cloning of the underlying Arc anywhere along the creation path. This ensures the driver remains the sole authority over Namespace ownership and lifecycle.

2. `Namespace` being reused after disk removal from VTL2 settings.

This is the issue being described in #2656 . This PR brings back the check to verify single-ownership of the Namespace objects. This time around it is possible to verify using the above described Arc/Weak semantics.

alandau · 2026-01-16T23:26:38Z

In general, Arc (shared ownership) is more brittle than unique ownership, and here we're going a step further of sometimes storing an Arc and sometimes a Weak, which is even more brittle. That said, this looks like an elegant way out, if you can prove (via tests and/or code review) that it covers all situations.

gurasinghMS · 2026-01-17T02:43:36Z

In general, Arc (shared ownership) is more brittle than unique ownership, and here we're going a step further of sometimes storing an Arc and sometimes a Weak, which is even more brittle. That said, this looks like an elegant way out, if you can prove (via tests and/or code review) that it covers all situations.

Like you, I also have some concerns around the brittleness of this approach. Although in the classical sense of memory safety and availability I think rust should have us covered!

gurasinghMS · 2026-01-20T23:08:51Z

vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs

-                return Ok(handle.namespace.clone());
-            }
+    pub async fn namespace(&mut self, nsid: u32) -> Result<NamespaceHandle, NamespaceError> {
+        if let Some(namespace) = self.namespaces.get_mut(&nsid) {


I wonder if there is a more idiomatic way to do this ..... it looks a little off to me.

Copilot

Pull request overview

This PR refactors the NVMe driver to use Arc/Weak semantics to address two critical issues: (1) namespace persistence after removal from the controller, and (2) namespace reuse after disk removal from VTL2 settings. The solution introduces a non-cloneable NamespaceHandle wrapper around Arc<Namespace> to enforce single-ownership, while the driver stores only Weak references internally.

Changes:

Introduced NamespaceHandle wrapper to prevent cloning and enforce single ownership of namespaces
Implemented WeakOrStrong enum to manage namespace lifecycle during normal operation and save/restore
Updated all callsites to use NamespaceHandle instead of Arc<Namespace>

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
vm/devices/storage/disk_nvme/nvme_driver/src/namespace.rs	Added NamespaceHandle wrapper with delegated methods; made check_active() public; renamed error variant
vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs	Implemented WeakOrStrong enum; updated namespace() method with duplicate detection; modified save() logic
vm/devices/storage/disk_nvme/nvme_driver/src/lib.rs	Updated exports to use NamespaceHandle instead of Namespace
vm/devices/storage/disk_nvme/src/lib.rs	Updated NvmeDisk to use NamespaceHandle
openhcl/underhill_core/src/nvme_manager/*.rs	Updated all interfaces to use NamespaceHandle

vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs

Copilot · 2026-01-20T23:31:16Z

vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs

+                    let is_weak = namespace.is_weak(); // This value will change after invokeing get_arc().
+                    if let Some(ns) = namespace.get_arc()
+                        && ns.check_active().is_ok()
+                        && is_weak


The condition is_weak in the save logic appears incorrect. When saving right after a restore (before any namespace() calls), all namespaces will be Strong, causing is_weak to be false for all of them. This means none of the restored namespaces will be saved, which breaks save-restore cycles.

The logic should save any namespace that can be successfully obtained via get_arc() and passes the check_active() test, regardless of whether it started as Weak or Strong. Consider removing the && is_weak condition or inverting it to && !is_weak if the intent is to only save Strong references (pre-handout namespaces).

Suggested change

let is_weak = namespace.is_weak(); // This value will change after invokeing get_arc().

if let Some(ns) = namespace.get_arc()

&& ns.check_active().is_ok()

&& is_weak

if let Some(ns) = namespace.get_arc()

&& ns.check_active().is_ok()

This is a good point so I am keeping this comment open. The idea behind NOT saving strong ref namespaces is that they were never requested for by a driver and hence there is no reasonable expectation that they should be available post restore.
Current code cuts down the size of saved data. As copilot pointed out, repeated back-to-back servicing where the namespace has not yet been requested could cause missed saves. However, I don't think we have a reasonable expectation for hammering a system with servicing operations .

…ferences to the Namespace object uncloneable

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

github-actions · 2026-01-21T20:59:01Z

At least one Petri test failed.

github-actions · 2026-01-22T02:28:36Z

At least one Petri test failed.

github-actions · 2026-01-22T05:33:39Z

At least one Petri test failed.

github-actions · 2026-01-22T18:35:46Z

At least one Petri test failed.

github-actions · 2026-01-22T20:12:52Z

At least one Petri test failed.

alandau

Yay! All tests passed! Perhaps do a dummy change (or trigger another test run without a change if there's a way to do it) to make sure it's not flaky and won't hunt us down the road.

mattkur · 2026-01-25T20:53:29Z

Yay! All tests passed! Perhaps do a dummy change (or trigger another test run without a change if there's a way to do it) to make sure it's not flaky and won't hunt us down the road.

I triggered a new test run by merging in main. Once you're comfy with this, @alandau, I'll approve and this can go in. Thanks for taking on the review of this change :)

alandau · 2026-01-26T18:41:46Z

@mattkur, it was consistently failing on every run, and now it passed on 2 runs in a row, and the branch is out of date again, so another run will be needed before merging. I think this is good to go, unless @gurasinghMS has reservations.

alandau · 2026-01-26T19:36:51Z

openvmm_openhcl_linux_x64_many_nvme_devices_servicing_very_heavy failed - not sure if it's related. The hot-add-remove test passed.

github-actions · 2026-01-26T19:46:47Z

At least one Petri test failed.

… unique ownership for namespaces by nsid (microsoft#2657) This PR fixes a couple of problems with the driver. ## 1. `Namespace` persisting and being reused after namespace removal from the controller ![Figjamexport2](https://github.com/user-attachments/assets/3a098676-9c4d-428d-981c-6880ddaa7262). It addresses the situation where a namespace is removed from the controller (and therefore from the disk exposed via VTL0), but the driver continues to hold a reference to that namespace. In this state, the driver would return a stale or invalid Namespace object—one that can no longer perform valid I/O. The approach in this PR is to avoid keeping a strong Arc<Namespace> in the driver. Instead, the driver stores only a Weak<Namespace> for any namespace it hands out. When the disk is dropped, the strong reference disappears, causing the driver’s weak reference to naturally invalidate via normal Arc/Weak semantics. This eliminates the dangling reference issue without requiring additional bookkeeping. As discussed during office hours, the window for failure with this approach is expected to be extremely narrow, as the driver relies on strict single-ownership of the Namespace object. To further enforce this single-ownership model, this PR introduces a non-cloneable wrapper around Arc<Namespace>. This wrapper exposes the same interface as Namespace and is consumed by the disk, but it prevents cloning of the underlying Arc<Namespace> anywhere along the creation path. This ensures the driver remains the sole authority over Namespace ownership and lifecycle. ## 2. `Namespace` being reused after disk removal from VTL2 settings. <img width="7096" height="2979" alt="image" src="https://github.com/user-attachments/assets/f519db98-447c-4a95-a7a2-c70536e07ae0" /> This is the issue being described in microsoft#2656 . This PR brings back the check to verify single-ownership of the `Namespace` objects. This time around it is possible to verify using the above described `Arc`/`Weak` semantics. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Matt LaFayette (Kurjanowicz) <mattkur@microsoft.com> Co-authored-by: Alex Landau <alexlandau@microsoft.com> (cherry picked from commit 34cc2bb)

… unique ownership for namespaces by nsid (#2657) (#2697) Clean cherry pick of PR #2657 This PR fixes a couple of problems with the driver. ## 1. `Namespace` persisting and being reused after namespace removal from the controller ![Figjamexport2](https://github.com/user-attachments/assets/3a098676-9c4d-428d-981c-6880ddaa7262). It addresses the situation where a namespace is removed from the controller (and therefore from the disk exposed via VTL0), but the driver continues to hold a reference to that namespace. In this state, the driver would return a stale or invalid Namespace object—one that can no longer perform valid I/O. The approach in this PR is to avoid keeping a strong Arc<Namespace> in the driver. Instead, the driver stores only a Weak<Namespace> for any namespace it hands out. When the disk is dropped, the strong reference disappears, causing the driver’s weak reference to naturally invalidate via normal Arc/Weak semantics. This eliminates the dangling reference issue without requiring additional bookkeeping. As discussed during office hours, the window for failure with this approach is expected to be extremely narrow, as the driver relies on strict single-ownership of the Namespace object. To further enforce this single-ownership model, this PR introduces a non-cloneable wrapper around Arc<Namespace>. This wrapper exposes the same interface as Namespace and is consumed by the disk, but it prevents cloning of the underlying Arc<Namespace> anywhere along the creation path. This ensures the driver remains the sole authority over Namespace ownership and lifecycle. ## 2. `Namespace` being reused after disk removal from VTL2 settings. <img width="7096" height="2979" alt="image" src="https://github.com/user-attachments/assets/f519db98-447c-4a95-a7a2-c70536e07ae0" /> This is the issue being described in #2656 . This PR brings back the check to verify single-ownership of the `Namespace` objects. This time around it is possible to verify using the above described `Arc`/`Weak` semantics. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Matt LaFayette (Kurjanowicz) <mattkur@microsoft.com> Co-authored-by: Alex Landau <alexlandau@microsoft.com>

mattkur · 2026-01-28T21:29:07Z

Backported to release/1.7.2511 in #2697

gurasinghMS mentioned this pull request Jan 16, 2026

nvme_driver: Remove the handle_in_use check from the Namespace references held by the nvme driver #2656

Closed

gurasinghMS changed the title ~~[wip] handle dangling references when controller namespaces are removed (Part 2)~~ [wip] handle dangling references when controller namespaces are removed Jan 16, 2026

mattkur added the backport_1.7.2511 label Jan 16, 2026

gurasinghMS commented Jan 20, 2026

View reviewed changes

gurasinghMS changed the title ~~[wip] handle dangling references when controller namespaces are removed~~ [nvme_driver] update driver to use Arc / Weak semantics to enforce single-owner semantics for namespaces Jan 20, 2026

gurasinghMS changed the title ~~[nvme_driver] update driver to use Arc / Weak semantics to enforce single-owner semantics for namespaces~~ [nvme_driver] update driver to use Arc / Weak semantics to enforce unique ownership for namespaces Jan 20, 2026

gurasinghMS changed the title ~~[nvme_driver] update driver to use Arc / Weak semantics to enforce unique ownership for namespaces~~ [nvme_driver] update driver to use Arc / Weak semantics to enforce unique ownership for namespaces by nsid Jan 20, 2026

gurasinghMS marked this pull request as ready for review January 20, 2026 23:26

gurasinghMS requested review from a team as code owners January 20, 2026 23:26

Copilot AI review requested due to automatic review settings January 20, 2026 23:26

Copilot started reviewing on behalf of gurasinghMS January 20, 2026 23:26 View session

Copilot AI reviewed Jan 20, 2026

View reviewed changes

gurasinghMS and others added 11 commits January 20, 2026 16:16

first pass

f8eabd1

no longer using strong ref counting

8ee51ca

Added a first pass on the strong/weak pointer approach

edee1bc

weak pointer strong pointer approach

1d6fe56

Now implemented a thin wrapper around Arc<Namespace> so as to make re…

7175d6c

…ferences to the Namespace object uncloneable

v1 implementation with the is strong and is weak stuff

5259989

More minor cleanup

ddeb8dd

Uncommenting prep steps

2fb91b0

Update vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs

db97283

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs

f4229c2

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs

0af769f

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

gurasinghMS force-pushed the handle-controller-namespaces-leaving branch from e903762 to 0af769f Compare January 21, 2026 00:17

gurasinghMS added 2 commits January 20, 2026 16:33

uncomment test for disk removal to verify changes in CI

127b0db

Remove unused import

b18dfce

gurasinghMS changed the title ~~[nvme_driver] update driver to use Arc / Weak semantics to enforce unique ownership for namespaces by nsid~~ nvme_driver: update driver to use Arc / Weak semantics to enforce unique ownership for namespaces by nsid Jan 21, 2026

gurasinghMS and others added 3 commits January 21, 2026 11:31

Merge branch 'main' into handle-controller-namespaces-leaving

ec20c27

doc build fixes

8ae9b20

Forgot to save some changes

7647119

gurasinghMS and others added 2 commits January 21, 2026 17:16

Added more debugging lines to verify namespace recreation

b357f1c

Merge branch 'main' into handle-controller-namespaces-leaving

a46466e

Fixed debug output issue

5cdb667

Merge branch 'main' into handle-controller-namespaces-leaving

98b4064

gurasinghMS added 2 commits January 22, 2026 11:04

Increasing the retries for finding device along with longer sleep

2e70d85

Added a log line with debugging in storvsp as wellg

1ff6ed4

Increase retries to find a disk and reduce total number of iterations

2f0af48

alandau approved these changes Jan 23, 2026

View reviewed changes

Merge branch 'main' into handle-controller-namespaces-leaving

7be1e20

Merge branch 'main' into handle-controller-namespaces-leaving

9e52f8c

mattkur approved these changes Jan 26, 2026

View reviewed changes

mattkur merged commit 34cc2bb into microsoft:main Jan 26, 2026
80 of 82 checks passed

gurasinghMS mentioned this pull request Jan 27, 2026

nvme_driver: update driver to use Arc / Weak semantics to enforce unique ownership for namespaces by nsid (#2657) #2697

Merged

mattkur added backported_1.7.2511 PR that has been backported to release/1.7.2511 and removed backport_1.7.2511 labels Jan 28, 2026

nvme_driver: update driver to use Arc / Weak semantics to enforce unique ownership for namespaces by nsid #2657

nvme_driver: update driver to use Arc / Weak semantics to enforce unique ownership for namespaces by nsid #2657

Uh oh!

Conversation

gurasinghMS commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Namespace persisting and being reused after namespace removal from the controller

2. Namespace being reused after disk removal from VTL2 settings.

Uh oh!

alandau commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gurasinghMS commented Jan 17, 2026

Uh oh!

gurasinghMS Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

gurasinghMS Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 21, 2026

Uh oh!

github-actions bot commented Jan 22, 2026

Uh oh!

github-actions bot commented Jan 22, 2026

Uh oh!

github-actions bot commented Jan 22, 2026

Uh oh!

github-actions bot commented Jan 22, 2026

Uh oh!

alandau left a comment

Choose a reason for hiding this comment

Uh oh!

mattkur commented Jan 25, 2026 • edited by alandau Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alandau commented Jan 26, 2026

Uh oh!

alandau commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 26, 2026

Uh oh!

Uh oh!

mattkur commented Jan 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nvme_driver: update driver to use `Arc` / `Weak` semantics to enforce unique ownership for namespaces by nsid #2657

nvme_driver: update driver to use `Arc` / `Weak` semantics to enforce unique ownership for namespaces by nsid #2657

gurasinghMS commented Jan 16, 2026 •

edited

Loading

1. `Namespace` persisting and being reused after namespace removal from the controller

2. `Namespace` being reused after disk removal from VTL2 settings.

alandau commented Jan 16, 2026 •

edited

Loading

gurasinghMS Jan 21, 2026 •

edited

Loading

mattkur commented Jan 25, 2026 •

edited by alandau

Loading

alandau commented Jan 26, 2026 •

edited

Loading