Skip to content

Commit a3a511b

Browse files
kvm: look RBD volumes up through librbd instead of refreshing the pool
getPhysicalDisk() asks libvirt for the volume. A volume that was just created by the management server is not in this host's libvirt pool cache, so the lookup misses and getVolume() falls back to refreshing the whole pool. Refreshing an RBD pool opens and stats every image in it, so the cost grows with the number of volumes in the pool and is paid on every VM start. On a pool holding 950 images that is 11.7 seconds added to each start, against 0.005 seconds to list the image names. Look RBD volumes up directly through librbd instead. Creating, cloning, resizing, copying and deleting RBD volumes in this class already use librbd directly; only the lookup went through libvirt. Because this runs on every VM start, the connection is handled more carefully than at the existing call sites: - the rados connection is shut down in a finally, so a client is not left for the finalizer. KVMStoragePoolManager.getPhysicalDisk retries a missing volume 100 times, so leaking here would mean 101 live librados clients for one absent volume. - the image is opened read only, since it is only stat'ed, and so cannot take the exclusive lock. - the image is closed through a helper that does not throw, so a failed close cannot discard a successful lookup or hide the original error. - the cephx key is only set when the pool has a user. librados aborts the process if it is handed a null value. Signed-off-by: Brad House <bhouse@nexthop.ai>
1 parent 10037c8 commit a3a511b

1 file changed

Lines changed: 81 additions & 0 deletions

File tree

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

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -659,6 +659,19 @@ public KVMStoragePool getStoragePool(String uuid, boolean refreshInfo) {
659659
public KVMPhysicalDisk getPhysicalDisk(String volumeUuid, KVMStoragePool pool) {
660660
LibvirtStoragePool libvirtPool = (LibvirtStoragePool)pool;
661661

662+
/*
663+
* An RBD volume is looked up through librbd rather than through libvirt.
664+
* A volume that was created by another host is not in the libvirt pool
665+
* cache, so looking it up through libvirt misses and forces a refresh of
666+
* the whole pool. Refreshing an RBD pool stats every image in it, so that
667+
* cost grows with the number of volumes in the pool and is paid on every
668+
* VM start. Every other RBD operation in this class already uses librbd
669+
* directly.
670+
*/
671+
if (pool.getType() == StoragePoolType.RBD) {
672+
return getRbdPhysicalDisk(volumeUuid, libvirtPool);
673+
}
674+
662675
try {
663676
StorageVol vol = getVolume(libvirtPool.getPool(), volumeUuid);
664677
KVMPhysicalDisk disk;
@@ -696,6 +709,74 @@ public KVMPhysicalDisk getPhysicalDisk(String volumeUuid, KVMStoragePool pool) {
696709
}
697710
}
698711

712+
/**
713+
* Looks an RBD volume up directly through librbd.
714+
*
715+
* The size reported by librbd is used for both the size and the virtual size of
716+
* the disk, matching what this class already does after converting an image into
717+
* an RBD volume.
718+
*/
719+
private KVMPhysicalDisk getRbdPhysicalDisk(String volumeUuid, LibvirtStoragePool pool) {
720+
Rados r = new Rados(pool.getAuthUserName());
721+
try {
722+
r.confSet("mon_host", pool.getSourceHost() + ":" + pool.getSourcePort());
723+
/*
724+
* The secret is null when the pool has no cephx user, and librados
725+
* aborts the process rather than returning an error if it is handed a
726+
* null value here.
727+
*/
728+
if (pool.getAuthUserName() != null) {
729+
r.confSet("key", pool.getAuthSecret());
730+
} else {
731+
r.confSet("auth_client_required", "none");
732+
}
733+
r.confSet("client_mount_timeout", "30");
734+
r.connect();
735+
736+
IoCTX io = r.ioCtxCreate(pool.getSourceDir());
737+
try {
738+
Rbd rbd = new Rbd(io);
739+
// The image is only stat'ed, so it is opened read only and cannot take the exclusive lock.
740+
RbdImage image = rbd.openReadOnly(volumeUuid);
741+
try {
742+
RbdImageInfo rbdInfo = image.stat();
743+
KVMPhysicalDisk disk = new KVMPhysicalDisk(pool.getSourceDir() + "/" + volumeUuid, volumeUuid, pool);
744+
disk.setFormat(PhysicalDiskFormat.RAW);
745+
disk.setSize(rbdInfo.size);
746+
disk.setVirtualSize(rbdInfo.size);
747+
return disk;
748+
} finally {
749+
closeRbdImage(rbd, image, volumeUuid);
750+
}
751+
} finally {
752+
r.ioCtxDestroy(io);
753+
}
754+
} catch (RadosException e) {
755+
logger.error("A Ceph RADOS operation failed (" + e.getReturnValue() + "). The error was: " + e.getMessage()
756+
+ " - " + ErrorCode.getErrorMessage(e.getReturnValue()));
757+
throw new CloudRuntimeException(e.toString(), e);
758+
} catch (RbdException e) {
759+
logger.error("A Ceph RBD operation failed (" + e.getReturnValue() + "). The error was: " + e.getMessage()
760+
+ " - " + ErrorCode.getErrorMessage(e.getReturnValue()));
761+
throw new CloudRuntimeException(e.toString(), e);
762+
} finally {
763+
r.shutDown();
764+
}
765+
}
766+
767+
/**
768+
* Closes an RBD image without throwing, so that a failure to close cannot discard a
769+
* successful result or hide the exception that is already on its way out.
770+
*/
771+
private void closeRbdImage(Rbd rbd, RbdImage image, String volumeUuid) {
772+
try {
773+
rbd.close(image);
774+
} catch (RbdException e) {
775+
logger.warn("Failed to close RBD image " + volumeUuid + " (" + e.getReturnValue() + "): "
776+
+ e.getMessage() + " - " + ErrorCode.getErrorMessage(e.getReturnValue()));
777+
}
778+
}
779+
699780
/**
700781
* adjust refcount
701782
*/

0 commit comments

Comments
 (0)