From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id E940B1FF0E9 for ; Thu, 16 Jul 2026 10:25:27 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 95BFA2148A; Thu, 16 Jul 2026 10:25:21 +0200 (CEST) Message-ID: Date: Thu, 16 Jul 2026 10:25:15 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH pve-manager 2/3] partially fix #1989: ui: qemu: disk: add qcow2 cache size config To: Erik Fastermann , pve-devel@lists.proxmox.com References: <20260605153512.265703-1-e.fastermann@proxmox.com> <20260605153512.265703-3-e.fastermann@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20260605153512.265703-3-e.fastermann@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784190296333 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.311 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 7SXPD2VX5UOQXQJLEBEGQNI7UYYNXE3Y X-Message-ID-Hash: 7SXPD2VX5UOQXQJLEBEGQNI7UYYNXE3Y X-MailFrom: d.csapak@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 > --- > 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(