Re: [PATCH manager v3] fix #4503: ui: qemu: add detect zeroes disk option

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

Am 18.06.26 um 1:49 PM schrieb Lukas Sichert:
> When discard is enabled, qemu-server defaults to 'detect-zeroes=unmap'.
> This can have a significant performance impact for zero writes on some
> setups, while users might still want to keep guest discard/TRIM support
> enabled.
> 
> Allow configuring the option independently so 'discard' can stay enabled
> while zero detection is disabled. Persist the checkbox state explicitly,
> as the backend treats an absent value as default-enabled behavior,
> resulting in 'detect-zeroes=unmap' when 'discard' is enabled.
> 
> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=4503
> Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
> ---
> 
> Notes:
>     changes from v2 to v3 (thanks @Fabian):
>     - add a comment explaining why this property is handled differently from
>       the others
>     
>     changes from v1 to v2:
>     - added bug-numer to commit header
>     - appended bug-link to commit body
> 
>  www/manager6/qemu/HDEdit.js | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
> index 1bb2bfda..2708d452 100644
> --- a/www/manager6/qemu/HDEdit.js
> +++ b/www/manager6/qemu/HDEdit.js
> @@ -96,6 +96,9 @@ Ext.define('PVE.qemu.HDInputPanel', {
>          PVE.Utils.propertyStringSet(me.drive, values.readOnly, 'ro', 'on');
>          PVE.Utils.propertyStringSet(me.drive, values.cache, 'cache');
>          PVE.Utils.propertyStringSet(me.drive, values.aio, 'aio');
> +        // persist an explicit false value, since the backend defaults to enabled
> +        // when the option is omitted.
> +        me.drive.detect_zeroes = values.detect_zeroes ? 1 : 0;

But this will now persist both, an explicit 1 and and explicit 0,
needlessly cluttering the configuration in one case.

Note that we already have a property that is 1 by default rather than 0,
namely 'backup'. It still uses propertyStringSet(), but inverts the
value passed for 'source'. I think this is a hack though, as it only
works because of the explicit 'value' override. We could fix this up
with something like:

>         propertyStringSet: function (target, source, name, value, defaultIsTrue) {
>             if (defaultIsTrue ? !source : source) {

Alternatively, we could use something like:

>         propertyStringSet: function (target, isNotDefault, name, value) {
>             if (isNotDefault) {
>                 target[name] = value;
>             } else {
>                 delete target[name];
>             }
>         },

and always pass an explicit 'value'. The function as a whole becomes
simpler rather than more complex so maybe that's preferable.

And then use it for 'backup' and 'detect_zeroes'.

In fact, there is another one which is true by default, namely
'replicate', but there it's switched around by being 'noreplicate'.
Still, its call to propertyStringSet() might also considered a bit hacky
and only works because a custom 'value' is explicitly specified.

>  
>          ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'].forEach((name) => {
>              let burst_name = `${name}_max`;
> @@ -152,6 +155,7 @@ Ext.define('PVE.qemu.HDInputPanel', {
>          values.diskformat = drive.format || 'raw';
>          values.cache = drive.cache || '__default__';
>          values.discard = drive.discard === 'on';
> +        values.detect_zeroes = PVE.Parser.parseBoolean(drive.detect_zeroes, 1);
>          values.ssd = PVE.Parser.parseBoolean(drive.ssd);
>          values.iothread = PVE.Parser.parseBoolean(drive.iothread);
>          values.readOnly = PVE.Parser.parseBoolean(drive.ro);
> @@ -317,6 +321,12 @@ Ext.define('PVE.qemu.HDInputPanel', {
>                  reference: 'discard',
>                  name: 'discard',
>              },
> +            {
> +                xtype: 'proxmoxcheckbox',
> +                fieldLabel: gettext('Detect zeroes'),
> +                reference: 'detect_zeroes',
> +                name: 'detect_zeroes',
> +            },
>              {
>                  xtype: 'proxmoxcheckbox',
>                  name: 'iothread',