kvm: fix storage pool refcount race and false umount success by bhouse-nexthop · Pull Request #14189 · apache/cloudstack · GitHub

Description

Two bugs in the KVM agent's storage pool teardown path. They are independent but they show up together, so they are fixed together.

Bug 1 - the storage pool refcount is not thread safe

final String mutexKey = storagePoolRefCounts.keySet().stream()
        .filter(k -> k.equals(uuid))
        .findFirst()
        .orElse(uuid);          // entry absent -> the CALLER'S OWN String instance
synchronized (mutexKey) { ... }

The lock is meant to be the one String instance held as the map key, so that all callers share a monitor. When the map has no entry for the pool, orElse(uuid) hands back the caller's own String instead. Two threads in that state synchronize on two different objects and the block guards nothing.

Which callers can actually collide:

	reached through	serialized?
increment	createStoragePool → KVMStoragePoolManager.createStoragePool	yes, that method is synchronized
decrement	deleteStoragePool → KVMStoragePoolManager.deleteStoragePool	no, that method is not

So two increments never race. The reachable losing interleaving is an increment against a decrement while the map has no entry for the pool: they take different monitors, and the decrement's remove() erases the increment. The count then reaches zero while the pool is still in use.

Observed on one KVM host over one day:

	count
deleteStoragePool calls	5,651
correctly skipped, pool still in use	5,629
proceeded to unmount	22
of those, failed with device is busy	22

All 22 failed. If the count were right, reaching zero would mean nothing holds the mount and the unmount would succeed. They also arrive in bursts, four threads tearing down the same pool inside 20 seconds:

08:52:04,623 (AgentRequest-Handler-14) deleteStoragePool ... had trouble unmounting the pool
08:52:12,647 (AgentRequest-Handler-18) deleteStoragePool ... had trouble unmounting the pool
08:52:20,647 (AgentRequest-Handler-23) deleteStoragePool ... had trouble unmounting the pool
08:52:24,612 (AgentRequest-Handler-27) deleteStoragePool ... had trouble unmounting the pool

Bug 2 - a failed umount is logged and returned as success

String result = Script.runSimpleBashScript("sleep 5 && umount " + targetPath);
if (result == null) {
    logger.info("Succeeded in unmounting " + targetPath);
    destroyStoragePoolHandleException(conn, uuid);
    return true;
}

Script.runScript() returns null in two different situations:

the command failed, in which case the output is discarded
the command succeeded and printed nothing

umount prints nothing on success, so result is always null and this branch always reported success. From an agent log, 28 ms apart:

INFO   Succeeded in unmounting /mnt/<uuid>
ERROR  Failed to destroy libvirt pool <uuid>: ... unexpected exit status 16: umount.nfs4: /mnt/<uuid>: device is busy

Fix

bug	fix
refcount race	use ConcurrentHashMap.compute(), which is atomic per key, and drop the broken lock
false success	take the outcome from whether the path is still a mount point, and return false instead of throwing

The umount is still run through runSimpleBashScript(), because that is what logs the reason the umount failed (device is busy and so on) and that line is the useful diagnostic. Only the decision moved: mountpoint -q says whether the pool is actually unmounted. That also makes "something else already unmounted it" a success rather than a failure.

deleteStoragePool() returns false on a genuine failure rather than throwing. It is called from finally blocks in LibvirtCopyVolumeCommandWrapper, LibvirtPrimaryStorageDownloadCommandWrapper and LibvirtComputingResource.templateToPrimaryDownload, and a throw from a finally discards the result of the operation that just succeeded. The only caller that reads the returned boolean is LibvirtModifyStoragePoolCommandWrapper, which uses the overload that takes pool details and is not on this path.

compute() also removes the entry by returning null, so the map still drops pools that are no longer in use. The behaviour seen by callers is unchanged: decStoragePoolRefCount() still reports whether the pool is still in use.

incStoragePoolRefCount and decStoragePoolRefCount become protected so the behaviour can be tested. They already are protected on main.

What this does not fix

Two related problems in the same path are left alone, to keep this change small. Both are worth fixing separately.

The decrement is not atomic with the teardown. compute() makes each adjustment atomic; it does not make "decrement reached zero" atomic with the unmount that follows it. Between decStoragePoolRefCount() returning false and destroyStoragePool() running, there is a libvirt connect and a secret lookup, and another thread can take the pool again in that window. That still ends in device is busy.
A refcount leak in createStoragePool. incStoragePoolRefCount is inside a try whose only catch is LibvirtException, but checkNetfsStoragePoolMounted and getStoragePool both throw CloudRuntimeException. On that path the compensating decrement is skipped and the count leaks by one, permanently, so the pool is never unmounted. This is the opposite failure to the one fixed here.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
build/CI

How Has This Been Tested?

Added testStoragePoolRefCountCountsEveryConcurrentIncrement to LibvirtStorageAdaptorTest.

What it does, per round:

take a fresh pool uuid, so the map starts with no entry for it
16 threads meet at a CyclicBarrier and then increment the refcount at once
each thread passes its own String instance, equal to the others but not the same object, the way the agent does when the uuid is parsed out of a separate command payload per request
release the pool 16 times: the first 15 releases must report it as still in use, the 16th must report it as free

It runs 500 rounds, because the window only exists while the map has no entry for the pool. Once an entry is there, the key set lookup does find a shared instance and the lock works.

Results:

	outcome
old code	fails, first seen at round 4: pool should still be in use after 15 of 16 releases
with this change	passes, repeated runs

It is a probabilistic detector, not a deterministic one. It needs real parallelism, so its power depends on the runner: it caught the bug on every run on 6 or more cores, around 6 runs in 10 on 4 cores, and not at all on 2 cores. It never fails with the fix applied.

The test uses a plain LibvirtStorageAdaptor rather than the class's shared Mockito @Spy, because routing 16,000 concurrent calls through Mockito's invocation recorder adds a lock of its own that could mask the race being tested.

Full KVM plugin test suite on this branch: 677 tests, 0 failures, 1 skipped.

Codecov Report

❌ Patch coverage is 70.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 18.13%. Comparing base (8074c3e) to head (6ed63c9).
⚠️ Report is 6 commits behind head on 4.22.

Files with missing lines	Patch %	Lines
.../hypervisor/kvm/storage/LibvirtStorageAdaptor.java	72.22%	5 Missing ⚠️
...ud/hypervisor/kvm/storage/KVMStoragePoolLocks.java	57.14%	2 Missing and 1 partial ⚠️
.../hypervisor/kvm/storage/KVMStoragePoolManager.java	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               4.22   #14189   +/-   ##
=========================================
  Coverage     18.13%   18.13%           
- Complexity    16445    16455   +10     
=========================================
  Files          5939     5940    +1     
  Lines        537306   537319   +13     
  Branches      65867    65868    +1     
=========================================
+ Hits          97439    97469   +30     
+ Misses       428749   428729   -20     
- Partials      11118    11121    +3

Flag	Coverage Δ
uitests	4.04% <ø> (ø)
unittests	19.22% <70.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vladimirpetrov @sureshanaparti could you take a look at this one? We'd like this fix to make it into the upcoming 4.22.2 release.

thanks @bhouse-nexthop, fix looks right. CLGTM

Two bugs in the storage pool teardown path. adjustStoragePoolRefCount() means to lock on the single String instance held as the map key, so that all callers share a monitor. When the map has no entry for the pool, orElse(uuid) returns the caller's own instance instead and the synchronized block guards nothing. Increments cannot race each other: they happen inside createStoragePool, which is only reached through KVMStoragePoolManager.createStoragePool, and that is synchronized. Decrements are not covered, because KVMStoragePoolManager.deleteStoragePool is not. So an increment and a decrement can run at the same time, and while the map has no entry for the pool they take different monitors and the decrement's remove() can erase the increment. The count then reaches zero while the pool is still in use. Use ConcurrentHashMap.compute(), which is atomic for the key. deleteStoragePool() decided whether the retried umount had worked from the return of runSimpleBashScript(), which is null both when the command fails, because runScript() discards the output on a non-zero exit, and when it succeeds without printing anything. A failed umount was therefore logged and returned as a success. Take the outcome from whether the path is still a mount point, which also covers the pool having been unmounted by something else in the meantime, and return false rather than throwing when it is still mounted: deleteStoragePool() is called from finally blocks, where a throw would discard the result of an operation that has already succeeded. Signed-off-by: Brad House <bhouse@nexthop.ai>

The refcount is atomic, but it cannot keep a pool alive on its own. deleteStoragePool() drops the last reference and then goes on to destroy and unmount the pool without holding anything. createStoragePool() is serialised by KVMStoragePoolManager, but deleteStoragePool() is not, so a create can find the still active pool and take a reference after the delete has seen the count reach zero, and the delete then tears the pool down underneath it. Take a monitor per pool uuid for the whole of createStoragePool() and deleteStoragePool(). Every delete path ends in deleteStoragePool(String), including LibvirtStoragePool.delete(), so locking there covers callers that bypass the manager. Pools other than the one being deleted are not held up, which matters because a delete can sit in the five second umount retry. Signed-off-by: Brad House <bhouse@nexthop.ai>

The previous commit took the per-pool lock inside LibvirtStorageAdaptor.createStoragePool(), which is only reached through KVMStoragePoolManager.createStoragePool(), and that is synchronized on the manager. A create of a pool that is being torn down therefore waited for the teardown while holding the manager wide lock. The umount retry in deleteStoragePool() runs with no timeout, so a umount hung on storage that has gone away held up the creates of every pool on the host, not just the one being deleted. Take the pool's lock in the manager first, before its own monitor, so a create waiting for a teardown holds nothing that other pools need. The locks move to KVMStoragePoolLocks so the manager and the adaptor share them; the adaptor still takes the same lock, reentrantly on the create path and on its own for deletes, including LibvirtStoragePool.delete(), which does not go through the manager. Signed-off-by: Brad House <bhouse@nexthop.ai>

@DaanHoogland @weizhouapache could one of you review this for 4.22.2? @Damans227 approved it on 10-07 and I rebased it onto current 4.22 today; it still needs a committer review and a test run. I understand 4.22.2 is due out late this week or early next.

@blueorangutan package

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19686

@blueorangutan test

@bhouse-nexthop , your findings are valueable and you fixes look sound; my only worry is the use of mountpoint as system executable. I might be seeing bears on the road here, but is it available on all supported platforms?

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

@DaanHoogland thanks. It's a fair thing to check, but this doesn't add a new dependency. LibvirtStorageAdaptor already runs mountpoint -q in two places on 4.22. checkNetfsStoragePoolMounted() and the stale-mount check in createNetfsStoragePool() both came in with #8887 in 4.19.1, and this change reuses the same call. mountpoint is part of util-linux, which is a base package on every distro the KVM agent supports (EL8/9/10, Ubuntu/Debian, SUSE). It's the same package that provides umount itself.

One thing to note: if the binary were ever missing, the shell would return 127 and the new check would read the pool as unmounted. That failure would be easy to spot, though. With mountpoint missing, checkNetfsStoragePoolMounted() would already fail every NFS pool create on that host, so the agent couldn't use NFS primary storage at all.

clgtm

[SF] Trillian test result (tid-17224)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 57354 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr14189-t17224-kvm-ol8.zip
Smoke tests completed. 149 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

Hi @bhouse-nexthop ,

I went through PR #14189 and noticed the two follow-up issues you mentioned:

The race between reference count decrement and storage pool teardown.
The potential reference count leak in createStoragePool() when a CloudRuntimeException occurs.

I'd be interested in investigating the second issue first, particularly adding regression tests for the exception-handling and reference count cleanup paths.

Has this issue already been tracked or addressed separately? If not, would you be comfortable with me investigating it and submitting a separate fix?

Thanks!

Test report: storage pool refcount and the false umount success, on a KVM zone

Environment

A CloudStack 24.0 (main) build with this PR applied, alongside some of my other open PRs.
Three KVM hosts on Ubuntu 24.04, nested virtualisation; Ceph RBD primary storage (zone-wide); NFS secondary storage.
25 small qcow2 templates registered for this test, each used once, so every deploy below is a template's first copy to primary storage. That copy mounts the template's directory from secondary storage under /mnt/<uuid>, and unmounts it when the copy is done.
On a zone-wide pool, the management server can send the copy to any host, whichever host the VM goes to. So every check below covers all three hosts.

1. A busy mount is reported as a failed unmount

Each host ran a watcher that cds into any new secondary mount under /mnt and holds it for 15 minutes.
I deployed one VM from a fresh template.

The deploy succeeded. The agent that ran the copy logged:

ERROR deleteStoragePool removed pool from libvirt, but libvirt had trouble unmounting the pool. Trying umount location /mnt/<uuid> again in a few seconds
WARN  ... sleep 5 && umount /mnt/<uuid> ... umount.nfs4: /mnt/<uuid>: device is busy
ERROR Failed to unmount /mnt/<uuid>, it is still a mount point

There was no Succeeded in unmounting line. The busy mount was reported as a failure, and the command still finished.
The mount stays until something unmounts it; I unmounted it by hand once the watcher was gone. The description says that is out of scope.
Without the PR (from the description's production logs): Succeeded in unmounting, followed about 28 ms later by an ERROR device is busy.

2. Parallel first copies: no busy unmounts, no leaked mounts

Two rounds of parallel deploys, each from its own fresh template: 8 in the first round and 7 in the second. With nothing holding the mounts:

Round	Deploys Running	Copies (create / remove pool), by host	Removed cleanly	had trouble unmounting / device is busy	Mounts under /mnt after
A	8 / 8	8 / 8: 5, 1 and 2	8	0 / 0	0 new
B	7 / 7	7 / 7: 2, 4 and 1	7	0 / 0	0 new

Every copy's pool was created, used and removed from libvirt, with no fallback umount needed.
The description's production baseline is 22 of 22 copies failing the unmount.
Copies that ran at the same time on one host each had their own mount (one directory per template), so the refcount never had to hold a pool open across two users here. The PR's unit tests cover a shared pool.

Not covered: the lock-stall check. The jstack snapshots I took during the rounds are unusable because of a mistake in my capture script, so I make no claim about cross-pool lock waits. The PR's unit tests cover the locking.

Result: pass.

A busy mount is reported as a failed unmount, not a false success.
Parallel first copies show no busy-unmount failures, and no mount is left behind.

Hi @bhouse-nexthop , @DaanHoogland

I investigated the separate storage pool refcount leak mentioned in your PR description and have submitted a follow-up PR to address it: #14694.

The fix ensures that the reference count is decremented if storage pool creation fails after the increment, including failures that throw CloudRuntimeException.

I've also added a regression test covering the getStoragePool() failure case.

This change is intentionally scoped to the refcount leak and does not modify the race or unmount fixes in your PR.

Thanks for highlighting this issue!

@dheeraj12347 thanks for following up on the refcount leak. I left some notes on #14694: the fix looks right, but as written it depends on this PR's createStoragePoolLocked and the now-protected ref count methods, so it doesn't compile against 4.22 on its own.

boring-cyborg Bot added the component:kvm label Sep 17, 2026

bhouse-nexthop force-pushed the fix-nfs-storage-pool-refcount-and-umount branch from d1e3ea0 to ca1cfd5 Compare September 17, 2026 02:37

bhouse-nexthop requested review from DaanHoogland and weizhouapache September 18, 2026 00:50

bhouse-nexthop requested review from sureshanaparti and vladimirpetrov October 1, 2026 13:03

bhouse-nexthop force-pushed the fix-nfs-storage-pool-refcount-and-umount branch from ca1cfd5 to f7bfe93 Compare October 1, 2026 15:27

Damans227 reviewed Oct 1, 2026

View reviewed changes

Comment thread ...ns/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java Resolved

bhouse-nexthop requested a review from Damans227 October 1, 2026 20:31

Damans227 reviewed Oct 2, 2026

View reviewed changes

Comment thread ...ns/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java Resolved

bhouse-nexthop requested a review from Damans227 October 2, 2026 18:49

Damans227 approved these changes Oct 7, 2026

View reviewed changes

bhouse-nexthop added 3 commits October 22, 2026 13:10

bhouse-nexthop force-pushed the fix-nfs-storage-pool-refcount-and-umount branch from 7e093df to 6ed63c9 Compare October 22, 2026 13:12

DaanHoogland approved these changes Oct 23, 2026

View reviewed changes

DaanHoogland added the status:needs-testing label Oct 23, 2026

dheeraj12347 mentioned this pull request Nov 12, 2026

KVM: Fix storage pool refcount leak on creation failure #14694

Open

2 tasks done

kvm: fix storage pool refcount race and false umount success by bhouse-nexthop · Pull Request #14189 · apache/cloudstack · GitHub

Repository navigation

kvm: fix storage pool refcount race and false umount success - #14189

Conversation

bhouse-nexthop commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Fix

What this does not fix

Types of changes

How Has This Been Tested?

Uh oh!

codecov Bot commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bhouse-nexthop commented Oct 1, 2026

Uh oh!

Uh oh!

Uh oh!

Damans227 left a comment • 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!

bhouse-nexthop commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaanHoogland commented Oct 22, 2026

Uh oh!

blueorangutan commented Oct 22, 2026

Uh oh!

blueorangutan commented Oct 22, 2026

Uh oh!

DaanHoogland commented Oct 23, 2026

Uh oh!

DaanHoogland commented Oct 23, 2026

Uh oh!

blueorangutan commented Oct 23, 2026

Uh oh!

bhouse-nexthop commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Oct 24, 2026

Uh oh!

dheeraj12347 commented Nov 8, 2026

Uh oh!

bhouse-nexthop commented Nov 9, 2026

Test report: storage pool refcount and the false umount success, on a KVM zone

Uh oh!

dheeraj12347 commented Nov 12, 2026

Uh oh!

bhouse-nexthop commented Nov 12, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bhouse-nexthop commented Sep 17, 2026 •
edited
Loading

codecov Bot commented Sep 17, 2026 •
edited
Loading

Damans227 left a comment •
edited
Loading

bhouse-nexthop commented Oct 22, 2026 •
edited
Loading

bhouse-nexthop commented Oct 23, 2026 •
edited
Loading