Re: [PATCH pve-manager 2/3] partially fix #1989: ui: qemu: disk: add qcow2 cache size config

[Dominik Csapak <d.csapak@proxmox.com>]

a few comments inline

On 6/5/26 5:34 PM, Erik Fastermann wrote:
> Add the option to control the size of the qcow2 cache in the vm disk
> create and edit dialogs. This can provide significant performance gains
> in some vm configurations.
> 
> For a detailed explanation see the QEMU docs [0]. Currently only the
> cache-size is configurable in the frontend, either directly or based on
> the size of the disk.
> 
> [0] https://gitlab.com/qemu-project/qemu/-/blob/master/docs/qcow2-cache.txt
> 
> Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
> ---
>   www/manager6/Parser.js      |  2 +-
>   www/manager6/qemu/HDEdit.js | 67 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/www/manager6/Parser.js b/www/manager6/Parser.js
> index 36df8e9d..f8d2bdad 100644
> --- a/www/manager6/Parser.js
> +++ b/www/manager6/Parser.js
> @@ -207,7 +207,7 @@ Ext.define('PVE.Parser', {
>                   if (!p || p.match(/^\s*$/)) {
>                       return undefined; // continue
>                   }
> -                let match = p.match(/^([a-z_]+)=(\S+)$/);
> +                let match = p.match(/^([0-9a-z-]+)=(\S+)$/);
>                   if (!match) {
>                       if (!p.match(/[=]/)) {
>                           res.file = p;
> diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
> index 1bb2bfda..3716a0b0 100644
> --- a/www/manager6/qemu/HDEdit.js
> +++ b/www/manager6/qemu/HDEdit.js
> @@ -18,6 +18,8 @@ Ext.define('PVE.qemu.HDInputPanel', {
>               isSCSI: false,
>               isVirtIO: false,
>               isSCSISingle: false,
> +            isQcow2: false,
> +            qcow2CacheSizeBasedOnDisk: false,
>           },
>       },
>   
> @@ -54,6 +56,22 @@ Ext.define('PVE.qemu.HDInputPanel', {
>                       vm.set('isSCSISingle', value === 'virtio-scsi-single');
>                   },
>               },
> +            'field[name=diskformat]': {
> +                change: function (f, value) {
> +                    this.getViewModel().set('isQcow2', value === 'qcow2');
> +                },
> +                afterrender: function (f) {
> +                    this.getViewModel().set('isQcow2', f.getValue() === 'qcow2');
> +                },

why is this necessary? the change handler should take care of that on 
the first change?

> +            },
> +            'field[name=qcow2CacheSizeBasedOnDisk]': {
> +                change: function (f, value) {
> +                    this.getViewModel().set('qcow2CacheSizeBasedOnDisk', value);
> +                },

IIRC this should not be necessary if you give the field a reference?

> +                afterrender: function (f) {
> +                    this.getViewModel().set('qcow2CacheSizeBasedOnDisk', f.getValue());
> +                },

same question here as for the isqcow2

> +            },
>           },
>   
>           init: function (view) {
> @@ -88,6 +106,20 @@ Ext.define('PVE.qemu.HDInputPanel', {
>               me.drive.format = values.diskformat;
>           }
>   
> +        if (me.drive?.format === 'qcow2' || me.drive?.file?.endsWith('.qcow2')) {
> +            if (values.qcow2CacheSize) {
> +                me.drive['qcow2-cache-size'] = values.qcow2CacheSize;
> +            } else {
> +                delete me.drive['qcow2-cache-size'];
> +            }
> +        }
> +
> +        PVE.Utils.propertyStringSet(
> +            me.drive,
> +            values.qcow2CacheSizeBasedOnDisk,
> +            'qcow2-cache-size-based-on-disk',
> +        );
> +
>           PVE.Utils.propertyStringSet(me.drive, !values.backup, 'backup', '0');
>           PVE.Utils.propertyStringSet(me.drive, values.noreplicate, 'replicate', 'no');
>           PVE.Utils.propertyStringSet(me.drive, values.discard, 'discard', 'on');
> @@ -156,6 +188,8 @@ Ext.define('PVE.qemu.HDInputPanel', {
>           values.iothread = PVE.Parser.parseBoolean(drive.iothread);
>           values.readOnly = PVE.Parser.parseBoolean(drive.ro);
>           values.aio = drive.aio || '__default__';
> +        values.qcow2CacheSizeBasedOnDisk = drive['qcow2-cache-size-based-on-disk'];
> +        values.qcow2CacheSize = drive['qcow2-cache-size'];
>   
>           values.mbps_rd = drive.mbps_rd;
>           values.mbps_wr = drive.mbps_wr;
> @@ -167,6 +201,12 @@ Ext.define('PVE.qemu.HDInputPanel', {
>           values.iops_wr_max = drive.iops_wr_max;
>   
>           me.setValues(values);
> +
> +        me.getViewModel().set(
> +            'isQcow2',
> +            values.diskformat === 'qcow2' || values.hdimage?.endsWith('.qcow2'),
> +        );
> +        this.getViewModel().set('qcow2CacheSizeBasedOnDisk', values.qcow2CacheSizeBasedOnDisk);
>       },
>   
>       setNodename: function (nodename) {
> @@ -355,6 +395,33 @@ Ext.define('PVE.qemu.HDInputPanel', {
>                       disabled: '{!isVirtIO && !isSCSI}',
>                   },
>               },
> +            {
> +                xtype: 'numberfield',
> +                name: 'qcow2CacheSize',
> +                minValue: 1,
> +                fieldLabel: gettext('qcow2 cache') + ' (MiB)',
> +                emptyText: gettext('Default cache size'),

as mentioned in the other patch, documenting the qemu default here would
be nice (if it does not change too often)

> +                bind: {
> +                    disabled: '{!isQcow2 || qcow2CacheSizeBasedOnDisk}',
> +                },
> +                listeners: {
> +                    disable: function (field) {
> +                        console.log(field);
> +                        field.setValue(null);
> +                    },
> +                },
> +            },
> +            {
> +                xtype: 'proxmoxcheckbox',
> +                name: 'qcow2CacheSizeBasedOnDisk',
> +                reference: 'qcow2CacheSizeBasedOnDisk',
> +                defaultValue: 0,
> +                fieldLabel: gettext('qcow2 cache based on disk'),
> +                clearOnDisable: true,
> +                bind: {
> +                    disabled: '{!isQcow2}',
> +                },
> +            },
>           );
>   
>           advancedColumn2.push(