Repository navigation
Conversation
Guest TRIM is silently dropped for every qcow2 volume on the virtio bus. The guest issues the discard and QEMU accepts it at the device, but the qcow2 node is left at discard=ignore, so no hole is ever punched and the image only ever grows. discard=unmap was set only for SCSI, for the separate virtio-blk bus value, and for Linstor pools. The plain virtio bus matched none of them, and it is the default for file-backed primary storage. The exclusion dates from CLOUDSTACK-8239, when virtio-blk could not do discard; QEMU gained that support in 4.0 and the condition was never revisited. The two call sites had also drifted apart, the VM start path accepting SCSI and virtio-blk while the volume attach path accepted SCSI alone, so the rule now lives in diskBusSupportsDiscard and both sites use it. qemuDriver already defaults to true, so adding virtio changes only the discard attribute. Measured on an NFS primary storage pool: fstrim reported releasing 19.5 GiB on a 20 GiB volume holding 11 MB of data, while the host file stayed at exactly 19725232 allocated blocks. After the change the same volume dropped to 334720 blocks. Across one zone this recovered 317 GiB on a pool that was 97% full.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The discard check uses the root controller rather than the disk’s final bus, exposing IDE data disks to the QEMU < 7.0 discard bug, while hot-attached NFS virtio volumes still omit discard.
2 open issues
What changed in this PR
This PR extends KVM discard handling to plain virtio disks so guest TRIM can reclaim space on file-backed storage.
Changes:
- Introduces a shared discard-support predicate for SCSI, VIRTIO, and VIRTIOBLK.
- Uses the predicate in
LibvirtComputingResourcedisk creation and attach/detach logic. - Adds NFS discard and bus-matrix tests, reusing setup from the existing Linstor test.
| File | Description |
|---|---|
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java |
Extracts shared VM setup and adds discard regression tests. |
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java |
Centralizes the bus predicate and includes VIRTIO in discard handling. |
Review effort: Balanced
| disk.setPhysicalBlockIOSize(pool.getSupportedPhysicalBlockSize()); | ||
|
|
||
| if (diskBusType == DiskDef.DiskBus.SCSI || diskBusType == DiskDef.DiskBus.VIRTIOBLK) { | ||
| if (diskBusSupportsDiscard(diskBusType)) { |
|
|
||
| diskdef = new DiskDef(); | ||
| if (busT == DiskDef.DiskBus.SCSI) { | ||
| if (diskBusSupportsDiscard(busT)) { |
Member
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Guest TRIM is silently dropped for every qcow2 volume on the
virtiobus. The guest issues the discard and QEMU accepts it at the device, but the qcow2 blockdev is left atdiscard=ignore, so no hole is ever punched and the image only ever grows.discard=unmapwas set only forSCSI, for the separateVIRTIOBLKbus value, and for Linstor pools. The plainVIRTIObus matched none of them, and it is the default for file-backed primary storage.The exclusion looks historical rather than deliberate. It dates from CLOUDSTACK-8239, when virtio-blk genuinely could not do discard. QEMU gained that support in 4.0 and the condition was never revisited.
The two call sites had also drifted apart:
SCSI || VIRTIOBLKSCSIonlyThe attach path is the stricter of the two and governs data volumes attached to running VMs, which is where this hurts most in practice.
Why this went unnoticed
Linstor pools already set
discard=unmapthrough their own branch, added with Linstor support and never generalised. Verified on a live host: a virtio disk on a Linstor pool carriesdiscard='unmap'today, while an identical guest on the same bus backed by NFS does not.That last row is the default configuration for Linux guests on file-backed primary storage, since
getGuestDiskModelreturnsVIRTIOfor Debian, Ubuntu, CentOS, RHEL, Fedora, Rocky, Alma, Oracle and FreeBSD. Anyone on Linstor, or anyone who had setrootDiskController=scsifor unrelated reasons, was silently immune.So this is not new behaviour: it extends to file-backed pools what Linstor users have had all along.
Measured behaviour
On an NFS primary storage pool, a 20 GiB volume holding 11 MB of live data:
fstrimfstrimreported 19.5 GiB trimmedfstrimEvery other link in the chain was verified working beforehand, so the qcow2 layer was the only thing dropping the request:
fstrim.timerenabled and running weekly in the guest, last exit successvirtio-blk-pciadvertisesdiscard(default on); guests report a non-zerodiscard_max_bytesRolled out across one zone, this recovered 317 GiB on a pool that had reached 97% full, and 69 GiB on a second pool. Volumes holding real data gave back little, as expected; volumes holding stale blocks gave back nearly everything. One Elasticsearch data volume went from 70.0 GiB to 561 MB.
Note the change only takes effect as disks are next defined, on VM start or volume attach, so running domains keep their existing XML until they cycle.
Implementation
The rule now lives in
diskBusSupportsDiscardand both call sites use it, which also stops the two conditions drifting apart again.qemuDriveralready defaults totrue, so addingVIRTIOchanges only thediscardattribute and nothing else in the generated domain XML.Types of changes
How Has This Been Tested?
testDiskBusSupportsDiscardpins the full bus matrix, including thatIDEandSATAstay excluded.createNfsVbdDiscarddrivescreateVbdend to end on an NFS qcow2 volume and assertsUNMAPfor virtio andIGNOREfor IDE. The existingcreateLinstorVdbonly ever covered the Linstor path, which is why virtio worked there and nowhere else.createVbdtests is extracted intoprepareVmForCreateVbd, so that block exists once rather than twice.KVM plugin suite on this branch: 1025 tests, 0 failures, 0 errors, 3 skipped.
Also verified in production use on KVM with NFS primary storage across 7 instances, as described above.