From ca1cfd5f4c49f1a948b3af7bd9e63d303102c35e Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 17 Sep 2026 02:00:58 +0000 Subject: [PATCH] kvm: fix storage pool refcount race and misreported umount result 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 --- .../kvm/storage/LibvirtStorageAdaptor.java | 51 +++++++++------- .../storage/LibvirtStorageAdaptorTest.java | 58 +++++++++++++++++++ 2 files changed, 89 insertions(+), 20 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java index 059f4f8b67af..97157f6374f5 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java @@ -700,27 +700,22 @@ public KVMPhysicalDisk getPhysicalDisk(String volumeUuid, KVMStoragePool pool) { * adjust refcount */ private int adjustStoragePoolRefCount(String uuid, int adjustment) { - final String mutexKey = storagePoolRefCounts.keySet().stream() - .filter(k -> k.equals(uuid)) - .findFirst() - .orElse(uuid); - synchronized (mutexKey) { - // some access on the storagePoolRefCounts.key(mutexKey) element - int refCount = storagePoolRefCounts.computeIfAbsent(mutexKey, k -> 0); - refCount += adjustment; - if (refCount < 1) { - storagePoolRefCounts.remove(mutexKey); - } else { - storagePoolRefCounts.put(mutexKey, refCount); - } - return refCount; - } + /* + * compute() is atomic for the key, so concurrent callers cannot lose an + * update. Returning null from the remapping function removes the entry, + * which keeps the map free of pools that are no longer in use. + */ + Integer refCount = storagePoolRefCounts.compute(uuid, (key, count) -> { + int adjusted = (count == null ? 0 : count) + adjustment; + return adjusted < 1 ? null : adjusted; + }); + return refCount == null ? 0 : refCount; } /** * Thread-safe increment storage pool usage refcount * @param uuid UUID of the storage pool to increment the count */ - private void incStoragePoolRefCount(String uuid) { + protected void incStoragePoolRefCount(String uuid) { adjustStoragePoolRefCount(uuid, 1); } /** @@ -728,7 +723,7 @@ private void incStoragePoolRefCount(String uuid) { * @param uuid UUID of the storage pool to decrement the count * @return true if the storage pool is still used, else false. */ - private boolean decStoragePoolRefCount(String uuid) { + protected boolean decStoragePoolRefCount(String uuid) { return adjustStoragePoolRefCount(uuid, -1) > 0; } @@ -948,13 +943,29 @@ public boolean deleteStoragePool(String uuid) { String targetPath = _mountPoint + File.separator + uuid; logger.error("deleteStoragePool removed pool from libvirt, but libvirt had trouble unmounting the pool. Trying umount location " + targetPath + " again in a few seconds"); - String result = Script.runSimpleBashScript("sleep 5 && umount " + targetPath); - if (result == null) { + /* + * runSimpleBashScript() returns null both when the command fails, + * because runScript() discards the output on a non-zero exit, and + * when it succeeds without printing anything. Its result therefore + * cannot say whether the umount worked. It is still used to run the + * umount, because it logs the failure reason, which is the useful + * diagnostic, but the outcome is taken from whether the path is + * still a mount point. That also covers the pool having been + * unmounted by something else in the meantime. + */ + Script.runSimpleBashScript("sleep 5 && umount " + targetPath); + if (Script.runSimpleBashScriptForExitValue("mountpoint -q " + targetPath) != 0) { logger.info("Succeeded in unmounting " + targetPath); destroyStoragePoolHandleException(conn, uuid); return true; } - logger.error("Failed to unmount " + targetPath); + /* + * Do not throw here. deleteStoragePool() is called from finally + * blocks, where a throw would discard the result of an operation + * that has already succeeded. + */ + logger.error("Failed to unmount " + targetPath + ", it is still a mount point"); + return false; } throw new CloudRuntimeException(e.toString(), e); } diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptorTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptorTest.java index 88346abd0176..a91e9e045054 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptorTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptorTest.java @@ -22,11 +22,20 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.never; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.UUID; +import java.util.concurrent.BrokenBarrierException; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; import org.junit.After; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -176,4 +185,53 @@ public void testUpdateLocalPoolIops_NullResultFromScript() { Mockito.verify(mockPool, never()).setUsedIops(anyLong()); } + + @Test(timeout = 120000) + public void testStoragePoolRefCountCountsEveryConcurrentIncrement() throws Exception { + final LibvirtStorageAdaptor adaptor = new LibvirtStorageAdaptor(null); + final int threads = 16; + final int rounds = 500; + final CyclicBarrier barrier = new CyclicBarrier(threads); + final ExecutorService executor = Executors.newFixedThreadPool(threads); + + try { + for (int round = 0; round < rounds; round++) { + // A fresh uuid each round, so every round starts with no entry for the pool. + final String uuid = String.valueOf(UUID.randomUUID()); + final List> futures = new ArrayList<>(); + + for (int i = 0; i < threads; i++) { + futures.add(executor.submit(() -> { + /* + * Every caller arrives with its own String instance, the way the + * agent does when the uuid is parsed out of a separate command + * payload for each request. The instances are equal but they are + * not the same object. + */ + final String ownInstance = new String(uuid); + try { + barrier.await(); + } catch (InterruptedException | BrokenBarrierException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException(e); + } + adaptor.incStoragePoolRefCount(ownInstance); + })); + } + for (Future future : futures) { + future.get(60, TimeUnit.SECONDS); + } + + // Every increment must be counted, so the pool stays in use until the last release. + for (int i = 1; i < threads; i++) { + Assert.assertTrue("Round " + round + ": pool should still be in use after " + i + + " of " + threads + " releases", adaptor.decStoragePoolRefCount(uuid)); + } + Assert.assertFalse("Round " + round + ": pool should no longer be in use after the last release", + adaptor.decStoragePoolRefCount(uuid)); + } + } finally { + executor.shutdownNow(); + } + } }