[pve-devel] [PATCH v3 manager 1/2] HDEdit: check iothread by default and move it from advanced section

[pve-devel] [PATCH v3 manager 1/2] HDEdit: check iothread by default and move it from advanced section

[Matthias Heiserer]

Existing disks are not changed by this.
Especially in benchmarks, iothreads significantly improve IO performance.


Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---

Changes from v2:
* also check iothread when adding a disk to an existing VM and 
 scsi single
* use bind instead of hardcoded true

 www/manager6/qemu/HDEdit.js | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
index c643ee73..adee2591 100644
--- a/www/manager6/qemu/HDEdit.js
+++ b/www/manager6/qemu/HDEdit.js
@@ -14,6 +14,7 @@ Ext.define('PVE.qemu.HDInputPanel', {
 	data: {
 	    isScsi: false,
 	    isVirtIO: false,
+	    isSCSISingle: false,
 	},
     },
 
@@ -53,6 +54,12 @@ Ext.define('PVE.qemu.HDInputPanel', {
 		    this.lookupReference('scsiController').setValue(vmScsiType);
 		},
 	    },
+	    'field[name=scsiController]': {
+		change: function(f, value) {
+		    let vm = this.getViewModel();
+		    vm.set('isSCSISingle', value === 'virtio-scsi-single');
+		},
+	    },
 	},
 
 	init: function(view) {
@@ -195,6 +202,7 @@ Ext.define('PVE.qemu.HDInputPanel', {
 	    me.scsiController = Ext.create('Ext.form.field.Display', {
 		fieldLabel: gettext('SCSI Controller'),
 		reference: 'scsiController',
+		name: 'scsiController',
 		bind: me.insideWizard ? {
 		    value: '{current.scsihw}',
 		    visible: '{isSCSI}',
@@ -251,6 +259,16 @@ Ext.define('PVE.qemu.HDInputPanel', {
 		reference: 'discard',
 		name: 'discard',
 	    },
+	    {
+		xtype: 'proxmoxcheckbox',
+		name: 'iothread',
+		fieldLabel: 'IO thread',
+		clearOnDisable: true,
+		bind: {
+		    disabled: '{!isVirtIO && !isSCSI}',
+		    value: '{isSCSISingle}',
+		},
+	    },
 	);
 
 	advancedColumn1.push(
@@ -263,15 +281,6 @@ Ext.define('PVE.qemu.HDInputPanel', {
 		    disabled: '{isVirtIO}',
 		},
 	    },
-	    {
-		xtype: 'proxmoxcheckbox',
-		name: 'iothread',
-		fieldLabel: 'IO thread',
-		clearOnDisable: true,
-		bind: {
-		    disabled: '{!isVirtIO && !isSCSI}',
-		},
-	    },
 	    {
 		xtype: 'proxmoxcheckbox',
 		name: 'readOnly', // `ro` in the config, we map in get/set values
-- 
2.30.2

[pve-devel] [PATCH v3 qemu-server 1/2] bump pve-common

[Matthias Heiserer]

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
Required so log_warn can be used by the other patch

Changes from v1/v2:
new patch

 debian/control | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/debian/control b/debian/control
index 7de9e84..2fdc529 100644
--- a/debian/control
+++ b/debian/control
@@ -7,7 +7,7 @@ Build-Depends: debhelper (>= 12~),
                libio-multiplex-perl,
                libjson-c-dev,
                libpve-cluster-perl,
-               libpve-common-perl (>= 7.1-3),
+               libpve-common-perl (>= 7.1-4),
                libpve-guest-common-perl (>= 4.1-1),
                libpve-storage-perl (>= 6.1-7),
                libtest-mockmodule-perl,
-- 
2.30.2

[pve-devel] [PATCH v3 manager 2/2] OS defaults: use SCSI single as default controller

[Matthias Heiserer]

Existing installs are not changed by this.

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---

No changes from v1/v2

 www/manager6/qemu/OSDefaults.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/www/manager6/qemu/OSDefaults.js b/www/manager6/qemu/OSDefaults.js
index eed9eebc..5e588a58 100644
--- a/www/manager6/qemu/OSDefaults.js
+++ b/www/manager6/qemu/OSDefaults.js
@@ -42,7 +42,7 @@ Ext.define('PVE.qemu.OSDefaults', {
 		    scsi: 2,
 		    virtio: 1,
 	    },
-	    scsihw: 'virtio-scsi-pci',
+	    scsihw: 'virtio-scsi-single',
 	};
 
        // virtio-net is in kernel since 2.6.25
-- 
2.30.2

[pve-devel] [PATCH v3 qemu-server 2/2] Warn in GUI for unlikely iothread config - fixes #3890

[Matthias Heiserer]

Previously, only a plaintext line in the task log showed something was off.
Now, the GUI will show it as a warning.


Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---

Changes from v2:
* locally import PVE::RestEnv log_warn

No changes from v1

 PVE/QemuServer.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index e0fc36e..408133e 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -41,6 +41,7 @@ use PVE::Storage;
 use PVE::SysFSTools;
 use PVE::Systemd;
 use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_foreach get_host_arch $IPV6RE);
+use PVE::RESTEnvironment qw(log_warn);
 
 use PVE::QMPClient;
 use PVE::QemuConfig;
@@ -3961,7 +3962,9 @@ sub config_to_command {
 		$iothread .= ",iothread=iothread-$controller_prefix$controller";
 		push @$cmd, '-object', "iothread,id=iothread-$controller_prefix$controller";
 	    } elsif ($drive->{iothread}) {
-		warn "iothread is only valid with virtio disk or virtio-scsi-single controller, ignoring\n";
+		log_warn(
+		    "iothread is only valid with virtio disk or virtio-scsi-single controller, ignoring\n"
+		);
 	    }
 
 	    my $queues = '';
-- 
2.30.2

Re: [pve-devel] [PATCH v3 manager 1/2] HDEdit: check iothread by default and move it from advanced section

[Fabian Ebner]

Am 12.05.22 um 11:24 schrieb Matthias Heiserer:
> Existing disks are not changed by this.
> Especially in benchmarks, iothreads significantly improve IO performance.
> 
> 
> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---
> 
> Changes from v2:
> * also check iothread when adding a disk to an existing VM and 
>  scsi single
> * use bind instead of hardcoded true

This feels like to much automagic to me, because changes to the checkbox
(even if checkbox is for a virtio disk) will change the controller type
and vice versa. This also makes it impossible to only set iothread on
certain disks or use the "Virtio SCSI single" controller type without
setting iothread.

Is it possible to instead have the checkbox be invalid with an
appropriate error for the user when it's a bad configuration?

Re: [pve-devel] [PATCH v3 qemu-server 2/2] Warn in GUI for unlikely iothread config - fixes #3890

[Fabian Ebner]

Please use "fix #3890:" as a prefix to be consistent with how other bug
fix commits do it.

Re: [pve-devel] [PATCH v3 manager 1/2] HDEdit: check iothread by default and move it from advanced section

[Matthias Heiserer]

On 18.05.2022 11:40, Fabian Ebner wrote:
> Am 12.05.22 um 11:24 schrieb Matthias Heiserer:
>> Existing disks are not changed by this.
>> Especially in benchmarks, iothreads significantly improve IO performance.
>>
>>
>> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
>> ---
>>
>> Changes from v2:
>> * also check iothread when adding a disk to an existing VM and
>>   scsi single
>> * use bind instead of hardcoded true
> 
> This feels like to much automagic to me, because changes to the checkbox
> (even if checkbox is for a virtio disk) will change the controller type
> and vice versa. This also makes it impossible to only set iothread on
> certain disks or use the "Virtio SCSI single" controller type without
> setting iothread.
> 
> Is it possible to instead have the checkbox be invalid with an
> appropriate error for the user when it's a bad configuration?

Changes to the checkbox already change the Controller to Virtio SCSI 
(single), regardless of what was selected before. Anyways, the automatic 
change only happens in the wizard.

In case that a user wants iothread with a controller other than SCSI 
single or on a single disk, they can change that in the VM config, but I 
don't think that this is a common problem, at least much less common 
than wanting iothread enabled per default.

But I guess we can show a warning instead.

Re: [pve-devel] [PATCH v3 manager 1/2] HDEdit: check iothread by default and move it from advanced section

[Fabian Ebner]

Am 19.05.22 um 15:35 schrieb Matthias Heiserer:
> On 18.05.2022 11:40, Fabian Ebner wrote:
>> Am 12.05.22 um 11:24 schrieb Matthias Heiserer:
>>> Existing disks are not changed by this.
>>> Especially in benchmarks, iothreads significantly improve IO
>>> performance.
>>>
>>>
>>> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
>>> ---
>>>
>>> Changes from v2:
>>> * also check iothread when adding a disk to an existing VM and
>>>   scsi single
>>> * use bind instead of hardcoded true
>>
>> This feels like to much automagic to me, because changes to the checkbox
>> (even if checkbox is for a virtio disk) will change the controller type
>> and vice versa. This also makes it impossible to only set iothread on
>> certain disks or use the "Virtio SCSI single" controller type without
>> setting iothread.
>>
>> Is it possible to instead have the checkbox be invalid with an
>> appropriate error for the user when it's a bad configuration?
> 
> Changes to the checkbox already change the Controller to Virtio SCSI
> (single), regardless of what was selected before. Anyways, the automatic
> change only happens in the wizard.
> 

You're right, and I'd argue that the current behavior isn't ideal either
;). I guess with only a single scsi disk it makes sense to automatically
switch, because the iothread setting would be invalid otherwise. It also
happens for a virtio disk though, where the scsi controller type isn't
even visible in the disk edit tab. One can still argue that it's just
not relevant there. But we switched to using a multi-disk panel some
time ago, and in that context it's just confusing, because changes to
each iothread checkbox will affect the scsi controller type.

> In case that a user wants iothread with a controller other than SCSI
> single or on a single disk, they can change that in the VM config, but I
> don't think that this is a common problem, at least much less common
> than wanting iothread enabled per default.
> 
> But I guess we can show a warning instead

Re: [pve-devel] [PATCH v3 manager 1/2] HDEdit: check iothread by default and move it from advanced section

[Matthias Heiserer]

On 20.05.2022 08:52, Fabian Ebner wrote:
> Am 19.05.22 um 15:35 schrieb Matthias Heiserer:
>> On 18.05.2022 11:40, Fabian Ebner wrote:
>>> Am 12.05.22 um 11:24 schrieb Matthias Heiserer:
>>>> Existing disks are not changed by this.
>>>> Especially in benchmarks, iothreads significantly improve IO
>>>> performance.
>>>>
>>>>
>>>> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
>>>> ---
>>>>
>>>> Changes from v2:
>>>> * also check iothread when adding a disk to an existing VM and
>>>>    scsi single
>>>> * use bind instead of hardcoded true
>>>
>>> This feels like to much automagic to me, because changes to the checkbox
>>> (even if checkbox is for a virtio disk) will change the controller type
>>> and vice versa. This also makes it impossible to only set iothread on
>>> certain disks or use the "Virtio SCSI single" controller type without
>>> setting iothread.
>>>
>>> Is it possible to instead have the checkbox be invalid with an
>>> appropriate error for the user when it's a bad configuration?
>>
>> Changes to the checkbox already change the Controller to Virtio SCSI
>> (single), regardless of what was selected before. Anyways, the automatic
>> change only happens in the wizard.
>>
> 
> You're right, and I'd argue that the current behavior isn't ideal either
> ;). I guess with only a single scsi disk it makes sense to automatically
> switch, because the iothread setting would be invalid otherwise. It also
> happens for a virtio disk though, where the scsi controller type isn't
> even visible in the disk edit tab. One can still argue that it's just
> not relevant there. But we switched to using a multi-disk panel some
> time ago, and in that context it's just confusing, because changes to
> each iothread checkbox will affect the scsi controller type.
> 
I send in a v4, please take a look.
There is still no warning for bad configurations, rather, I removed the 
bin. Now, you can configure all disks individually, with SCSI single and 
iothread enabled still being the default. The iothread are still binded 
to the scsi controller, so if someone were to change some iothread 
settings, go back to the controller, set it to something other than SCSI 
single, then change it back to scsi single, the iothread configuration 
would be lost/all enabled.

Please let me know what you think about it, if it's closer to what you 
have in mind.