From 35a3060feba1c0713c96195db9300b8fa174bfaa Mon Sep 17 00:00:00 2001 From: calvix <7136358+calvix@users.noreply.github.com> Date: Thu, 24 Sep 2026 17:42:46 +0200 Subject: [PATCH 1/2] kvm: make the virtio-blk disk controller usable The VM detail rootDiskController=virtio-blk has never been able to start a VM. DiskBus renders its own value into the libvirt disk XML, and 'virtio-blk' is not one of libvirt's target buses (ide, scsi, virtio, xen, usb, sata, sd, fdc, uml), so libvirt rejects the domain and every host fails the start with: XML error: Invalid value for attribute 'bus' in element 'target': 'virtio-blk' The point of the controller was a virtio disk with discard='unmap', so that combination has been unreachable: only SCSI disks get discard on KVM today, and plain virtio disks keep libvirt's default discard='ignore', which drops a guest's TRIM before it reaches the storage. Render the libvirt bus name rather than the enum value, leaving toString() and fromValue() alone so the detail keeps parsing, and label virtio-blk disks vd* as virtio disks are labelled. Data disks now follow a virtio-blk root as they already follow a SCSI one, so a VM that opts in gets discard on all its disks, and the second attach path gives a hot-plugged disk the same discard the volume-attach path does. A disk being attached cannot recover the controller from the running domain XML, where a virtio-blk disk is indistinguishable from a virtio one, so a data disk with no controller of its own follows the root detail; without that a hot-plugged disk silently loses discard until the next stop/start. Verified on a KVM host with Ceph RBD primary storage: the VM starts, every disk is bus='virtio' with discard='unmap', guest device names are stable across reboot and stop/start, and after an fstrim in the guest 700 MiB of the 728 MiB written was returned to the pool. --- .../resource/LibvirtComputingResource.java | 6 +- .../hypervisor/kvm/resource/LibvirtVMDef.java | 15 ++++- .../kvm/storage/KVMStorageProcessor.java | 9 +++ .../kvm/resource/LibvirtVMDefTest.java | 60 +++++++++++++++++++ .../kvm/storage/KVMStorageProcessorTest.java | 36 +++++++++++ 5 files changed, 122 insertions(+), 4 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index 7c3f826be171..f3eb092bb036 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -3833,7 +3833,7 @@ public int compare(final DiskTO arg0, final DiskTO arg1) { DiskDef.DiskBus diskBusTypeData = getDataDiskModelFromVMDetail(vmSpec); if (diskBusTypeData == null) { - diskBusTypeData = (diskBusType == DiskDef.DiskBus.SCSI) ? diskBusType : DiskDef.DiskBus.VIRTIO; + diskBusTypeData = (diskBusType == DiskDef.DiskBus.SCSI || diskBusType == DiskDef.DiskBus.VIRTIOBLK) ? diskBusType : DiskDef.DiskBus.VIRTIO; } final DiskDef disk = new DiskDef(); @@ -4256,13 +4256,15 @@ public synchronized String attachOrDetachDisk(final Connect conn, if (disk.getDeviceType() == DeviceType.DISK) { if (disk.getBusType() == DiskDef.DiskBus.SCSI) { busT = DiskDef.DiskBus.SCSI; + } else if (disk.getBusType() == DiskDef.DiskBus.VIRTIOBLK) { + busT = DiskDef.DiskBus.VIRTIOBLK; } break; } } diskdef = new DiskDef(); - if (busT == DiskDef.DiskBus.SCSI) { + if (busT == DiskDef.DiskBus.SCSI || busT == DiskDef.DiskBus.VIRTIOBLK) { diskdef.setQemuDriver(true); diskdef.setDiscard(DiscardType.UNMAP); } diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java index 439e4f663416..6f98c06accc8 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java @@ -871,6 +871,17 @@ public enum DiskBus { _bus = bus; } + /** + * The name libvirt knows this bus by. It is the enum's own value everywhere except + * VIRTIOBLK: 'virtio-blk' is a CloudStack disk controller name, not one of libvirt's + * target buses (ide, scsi, virtio, xen, usb, sata, sd, fdc, uml), and a virtio-blk + * disk is a virtio one. Rendering the enum value instead makes libvirt reject the + * domain XML, so the VM cannot start. + */ + public String libvirtBus() { + return this == VIRTIOBLK ? VIRTIO._bus : _bus; + } + public static DiskBus fromValue(String bus) { for (DiskBus b : DiskBus.values()) { if (b.toString().equalsIgnoreCase(bus)) { @@ -1031,7 +1042,7 @@ private String getDevLabel(int devId, DiskBus bus, boolean forIso) { if (bus == DiskBus.SCSI) { return "sd" + getDevLabelSuffix(devId); - } else if (bus == DiskBus.VIRTIO) { + } else if (bus == DiskBus.VIRTIO || bus == DiskBus.VIRTIOBLK) { return "vd" + getDevLabelSuffix(devId); } else if (bus == DiskBus.SATA){ return "sd" + getDevLabelSuffix(devId); @@ -1429,7 +1440,7 @@ public String toString() { } diskBuilder.append("\n"); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java index 0cb32da318ea..704ba00df874 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java @@ -1788,6 +1788,15 @@ protected DiskDef.DiskBus getAttachDiskBusType(int deviceId, List disks diskController, controllerKey, deviceId); return busType; } + if (deviceId != 0 && DiskDef.DiskBus.VIRTIOBLK == DiskDef.DiskBus.fromValue( + MapUtils.getString(controllerInfo, VmDetailConstants.ROOT_DISK_CONTROLLER))) { + // A data disk with no controller of its own follows the root, as it already does when the + // VM starts. The scan below cannot help here: a virtio-blk disk is rendered with libvirt's + // 'virtio' bus, so it is indistinguishable from a plain virtio disk in the domain XML, and + // the disk would silently lose discard until the next stop/start. + logger.debug("Attaching disk (deviceId={}) with the root controller virtio-blk", deviceId); + return DiskDef.DiskBus.VIRTIOBLK; + } for (final DiskDef disk : disks) { if (disk.getDeviceType() != DeviceType.DISK) { continue; diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java index 56ad267eac7e..e0a7530c66fe 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java @@ -19,6 +19,9 @@ package com.cloud.hypervisor.kvm.resource; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import java.io.File; import java.util.Arrays; import java.util.List; @@ -597,4 +600,61 @@ public void testTpmModel() { "\n" + "\n", tpmDef.toString()); } + + @Test + public void testDiskDefVirtioBlkUsesVirtioBusAndVdLabel() { + DiskDef disk = new DiskDef(); + disk.defFileBasedDisk("/var/lib/libvirt/images/disk.qcow2", 1, DiskDef.DiskBus.VIRTIOBLK, DiskDef.DiskFmtType.QCOW2); + + String xmlDef = disk.toString(); + // 'virtio-blk' is a CloudStack controller name; libvirt only accepts the bus 'virtio' and + // rejects the whole domain XML otherwise, which is what stopped these VMs from starting. + assertTrue(xmlDef, xmlDef.contains("")); + assertFalse(xmlDef, xmlDef.contains("virtio-blk")); + assertEquals(DiskDef.DiskBus.VIRTIOBLK, disk.getBusType()); + } + + @Test + public void testDiskDefVirtioBlkLabelsMatchVirtio() { + for (int devId = 0; devId < 4; devId++) { + DiskDef virtio = new DiskDef(); + virtio.defFileBasedDisk("/var/lib/libvirt/images/disk.qcow2", devId, DiskDef.DiskBus.VIRTIO, DiskDef.DiskFmtType.QCOW2); + DiskDef virtioBlk = new DiskDef(); + virtioBlk.defFileBasedDisk("/var/lib/libvirt/images/disk.qcow2", devId, DiskDef.DiskBus.VIRTIOBLK, DiskDef.DiskFmtType.QCOW2); + assertEquals(virtio.getDiskLabel(), virtioBlk.getDiskLabel()); + } + } + + @Test + public void testDiskDefVirtioBlkWithDiscardUnmap() { + DiskDef disk = new DiskDef(); + disk.defFileBasedDisk("/var/lib/libvirt/images/disk.qcow2", 0, DiskDef.DiskBus.VIRTIOBLK, DiskDef.DiskFmtType.QCOW2); + disk.setQemuDriver(true); + disk.setDiscard(DiskDef.DiscardType.UNMAP); + + String xmlDef = disk.toString(); + // the point of the virtio-blk controller: a virtio disk that passes the guest's discard through + assertTrue(xmlDef, xmlDef.contains("discard='unmap'")); + assertTrue(xmlDef, xmlDef.contains("bus='virtio'")); + assertFalse(xmlDef, xmlDef.contains("virtio-blk")); + } + + @Test + public void testDiskDefVirtioIsUnchanged() { + DiskDef disk = new DiskDef(); + disk.defFileBasedDisk("/var/lib/libvirt/images/disk.qcow2", 0, DiskDef.DiskBus.VIRTIO, DiskDef.DiskFmtType.QCOW2); + + String xmlDef = disk.toString(); + assertTrue(xmlDef, xmlDef.contains("")); + assertFalse(xmlDef, xmlDef.contains("discard=")); + } + + @Test + public void testDiskDefScsiIsUnchanged() { + DiskDef disk = new DiskDef(); + disk.defFileBasedDisk("/var/lib/libvirt/images/disk.qcow2", 0, DiskDef.DiskBus.SCSI, DiskDef.DiskFmtType.QCOW2); + + String xmlDef = disk.toString(); + assertTrue(xmlDef, xmlDef.contains("")); + } } diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java index a85fc31224e0..e98943d36279 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java @@ -23,6 +23,7 @@ import com.ceph.rbd.Rbd; import com.ceph.rbd.RbdException; import com.ceph.rbd.RbdImage; +import com.cloud.vm.VmDetailConstants; import com.cloud.exception.InternalErrorException; import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; import com.cloud.hypervisor.kvm.resource.LibvirtDomainXMLParser; @@ -765,4 +766,39 @@ public void ensureLibvirtSupportsEncryptedRbdHotplugIgnoresNonRbd() { // version is not even consulted for them. storageProcessor.ensureLibvirtSupportsEncryptedRbdHotplug(StoragePoolType.NetworkFilesystem); } + + private LibvirtVMDef.DiskDef diskWithBus(LibvirtVMDef.DiskDef.DiskBus bus) { + LibvirtVMDef.DiskDef disk = new LibvirtVMDef.DiskDef(); + disk.defFileBasedDisk("/var/lib/libvirt/images/disk.qcow2", 0, bus, LibvirtVMDef.DiskDef.DiskFmtType.QCOW2); + return disk; + } + + @Test + public void getAttachDiskBusTypeUsesTheDataDiskControllerWhenItIsSet() { + Map controllerInfo = Map.of(VmDetailConstants.DATA_DISK_CONTROLLER, "virtio-blk"); + Assert.assertEquals(LibvirtVMDef.DiskDef.DiskBus.VIRTIOBLK, + storageProcessor.getAttachDiskBusType(1, List.of(diskWithBus(LibvirtVMDef.DiskDef.DiskBus.VIRTIO)), controllerInfo)); + } + + @Test + public void getAttachDiskBusTypeFollowsAVirtioBlkRootForDataDisks() { + // a virtio-blk disk renders with libvirt's 'virtio' bus, so scanning the domain XML cannot + // tell the two apart; without this the hot-plugged disk loses discard until the next start + Map controllerInfo = Map.of(VmDetailConstants.ROOT_DISK_CONTROLLER, "virtio-blk"); + Assert.assertEquals(LibvirtVMDef.DiskDef.DiskBus.VIRTIOBLK, + storageProcessor.getAttachDiskBusType(1, List.of(diskWithBus(LibvirtVMDef.DiskDef.DiskBus.VIRTIO)), controllerInfo)); + } + + @Test + public void getAttachDiskBusTypeKeepsVirtioForAVirtioRoot() { + Map controllerInfo = Map.of(VmDetailConstants.ROOT_DISK_CONTROLLER, "virtio"); + Assert.assertEquals(LibvirtVMDef.DiskDef.DiskBus.VIRTIO, + storageProcessor.getAttachDiskBusType(1, List.of(diskWithBus(LibvirtVMDef.DiskDef.DiskBus.VIRTIO)), controllerInfo)); + } + + @Test + public void getAttachDiskBusTypeStillDetectsScsiFromTheRunningDisks() { + Assert.assertEquals(LibvirtVMDef.DiskDef.DiskBus.SCSI, + storageProcessor.getAttachDiskBusType(1, List.of(diskWithBus(LibvirtVMDef.DiskDef.DiskBus.SCSI)), Map.of())); + } } From c801f496ec7d5bdbd2b84f61241165585e9af1d9 Mon Sep 17 00:00:00 2001 From: calvix <7136358+calvix@users.noreply.github.com> Date: Thu, 24 Sep 2026 18:36:27 +0200 Subject: [PATCH 2/2] kvm: bind virtio-blk disks to an iothread, and let running SCSI disks win on attach A disk with iothreads enabled got its iothread= binding only when its bus was VIRTIO, although a virtio-blk disk is rendered with the same libvirt bus. A virtio-blk VM with the iothreads detail therefore ran every disk with io='threads' but no iothread, and data disks following a virtio-blk root now lost it as well. Treat VIRTIOBLK like VIRTIO there. When a data disk is attached without a controller of its own, the virtio-blk root fallback ran before the scan of the running domain. The root detail can be changed on a running VM, so a VM started on SCSI whose detail was later set to virtio-blk would get a virtio disk hot-plugged next to its sd* ones. Scan the running disks first and use the fallback only when no SCSI disk is there. --- .../hypervisor/kvm/resource/LibvirtVMDef.java | 2 +- .../kvm/storage/KVMStorageProcessor.java | 20 ++++++++++--------- .../kvm/resource/LibvirtVMDefTest.java | 10 ++++++++++ .../kvm/storage/KVMStorageProcessorTest.java | 9 +++++++++ 4 files changed, 31 insertions(+), 10 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java index 6f98c06accc8..3f221d749bbf 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java @@ -1397,7 +1397,7 @@ public String toString() { diskBuilder.append(String.format("io='%s' ", ioDriver)); } - if (isIothreadsEnabled && _bus == DiskBus.VIRTIO) { + if (isIothreadsEnabled && (_bus == DiskBus.VIRTIO || _bus == DiskBus.VIRTIOBLK)) { diskBuilder.append(String.format("iothread='%s' ", NUMBER_OF_IOTHREADS)); } diskBuilder.append("/>\n"); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java index 704ba00df874..0d93471219df 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java @@ -1788,15 +1788,6 @@ protected DiskDef.DiskBus getAttachDiskBusType(int deviceId, List disks diskController, controllerKey, deviceId); return busType; } - if (deviceId != 0 && DiskDef.DiskBus.VIRTIOBLK == DiskDef.DiskBus.fromValue( - MapUtils.getString(controllerInfo, VmDetailConstants.ROOT_DISK_CONTROLLER))) { - // A data disk with no controller of its own follows the root, as it already does when the - // VM starts. The scan below cannot help here: a virtio-blk disk is rendered with libvirt's - // 'virtio' bus, so it is indistinguishable from a plain virtio disk in the domain XML, and - // the disk would silently lose discard until the next stop/start. - logger.debug("Attaching disk (deviceId={}) with the root controller virtio-blk", deviceId); - return DiskDef.DiskBus.VIRTIOBLK; - } for (final DiskDef disk : disks) { if (disk.getDeviceType() != DeviceType.DISK) { continue; @@ -1807,6 +1798,17 @@ protected DiskDef.DiskBus getAttachDiskBusType(int deviceId, List disks return DiskDef.DiskBus.VIRTIOBLK; } } + if (deviceId != 0 && DiskDef.DiskBus.VIRTIOBLK == DiskDef.DiskBus.fromValue( + MapUtils.getString(controllerInfo, VmDetailConstants.ROOT_DISK_CONTROLLER))) { + // A data disk with no controller of its own follows the root, as it already does when the + // VM starts. The scan above cannot see this case: a virtio-blk disk is rendered with + // libvirt's 'virtio' bus, so it is indistinguishable from a plain virtio disk in the domain + // XML, and the disk would silently lose discard until the next stop/start. The scan still + // wins when the running VM is on SCSI, so a detail changed after the VM started cannot + // put a virtio disk next to its SCSI ones. + logger.debug("Attaching disk (deviceId={}) with the root controller virtio-blk", deviceId); + return DiskDef.DiskBus.VIRTIOBLK; + } return DiskDef.DiskBus.VIRTIO; } diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java index e0a7530c66fe..9b52874fddb1 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java @@ -657,4 +657,14 @@ public void testDiskDefScsiIsUnchanged() { String xmlDef = disk.toString(); assertTrue(xmlDef, xmlDef.contains("")); } + + @Test + public void testDiskDefVirtioBlkGetsAnIothreadLikeVirtio() { + DiskDef disk = new DiskDef(); + disk.defFileBasedDisk("/var/lib/libvirt/images/disk.qcow2", 0, DiskDef.DiskBus.VIRTIOBLK, DiskDef.DiskFmtType.QCOW2); + disk.isIothreadsEnabled(true); + + String xmlDef = disk.toString(); + assertTrue(xmlDef, xmlDef.contains("iothread=")); + } } diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java index e98943d36279..e3aa13193789 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java @@ -801,4 +801,13 @@ public void getAttachDiskBusTypeStillDetectsScsiFromTheRunningDisks() { Assert.assertEquals(LibvirtVMDef.DiskDef.DiskBus.SCSI, storageProcessor.getAttachDiskBusType(1, List.of(diskWithBus(LibvirtVMDef.DiskDef.DiskBus.SCSI)), Map.of())); } + + @Test + public void getAttachDiskBusTypePrefersTheRunningScsiDisksOverAVirtioBlkRootDetail() { + // the detail can be changed on a running VM; the disks it actually runs on win, so a SCSI VM + // does not get a virtio disk hot-plugged next to its sd* ones + Map controllerInfo = Map.of(VmDetailConstants.ROOT_DISK_CONTROLLER, "virtio-blk"); + Assert.assertEquals(LibvirtVMDef.DiskDef.DiskBus.SCSI, + storageProcessor.getAttachDiskBusType(1, List.of(diskWithBus(LibvirtVMDef.DiskDef.DiskBus.SCSI)), controllerInfo)); + } }