Repository navigation
kvm: fix storage pool refcount race and false umount success - #14189
bhouse-nexthop wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d1e3ea0 to
ca1cfd5
Compare
|
@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. |
ca1cfd5 to
f7bfe93
Compare
There was a problem hiding this comment.
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
7e093df to
6ed63c9
Compare
|
@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. 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 |
|
[SF] Trillian test result (tid-17224)
|
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
The lock is meant to be the one
Stringinstance 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 ownStringinstead. Two threads in that state synchronize on two different objects and the block guards nothing.Which callers can actually collide:
createStoragePool→KVMStoragePoolManager.createStoragePoolsynchronizeddeleteStoragePool→KVMStoragePoolManager.deleteStoragePoolSo 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:
deleteStoragePoolcallsdevice is busyAll 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:
Bug 2 - a failed umount is logged and returned as success
Script.runScript()returnsnullin two different situations:umountprints nothing on success, soresultis always null and this branch always reported success. From an agent log, 28 ms apart:Fix
ConcurrentHashMap.compute(), which is atomic per key, and drop the broken lockfalseinstead of throwingThe umount is still run through
runSimpleBashScript(), because that is what logs the reason the umount failed (device is busyand so on) and that line is the useful diagnostic. Only the decision moved:mountpoint -qsays whether the pool is actually unmounted. That also makes "something else already unmounted it" a success rather than a failure.deleteStoragePool()returnsfalseon a genuine failure rather than throwing. It is called fromfinallyblocks inLibvirtCopyVolumeCommandWrapper,LibvirtPrimaryStorageDownloadCommandWrapperandLibvirtComputingResource.templateToPrimaryDownload, and a throw from afinallydiscards the result of the operation that just succeeded. The only caller that reads the returned boolean isLibvirtModifyStoragePoolCommandWrapper, which uses the overload that takes pool details and is not on this path.compute()also removes the entry by returningnull, 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.incStoragePoolRefCountanddecStoragePoolRefCountbecomeprotectedso the behaviour can be tested. They already areprotectedonmain.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. BetweendecStoragePoolRefCount()returning false anddestroyStoragePool()running, there is a libvirt connect and a secret lookup, and another thread can take the pool again in that window. That still ends indevice is busy.A refcount leak in
createStoragePool.incStoragePoolRefCountis inside atrywhose onlycatchisLibvirtException, butcheckNetfsStoragePoolMountedandgetStoragePoolboth throwCloudRuntimeException. 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
How Has This Been Tested?
Added
testStoragePoolRefCountCountsEveryConcurrentIncrementtoLibvirtStorageAdaptorTest.What it does, per round:
CyclicBarrierand then increment the refcount at onceStringinstance, 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 requestIt 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:
pool should still be in use after 15 of 16 releasesIt 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
LibvirtStorageAdaptorrather 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.