Kukunin opened a new pull request, #14618: URL: https://github.com/apache/cloudstack/pull/14618
### Description 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 blockdev 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 `VIRTIOBLK` 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 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: | Call site | Before | |---|---| | VM start | `SCSI \|\| VIRTIOBLK` | | Volume attach | `SCSI` only | The 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=unmap` through their own branch, added with Linstor support and never generalised. Verified on a live host: a virtio disk on a Linstor pool carries `discard='unmap'` today, while an identical guest on the same bus backed by NFS does not. | Storage | Bus | Discard before this change | |---|---|---| | Linstor | virtio | yes, via its own branch | | Linstor | SCSI | yes | | NFS / SharedMountPoint / Filesystem / Gluster | SCSI | yes | | NFS / SharedMountPoint / Filesystem / Gluster | **virtio** | **no** | That last row is the default configuration for Linux guests on file-backed primary storage, since `getGuestDiskModel` returns `VIRTIO` for Debian, Ubuntu, CentOS, RHEL, Fedora, Rocky, Alma, Oracle and FreeBSD. Anyone on Linstor, or anyone who had set `rootDiskController=scsi` for 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: | | Allocated blocks | du | |---|---|---| | Before `fstrim` | 19,725,232 | 9.5 GiB | | After `fstrim` reported 19.5 GiB trimmed | 19,725,232 | 9.5 GiB | | After this change, same `fstrim` | 334,720 | 164 MB | Every other link in the chain was verified working beforehand, so the qcow2 layer was the only thing dropping the request: - `fstrim.timer` enabled and running weekly in the guest, last exit success - QEMU `virtio-blk-pci` advertises `discard` (default on); guests report a non-zero `discard_max_bytes` - Hole punching works on the NFS export: a 64 MB file dropped from 131072 to 65536 allocated blocks after punching a 32 MB hole Rolled 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 `diskBusSupportsDiscard` and both call sites use it, which also stops the two conditions drifting apart again. `qemuDriver` already defaults to `true`, so adding `VIRTIO` changes only the `discard` attribute and nothing else in the generated domain XML. ### Types of changes - [x] Bug fix (non-breaking change which fixes an issue) ### How Has This Been Tested? - `testDiskBusSupportsDiscard` pins the full bus matrix, including that `IDE` and `SATA` stay excluded. - `createNfsVbdDiscard` drives `createVbd` end to end on an NFS qcow2 volume and asserts `UNMAP` for virtio and `IGNORE` for IDE. The existing `createLinstorVdb` only ever covered the Linstor path, which is why virtio worked there and nowhere else. - The shared setup behind both `createVbd` tests is extracted into `prepareVmForCreateVbd`, 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. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
