nvmeof: Add mount cache and locking for safe nvme disconnect by gadididi · Pull Request #6192 · ceph/ceph-csi

gadididi · 2026-03-22T15:03:53Z

Describe what this PR does

Adds an in-memory cache to track which nvme devices are currently mounted at staging paths. This eliminates expensive findmnt --list system calls on every unstage operation.

Structure:
The cache maintains bidirectional maps for fast lookups:

pathToDevice: staging path -> device path
deviceToPath: device path -> staging path (reverse)

Initialization:
Cache is populated at node server startup by scanning existing mounts, then kept up to date as volumes are staged and unstaged.

Thread Safety:
Protected by sync.Mutex. We use a simple mutex instead of sync.RWMutex because:

Operations are very fast ( map lookups or remove. just 1 call for get copy , which it is called once per delete pod.. )
Read/write ratio is - not read-heavy

Why copying the map is safe:
When GetAllDevices() returns a copy of the cache, callers get a consistent snapshot at that moment. The copy happens after cache updates (first, the pod unmounts, then deletes itself, and last fetch the copy of cache), so concurrent unstage operations each see current state. Even if two unstages run simultaneously, each removes its device from the cache first, then gets a fresh snapshot showing remaining devices.
So the last of them who call to GetAllDevices() will get the update list, and will know if disconnect or not. (NodeStage cannot run at this time !!)

Related PR##

after this pr: #6183 will be merged, group locking (between NodeStage\NodeUnStage) will added.

Future concerns

next step is to add group lock after #6183 will be merged.

Checklist:

Commit Message Formatting: Commit titles and messages follow
guidelines in the developer
guide.
Reviewed the developer guide on Submitting a Pull
Request
Pending release
notes
updated with breaking and/or notable changes for the next major release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

/retest ci/centos/<job-name>: retest the <job-name> after unrelated
failure (please report the failure too!)

Copilot

Pull request overview

Adds an in-memory mount cache for NVMe-oF staging mounts so NodeUnstage can avoid repeated findmnt --list calls and decide when it’s safe to disconnect NVMe controllers.

Changes:

Changed GetAllNVMeMountedDevices() to return device -> stagingTarget mappings (instead of a bool set).
Introduced MountCache (bidirectional device/path cache) and initialized it at NodeServer startup.
Updated NodeUnstage disconnect decision flow to use the in-memory cache snapshot.

Reviewed changes

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

File	Description
internal/nvmeof/util/mounter.go	Changes mounted-device discovery to return device→mountpoint mapping.
internal/nvmeof/util/mounter_cache.go	Adds new mutex-protected mount cache with bidirectional maps.
internal/nvmeof/nvmeof_initiator.go	Updates disconnect interfaces/helpers to accept the new mounted-devices map type.
internal/nvmeof/nodeserver/nodeserver.go	Initializes cache on startup; updates stage/unstage flows to maintain and consult the cache for disconnect decisions.

Copilot · 2026-03-23T07:34:13Z

 	transaction.isMounted = true

+	// Resolve real device and update cache
+	realDevice, err := nvmeutil.GetDeviceFromMountpoint(ctx, stagingTargetPath)
+	if err != nil {
+		log.WarningLog(ctx, "failed to resolve device: %v", err)
+		// Fallback - try to continue
+		realDevice = devicePath
+	}
+
+	ns.mountCache.Add(realDevice, stagingTargetPath)


Between the successful mount (transaction.isMounted = true) and the later ns.mountCache.Add call, there is a window where the namespace is mounted in the system but missing from mountCache. Because NodeStage/NodeUnstage are only locked per-volume (volumeLocks), a concurrent unstage of a different namespace in the same subsystem could take a snapshot that omits this mount and incorrectly disconnect shared controllers. To avoid false negatives, consider updating mountCache immediately after the mount succeeds (using the best-available device identity) or adding a safety validation (e.g., re-check mounts via findmnt) before disconnect decisions.

Suggested change

transaction.isMounted = true

// Resolve real device and update cache

realDevice, err := nvmeutil.GetDeviceFromMountpoint(ctx, stagingTargetPath)

if err != nil {

log.WarningLog(ctx, "failed to resolve device: %v", err)

// Fallback - try to continue

realDevice = devicePath

}

ns.mountCache.Add(realDevice, stagingTargetPath)

// Immediately record the mount in the cache using the best-available device identity

ns.mountCache.Add(devicePath, stagingTargetPath)

// Resolve real device and refine cache entry if possible

realDevice, err := nvmeutil.GetDeviceFromMountpoint(ctx, stagingTargetPath)

if err != nil {

log.WarningLog(ctx, "failed to resolve device: %v", err)

// Fallback - keep using devicePath for cache

} else if realDevice != "" && realDevice != devicePath {

// Update cache to use the canonical device identifier for this mountpoint

ns.mountCache.RemoveByMountPoint(stagingTargetPath)

ns.mountCache.Add(realDevice, stagingTargetPath)

}

transaction.isMounted = true

Because NodeStage/NodeUnstage are only locked per-volume (volumeLocks), a concurrent unstage of a different namespace in the same subsystem could take a snapshot that omits this mount and incorrectly disconnect shared controllers

As I said in the PR description, in the next PR I will add group lock(which locks per group type stage\unstage) . Once this PR will be merged: #6183

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

nixpanic · 2026-03-27T11:29:53Z

+// MountCache is a thread-safe cache that maintains the mapping between staging paths and devices.
+// Note: This map is 1:1, meaning each staging path corresponds to exactly one device.
+// device can only be mounted to one staging path at a time, and each staging path can only have one device.
+type MountCache struct {


Can you provide a MountCache interface and mountCache struct that contains the actual implementation?

nixpanic · 2026-03-27T11:46:10Z

 	}

+	// Initialize mounted devices cache on startup to ensure we have an accurate view
+	mountedDevices, err := getNVMeMountedDevices(context.Background())


Make getNVMeMountedDevices() a function of NodeServer (call it initNVMeMountedDevices()?) and add the path/device directly in that function. This removes the for-loop a little below and makes this function a little smaller.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

nixpanic · 2026-04-08T16:49:27Z

@Mergifyio rebase

mergify · 2026-04-08T16:49:55Z

Deprecation notice: This pull request comes from a fork and was rebased using bot_account impersonation. This capability will be removed on July 1, 2026. After this date, the rebase action will no longer be able to rebase fork pull requests with this configuration. Please switch to the update action/command to ensure compatibility going forward.

mergify · 2026-04-08T16:49:56Z

rebase

✅ Branch has been successfully rebased

nixpanic · 2026-04-08T16:50:01Z

/test ci/centos/mini-e2e/k8s-1.35

Madhu-1 · 2026-04-09T05:44:01Z

@Mergifyio rebase

Madhu-1 · 2026-04-09T05:44:08Z

@Mergifyio queue

mergify · 2026-04-09T05:44:31Z

rebase

☑️ Nothing to do, the required conditions are not met

Details

any of:
- #commits-behind > 0 [📌 rebase requirement]
- -linear-history [📌 rebase requirement]
-closed [📌 rebase requirement]
-conflict [📌 rebase requirement]
queue-position = -1 [📌 rebase requirement]

mergify · 2026-04-09T05:44:32Z

Merge Queue Status

🛑 Queue command has been cancelled

ceph-csi-bot · 2026-04-09T07:24:29Z

/test ci/centos/k8s-e2e-external-storage/1.35

ceph-csi-bot · 2026-04-09T07:24:30Z

/test ci/centos/k8s-e2e-external-storage/1.34

ceph-csi-bot · 2026-04-09T07:24:30Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2026-04-09T07:24:30Z

/test ci/centos/mini-e2e-helm/k8s-1.35

ceph-csi-bot · 2026-04-09T07:24:30Z

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2026-04-09T07:24:30Z

/test ci/centos/mini-e2e-helm/k8s-1.34

ceph-csi-bot · 2026-04-09T07:24:31Z

/test ci/centos/k8s-e2e-external-storage/1.33

ceph-csi-bot · 2026-04-09T07:24:31Z

/test ci/centos/mini-e2e/k8s-1.35

ceph-csi-bot · 2026-04-09T07:24:31Z

/test ci/centos/mini-e2e/k8s-1.34

ceph-csi-bot · 2026-04-09T07:24:31Z

/test ci/centos/mini-e2e-helm/k8s-1.33

ceph-csi-bot · 2026-04-09T07:24:32Z

/test ci/centos/mini-e2e/k8s-1.33

mergify · 2026-04-09T10:25:43Z

Deprecation notice: This pull request comes from a fork and was queued with update_method=rebase and update_bot_account impersonation. This capability will be removed on July 1, 2026. After this date, the merge queue will no longer be able to rebase fork pull requests with this configuration. To avoid disruption, switch to update_method=merge in your queue rule.

mergify · 2026-04-09T10:25:51Z

Merge Queue Status

✅ Entered queue — 2026-04-09 10:25 UTC · Rule: default
✅ Checks skipped · PR is already up-to-date
✅ Merged — 2026-04-09 10:25 UTC · at 2a614e0a43d24b515412b2f3376ba5856fd68b1d

This pull request spent 15 seconds in the queue, including 1 second running CI.

Required conditions to merge

#approved-reviews-by >= 2 [🛡 GitHub branch protection]
- nvmeof: Add mount cache and locking for safe nvme disconnect #6192
#changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
- nvmeof: Add mount cache and locking for safe nvme disconnect #6192

gadididi requested a review from nixpanic March 22, 2026 15:03

gadididi self-assigned this Mar 22, 2026

gadididi added the component/nvme-of Issues and PRs related to NVMe-oF. label Mar 22, 2026

gadididi requested review from Rakshith-R and Copilot March 23, 2026 06:14

Copilot started reviewing on behalf of gadididi March 23, 2026 07:30 View session

Copilot AI reviewed Mar 23, 2026

View reviewed changes

gadididi force-pushed the nvmeof/add_mnt_cache_nodeserver branch 2 times, most recently from 8673fba to 077fce8 Compare March 24, 2026 14:22

gadididi requested a review from Copilot March 24, 2026 14:24

Copilot started reviewing on behalf of gadididi March 24, 2026 14:25 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

Comment thread internal/nvmeof/nodeserver/nodeserver.go

Comment thread internal/nvmeof/util/mounter.go Outdated

Comment thread internal/nvmeof/nodeserver/nodeserver.go Outdated

gadididi force-pushed the nvmeof/add_mnt_cache_nodeserver branch from 077fce8 to 9558efb Compare March 25, 2026 07:46

nixpanic reviewed Mar 27, 2026

View reviewed changes

gadididi force-pushed the nvmeof/add_mnt_cache_nodeserver branch 2 times, most recently from 5e46539 to 6d03f95 Compare March 30, 2026 09:31

gadididi requested a review from nixpanic March 30, 2026 09:31

gadididi mentioned this pull request Mar 30, 2026

nvmeof: Add GroupLock to coordinate stage and unstage operations and e2e tests #6210

Merged

6 tasks

nixpanic reviewed Mar 30, 2026

View reviewed changes

Comment thread internal/nvmeof/nodeserver/nodeserver.go

gadididi force-pushed the nvmeof/add_mnt_cache_nodeserver branch 2 times, most recently from 53bba11 to 2fe4143 Compare March 30, 2026 11:59

gadididi requested a review from nixpanic March 30, 2026 12:23

gadididi force-pushed the nvmeof/add_mnt_cache_nodeserver branch from 2fe4143 to 0803810 Compare March 30, 2026 12:23

gadididi requested a review from Copilot March 30, 2026 12:40

Copilot started reviewing on behalf of gadididi March 30, 2026 12:40 View session

Copilot AI reviewed Mar 30, 2026

View reviewed changes

Comment thread internal/nvmeof/nodeserver/nodeserver.go Outdated

gadididi force-pushed the nvmeof/add_mnt_cache_nodeserver branch from 0803810 to 353a958 Compare April 7, 2026 06:47

gadididi requested review from a team and removed request for Rakshith-R April 7, 2026 06:47

ceph-csi-bot force-pushed the nvmeof/add_mnt_cache_nodeserver branch from 353a958 to 2a614e0 Compare April 8, 2026 16:49

nixpanic approved these changes Apr 8, 2026

View reviewed changes

gadididi requested a review from a team April 8, 2026 20:09

Madhu-1 approved these changes Apr 9, 2026

View reviewed changes

nixpanic added the ok-to-test Label to trigger E2E tests label Apr 9, 2026

ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Apr 9, 2026

mergify Bot added the queued label Apr 9, 2026

mergify Bot merged commit 736dc70 into ceph:devel Apr 9, 2026
41 checks passed

gadididi deleted the nvmeof/add_mnt_cache_nodeserver branch April 9, 2026 10:26

mergify Bot removed the queued label Apr 9, 2026

Conversation

gadididi commented Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe what this PR does

Related PR##

Future concerns

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!

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

gadididi Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nixpanic Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

gadididi Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nixpanic Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

gadididi Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

nixpanic commented Apr 8, 2026

Uh oh!

mergify Bot commented Apr 8, 2026

Uh oh!

mergify Bot commented Apr 8, 2026

✅ Branch has been successfully rebased

Uh oh!

nixpanic commented Apr 8, 2026

Uh oh!

Madhu-1 commented Apr 9, 2026

Uh oh!

Madhu-1 commented Apr 9, 2026

Uh oh!

mergify Bot commented Apr 9, 2026

☑️ Nothing to do, the required conditions are not met

Uh oh!

mergify Bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Uh oh!

ceph-csi-bot commented Apr 9, 2026

Uh oh!

ceph-csi-bot commented Apr 9, 2026

Uh oh!

ceph-csi-bot commented Apr 9, 2026

Uh oh!

ceph-csi-bot commented Apr 9, 2026

Uh oh!

ceph-csi-bot commented Apr 9, 2026

Uh oh!

ceph-csi-bot commented Apr 9, 2026

gadididi commented Mar 22, 2026 •

edited

Loading

mergify Bot commented Apr 9, 2026 •

edited

Loading

mergify Bot commented Apr 9, 2026 •

edited

Loading