DaanHoogland commented on code in PR #9498:
URL: https://github.com/apache/cloudstack/pull/9498#discussion_r1762515483


##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java:
##########
@@ -637,6 +638,38 @@ public KVMPhysicalDisk getPhysicalDisk(String volumeUuid, 
KVMStoragePool pool) {
         }
     }
 
+    /**
+     * Thread-safe increment storage pool usage refcount
+     * @param uuid UUID of the storage pool to increment the count
+     */
+    private void incStoragePoolRefCount(String uuid) {
+        synchronized (storagePoolRefCounts) {
+            int refCount = storagePoolRefCounts.computeIfAbsent(uuid, k -> 0);
+            refCount += 1;
+            storagePoolRefCounts.put(uuid, refCount);
+        }
+    }
+
+    /**
+     * 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) {
+        synchronized (storagePoolRefCounts) {
+            Integer refCount = storagePoolRefCounts.get(uuid);
+            if (refCount != null && refCount > 1) {
+                s_logger.debug(String.format("Storage pool %s still in use, 
refCount %d", uuid, refCount));
+                refCount -= 1;
+                storagePoolRefCounts.put(uuid, refCount);
+                return true;
+            } else {
+                storagePoolRefCounts.remove(uuid);
+                return false;
+            }
+        }
+    }

Review Comment:
   two remarks:
   1. Can you considder a threadsafe collection ? i.e. concurrent map.
   2. the map can be guarded (as concurrent map or by your own synchronised 
blocks) but I don't think that will guard its elements from being guarded, only 
the structure of the map itself.
   3. (there is always a third) blocking access to the map globally may impact 
performance (though in this case that would have to be on very big hosts.
   
   I think you are right about making the map static, btw.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to