kvm: fix storage pool refcount race and false umount success

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java

@@ -700,35 +700,30 @@ 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);
     }
     /**
      * Thread-safe decrement storage pool usage refcount for the given uuid and return if storage pool still in use.
      * @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);
         }

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<Future<?>> 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();
+        }
+    }
 }