Re: [RFC common/manager/qemu-server 0/3] fix #1989: qemu: add qcow2 cache options

[Fiona Ebner <f.ebner@proxmox.com>]

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.