kvm: add a disk.discard VM detail and drop the virtio-blk disk controller by calvix · Pull Request #14247 · apache/cloudstack

calvix · 2026-09-25T09:17:14Z

Description

Discard on KVM disks can only be enabled implicitly today: SCSI disks and Linstor RAW disks get discard='unmap', every other disk keeps libvirt's default discard='ignore'. A guest's TRIM on a virtio disk never reaches the storage, so on thin storage such as Ceph RBD deleted data keeps occupying the pool.

The virtio-blk disk controller (#10229, for #9299 "Virtio-blk support fstrim") was meant to cover this, but libvirt has no virtio-blk bus, virtio already is a virtio-blk device, so a VM with rootDiskController=virtio-blk can't start:

org.libvirt.LibvirtException: XML error: Invalid value for attribute 'bus' in element 'target': 'virtio-blk'.

This PR first made virtio-blk usable as a controller that turns on discard. As discussed in the review, it now takes a different approach. discard is a separate VM setting instead of being implied by a controller name, and the virtio-blk option, which never worked, is removed.

Changes

New KVM VM detail disk.discard, unmap or ignore. It is applied to every disk of the VM when it starts and when a disk is attached (the server passes it in the attach details, like io.policy). Without the detail the disks keep their current defaults, so nothing changes for existing VMs. ignore also turns off the SCSI / Linstor default.
unmap is not set on IDE disks when QEMU has the IDE discard bug (isQemuDiscardBugFree, QEMU < 7.0), same as the Linstor case.
disk.discard is returned by listDetailOptions for KVM, and dropped on deploy for other hypervisors like iothreads and io.policy.
virtio-blk is no longer offered as a root or data disk controller (listDetailOptions, template UI) and the VIRTIOBLK bus is gone. A VM or template that still has virtio-blk set gets a virtio disk, so it starts instead of failing.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
Build/CI
Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

How Has This Been Tested?

Tested on a KVM cluster with Ceph RBD primary storage (Ubuntu 24.04 hosts), with this change ported to 4.23:

VM start, checked in the live domain XML:
- no detail: virtio disks without discard, as before;
- disk.discard=unmap: discard='unmap' on root and data disks;
- SCSI controller: unmap as before; SCSI with disk.discard=ignore: no discard;
- an invalid value (on): no discard, and a warning in the agent log;
- rootDiskController=virtio-blk (previously failed to start with the error above): starts with a virtio disk, no discard; with disk.discard=unmap added it gets unmap; dataDiskController=virtio-blk gives virtio data disks.
Hot-attach of a data disk: the new disk gets the same discard setting as on start, in each of the cases above.
fstrim in the guest after writing and deleting 1 GiB: with unmap the RBD image shrank from 1.0 GiB to 48 MiB of used space (both a hot-attached disk and a disk present at start). Without the detail it stays at 1.0 GiB: the guest reports the trim, but QEMU drops it.
Live migration, detach / re-attach and stop / start keep the setting.
updateVirtualMachine adding disk.discard=unmap to a stopped VM that has rootDiskController=virtio-blk works, so existing virtio-blk VMs can be moved to the detail.
listDetailOptions returns disk.discard with unmap and ignore, and the controller lists no longer contain virtio-blk; the UI no longer offers it.

JoaoJandre

CLGTM, did not test it

codecov · 2026-10-02T16:59:17Z

Codecov Report

❌ Patch coverage is 60.00000% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.42%. Comparing base (8eeccdb) to head (3726c40).
⚠️ Report is 5 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
...n/java/com/cloud/storage/VolumeApiServiceImpl.java	0.00%	8 Missing ⚠️
...ain/java/com/cloud/api/query/QueryManagerImpl.java	0.00%	3 Missing ⚠️
.../src/main/java/com/cloud/vm/UserVmManagerImpl.java	0.00%	3 Missing ⚠️
...ervisor/kvm/resource/LibvirtComputingResource.java	90.00%	0 Missing and 2 partials ⚠️
...ud/hypervisor/kvm/storage/KVMStorageProcessor.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20   #14247      +/-   ##
============================================
+ Coverage     16.38%   16.42%   +0.03%     
- Complexity    13614    13661      +47     
============================================
  Files          5669     5669              
  Lines        501532   501661     +129     
  Branches      60922    60948      +26     
============================================
+ Hits          82153    82373     +220     
+ Misses       410172   410057     -115     
- Partials       9207     9231      +24

Flag	Coverage Δ
uitests	`4.16% <ø> (+<0.01%)`	⬆️
unittests	`17.28% <60.00%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Damans227 · 2026-10-02T17:29:18Z

                    if (disk.getDeviceType() == DeviceType.DISK) {
                        if (disk.getBusType() == DiskDef.DiskBus.SCSI) {
                            busT = DiskDef.DiskBus.SCSI;
+                        } else if (disk.getBusType() == DiskDef.DiskBus.VIRTIOBLK) {


can this ever match now? the running vm reports these disks back as plain virtio, so a disk added this way still loses discard

You are right, it can't.

The running VM reports these only as plain virtio. The method goes through getAttachDiskBusType(), which falls back to rootDiskController, so the disk still gets discard (tested).

SO i just removed the dead checks.

makes sense, the attach path following the root controller keeps discard on, so dropping the dead checks is fine

weizhouapache · 2026-10-12T20:34:40Z

I have some concerns about honoring "virtio-blk" as a separate disk controller option.

In libvirt, "virtio" is the disk bus, and the corresponding device is already a VirtIO block device ("virtio-blk"). This is similar to how a VirtIO NIC is represented by a "virtio" interface model, corresponding to a "virtio-net" device.

As a result, the existing CloudStack "virtio" disk controller option already maps to a VirtIO block device. With this change, "virtio" and "virtio-blk" would effectively expose the same device model, with the main difference being that "virtio-blk" enables "discard='unmap'".

I think this could be confusing for users, because the controller option appears to describe the device type, while it also implicitly controls discard behavior.

Could we instead keep "virtio" as the controller option and handle discard/unmap as a separate configuration? This would make the distinction between the disk device model and its discard capability clearer.

calvix · 2026-10-13T07:02:55Z

I have some concerns about honoring "virtio-blk" as a separate disk controller option.

In libvirt, "virtio" is the disk bus, and the corresponding device is already a VirtIO block device ("virtio-blk"). This is similar to how a VirtIO NIC is represented by a "virtio" interface model, corresponding to a "virtio-net" device.

As a result, the existing CloudStack "virtio" disk controller option already maps to a VirtIO block device. With this change, "virtio" and "virtio-blk" would effectively expose the same device model, with the main difference being that "virtio-blk" enables "discard='unmap'".

I think this could be confusing for users, because the controller option appears to describe the device type, while it also implicitly controls discard behavior.

Could we instead keep "virtio" as the controller option and handle discard/unmap as a separate configuration? This would make the distinction between the disk device model and its discard capability clearer.

Alright, I will take a look at how complicated it would be. I assume you would also want to get rid of virtio-blk option completely?

weizhouapache · 2026-10-13T07:26:08Z

Alright, I will take a look at how complicated it would be. I assume you would also want to get rid of virtio-blk option completely?

thanks.
yes, if virtio-blk is buggy and never worked, I think we can remove it.

…ller Discard on KVM disks could only be had implicitly: SCSI and Linstor disks get discard='unmap', every other disk keeps libvirt's default 'ignore', so a guest's TRIM never reaches thin storage such as Ceph RBD. The virtio-blk controller option was meant to provide it for virtio disks, but libvirt has no such bus ('virtio' already is a virtio-blk device), so a VM using it could not start. - New KVM VM detail disk.discard (unmap|ignore). It is applied to every disk when the VM starts and when a disk is attached; without it the disks keep their current defaults. 'unmap' is not set on IDE disks with a QEMU that has the IDE discard bug. - virtio-blk is no longer offered as a disk controller. A VM or template that still has it set gets a virtio disk, as before the option existed.

calvix · 2026-10-14T07:57:39Z

Alright, I will take a look at how complicated it would be. I assume you would also want to get rid of virtio-blk option completely?

thanks. yes, if virtio-blk is buggy and never worked, I think we can remove it.

I reworked the feature

weizhouapache · 2026-10-14T08:40:01Z

Alright, I will take a look at how complicated it would be. I assume you would also want to get rid of virtio-blk option completely?

thanks. yes, if virtio-blk is buggy and never worked, I think we can remove it.

I reworked the feature

thanks @calvix for the update. it looks good to me overall.

@JoaoJandre @Damans227 could you re-review this PR ?

Damans227 · 2026-10-14T12:43:59Z

thanks @calvix, the rework looks good to me

boring-cyborg Bot added the component:kvm label Sep 25, 2026

JoaoJandre approved these changes Oct 2, 2026

View reviewed changes

Damans227 reviewed Oct 2, 2026

View reviewed changes

weizhouapache reviewed Oct 5, 2026

View reviewed changes

Comment thread plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java Outdated

calvix mentioned this pull request Oct 13, 2026

kvm: add a disk.discard VM detail and drop the virtio-blk disk controller calvix/cloudstack#17

Closed

2 of 9 tasks

calvix force-pushed the fix/kvm-virtio-blk-disk-controller branch from 7233d17 to 3726c40 Compare October 14, 2026 07:55

calvix changed the title ~~kvm: make the virtio-blk disk controller usable~~ kvm: add a disk.discard VM detail and drop the virtio-blk disk controller Oct 14, 2026

boring-cyborg Bot added component:api component:storage component:UI labels Oct 14, 2026

Navigation Menu

Repository navigation

kvm: add a disk.discard VM detail and drop the virtio-blk disk controller - #14247

kvm: add a disk.discard VM detail and drop the virtio-blk disk controller#14247
calvix wants to merge 1 commit into
apache:4.20from
calvix:fix/kvm-virtio-blk-disk-controller

calvix commented Sep 25, 2026 •

edited

Loading

Uh oh!

JoaoJandre left a comment

Uh oh!

codecov Bot commented Oct 2, 2026 •

edited

Loading

Uh oh!

Damans227 Oct 2, 2026

Uh oh!

calvix Oct 5, 2026

Uh oh!

Damans227 Oct 6, 2026

Uh oh!

Uh oh!

weizhouapache commented Oct 12, 2026

Uh oh!

calvix commented Oct 13, 2026

Uh oh!

weizhouapache commented Oct 13, 2026

Uh oh!

calvix commented Oct 14, 2026

Uh oh!

weizhouapache commented Oct 14, 2026

Uh oh!

Damans227 commented Oct 14, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

calvix commented Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

How Has This Been Tested?

Uh oh!

JoaoJandre left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Damans227 Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

calvix Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

Damans227 Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

weizhouapache commented Oct 12, 2026

Uh oh!

calvix commented Oct 13, 2026

Uh oh!

weizhouapache commented Oct 13, 2026

Uh oh!

calvix commented Oct 14, 2026

Uh oh!

weizhouapache commented Oct 14, 2026

Uh oh!

Damans227 commented Oct 14, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

calvix commented Sep 25, 2026 •

edited

Loading

codecov Bot commented Oct 2, 2026 •

edited

Loading