Skip to content

Commit d1e3ea0

Browse files
bhouse-nexthopbhouse-nexthop
committed
kvm: fix storage pool refcount race and false umount success
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 entry is absent, orElse(uuid) returns the caller's own instance instead and the synchronized block guards nothing. The entry is absent exactly when the count has just dropped to zero, which is the moment the lock is there to protect. Increments are lost, the count reaches zero while the pool is still in use, and the agent tries to unmount a pool that other VMs are still using. On one host every one of the 22 teardowns in a day failed with "device is busy". Use ConcurrentHashMap.compute(), which is atomic for the key, and drop the lock. deleteStoragePool() treats a null return from runSimpleBashScript() as a successful umount, but null means either that the command failed or that it succeeded and printed nothing on stdout. umount reports its errors on stderr, so a failed umount is indistinguishable from a successful one and is logged and returned as a success. Check the exit status instead. Signed-off-by: Brad House <bhouse@nexthop.ai>
1 parent 10037c8 commit d1e3ea0

2 files changed

Lines changed: 77 additions & 20 deletions

File tree

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

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -700,35 +700,30 @@ public KVMPhysicalDisk getPhysicalDisk(String volumeUuid, KVMStoragePool pool) {
700700
* adjust refcount
701701
*/
702702
private int adjustStoragePoolRefCount(String uuid, int adjustment) {
703-
final String mutexKey = storagePoolRefCounts.keySet().stream()
704-
.filter(k -> k.equals(uuid))
705-
.findFirst()
706-
.orElse(uuid);
707-
synchronized (mutexKey) {
708-
// some access on the storagePoolRefCounts.key(mutexKey) element
709-
int refCount = storagePoolRefCounts.computeIfAbsent(mutexKey, k -> 0);
710-
refCount += adjustment;
711-
if (refCount < 1) {
712-
storagePoolRefCounts.remove(mutexKey);
713-
} else {
714-
storagePoolRefCounts.put(mutexKey, refCount);
715-
}
716-
return refCount;
717-
}
703+
/*
704+
* compute() is atomic for the key, so concurrent callers cannot lose an
705+
* update. Returning null from the remapping function removes the entry,
706+
* which keeps the map free of pools that are no longer in use.
707+
*/
708+
Integer refCount = storagePoolRefCounts.compute(uuid, (key, count) -> {
709+
int adjusted = (count == null ? 0 : count) + adjustment;
710+
return adjusted < 1 ? null : adjusted;
711+
});
712+
return refCount == null ? 0 : refCount;
718713
}
719714
/**
720715
* Thread-safe increment storage pool usage refcount
721716
* @param uuid UUID of the storage pool to increment the count
722717
*/
723-
private void incStoragePoolRefCount(String uuid) {
718+
protected void incStoragePoolRefCount(String uuid) {
724719
adjustStoragePoolRefCount(uuid, 1);
725720
}
726721
/**
727722
* Thread-safe decrement storage pool usage refcount for the given uuid and return if storage pool still in use.
728723
* @param uuid UUID of the storage pool to decrement the count
729724
* @return true if the storage pool is still used, else false.
730725
*/
731-
private boolean decStoragePoolRefCount(String uuid) {
726+
protected boolean decStoragePoolRefCount(String uuid) {
732727
return adjustStoragePoolRefCount(uuid, -1) > 0;
733728
}
734729

@@ -948,13 +943,19 @@ public boolean deleteStoragePool(String uuid) {
948943
String targetPath = _mountPoint + File.separator + uuid;
949944
logger.error("deleteStoragePool removed pool from libvirt, but libvirt had trouble unmounting the pool. Trying umount location " + targetPath +
950945
" again in a few seconds");
951-
String result = Script.runSimpleBashScript("sleep 5 && umount " + targetPath);
952-
if (result == null) {
946+
/*
947+
* The exit status is what says whether the umount worked. umount
948+
* reports its errors on stderr, so a failed umount produces no
949+
* output and cannot be told apart from a successful one by
950+
* looking at the output alone.
951+
*/
952+
int exitValue = Script.runSimpleBashScriptForExitValue("sleep 5 && umount " + targetPath);
953+
if (exitValue == 0) {
953954
logger.info("Succeeded in unmounting " + targetPath);
954955
destroyStoragePoolHandleException(conn, uuid);
955956
return true;
956957
}
957-
logger.error("Failed to unmount " + targetPath);
958+
logger.error("Failed to unmount " + targetPath + ", umount exited with status " + exitValue);
958959
}
959960
throw new CloudRuntimeException(e.toString(), e);
960961
}

‎plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptorTest.java‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,19 @@
2222
import static org.mockito.ArgumentMatchers.anyString;
2323
import static org.mockito.Mockito.never;
2424

25+
import java.util.ArrayList;
2526
import java.util.HashMap;
27+
import java.util.List;
2628
import java.util.Map;
2729
import java.util.UUID;
30+
import java.util.concurrent.BrokenBarrierException;
31+
import java.util.concurrent.CyclicBarrier;
32+
import java.util.concurrent.ExecutorService;
33+
import java.util.concurrent.Executors;
34+
import java.util.concurrent.Future;
2835

2936
import org.junit.After;
37+
import org.junit.Assert;
3038
import org.junit.Before;
3139
import org.junit.Test;
3240
import org.junit.runner.RunWith;
@@ -176,4 +184,52 @@ public void testUpdateLocalPoolIops_NullResultFromScript() {
176184

177185
Mockito.verify(mockPool, never()).setUsedIops(anyLong());
178186
}
187+
188+
@Test
189+
public void testStoragePoolRefCountCountsEveryConcurrentIncrement() throws Exception {
190+
final int threads = 16;
191+
final int rounds = 500;
192+
final CyclicBarrier barrier = new CyclicBarrier(threads);
193+
final ExecutorService executor = Executors.newFixedThreadPool(threads);
194+
195+
try {
196+
for (int round = 0; round < rounds; round++) {
197+
// A fresh uuid each round, so every round starts with no entry for the pool.
198+
final String uuid = String.valueOf(UUID.randomUUID());
199+
final List<Future<?>> futures = new ArrayList<>();
200+
201+
for (int i = 0; i < threads; i++) {
202+
futures.add(executor.submit(() -> {
203+
/*
204+
* Every caller arrives with its own String instance, the way the
205+
* agent does when the uuid is parsed out of a separate command
206+
* payload for each request. The instances are equal but they are
207+
* not the same object.
208+
*/
209+
final String ownInstance = new String(uuid);
210+
try {
211+
barrier.await();
212+
} catch (InterruptedException | BrokenBarrierException e) {
213+
Thread.currentThread().interrupt();
214+
throw new IllegalStateException(e);
215+
}
216+
libvirtStorageAdaptor.incStoragePoolRefCount(ownInstance);
217+
}));
218+
}
219+
for (Future<?> future : futures) {
220+
future.get();
221+
}
222+
223+
// Every increment must be counted, so the pool stays in use until the last release.
224+
for (int i = 1; i < threads; i++) {
225+
Assert.assertTrue("Round " + round + ": pool should still be in use after " + i
226+
+ " of " + threads + " releases", libvirtStorageAdaptor.decStoragePoolRefCount(uuid));
227+
}
228+
Assert.assertFalse("Round " + round + ": pool should no longer be in use after the last release",
229+
libvirtStorageAdaptor.decStoragePoolRefCount(uuid));
230+
}
231+
} finally {
232+
executor.shutdownNow();
233+
}
234+
}
179235
}

0 commit comments

Comments
 (0)