Am 29.05.26 um 2:58 PM schrieb Erik Fastermann: > Hi all, > > I'm interested in feedback for my patches fixing #1989 [0], which add multiple > options to configure the qcow2 L2/refcount cache. This enables huge > performance > gains in some cases. > > I tested this for the current and older QEMU machine versions and while a VM > was > running. The add disk (vm creation and otherwise), import disk and edit disk > dialogs were considered. > > I also have the following questions: > > - Currently I also check the file extension is qcow2, because in many cases > the > disk format is currently not available (especially on the frontend). Is this > good enough in general?
In the backend, you can use the checked_volume_format() method. In the front-end, I guess we don't have a better option than to check the filename right now, but at least for non-third-party plugins it should be good enough. > - The cache size input on the frontend is currently in bytes, because fine > control might be required in some cases, but this is a little bit ugly. > Is there a better way to do this? What cases do you have in mind? I feel like 1 MiB granularity should be fine-grained enough in practice. If we really do want to cleanly support different granularities, there is https://bugzilla.proxmox.com/show_bug.cgi?id=3760 up for taking (with an ex-colleague, Noel, having shared some semi-finished patches that would have to be picked up and finished). But even if we go for that, I'm not sure less MiB granularity is necessary for the front-end. > > - Should the ability to control cluster_size and refcount_bits be included in > this patch or should a separate bug be created? There was some discussion > about this on Bugzilla [0]. The bug is mixing different things, yeah. We'd lose the existing discussion related to the creation options if going for a new entry, so maybe the best here would be to use "partially fix" in your subject and update the bug title to what's still missing after the feature has landed. > - Should minimum / maximum values be defined for all options and in what way? I do think minimum values would be nice. Nobody complained about wanting to lower the setting as far as I know, people care about performance with this feature. The minimum should not be lower than QEMU's required minimum and not impact disk performance too much. And if somebody complains later, we can always go lower. And for the maximum, maybe a few GiB? I'm wondering if we should require VM.Config.Memory permission (in addition to VM.Config.Disk) for these new options though. > - Should we validate that the cache sizes are multiples of the cluster size / > cache entry size? If that's a requirement, sure. > - I'm using oneOf for the cache_size to support the 'based-on-disk' variant, > but > this does not play nice with the generated Rust API types (currently set the > outer type to string, but this probably does not work). Should this be > changed > or the Rust generation extended or can I use a custom enum on the Rust side? If we plan on adding more oneOf schemas, then yes, better Rust support would be good. I'm not familiar enough with the details here right now though. Question is, should we rather go for having an explicit value of 0 be treated special as 'based-on-disk' instead of using the oneOf schema? It's not very explicit though, so maybe not. Maybe have a secondary boolean option to avoid the overloading, something like proportional_cache_size=true|false, which conflicts with an explicitly set value? Or rather than boolean, a multiplier? And only expose that multiplier in the UI, since that should cover the majority of use cases already? What do you think? > - Should any extra documentation be added for this feature? Documentation is always good. A (short) "cache size" subsection in https://pve.proxmox.com/pve-docs/chapter-qm.html#qm_hard_disk would be nice. I'll continue with the individual patches next week.
