[pve-devel] [PATCH v2 manager 1/2] GUI: HDEdit: check iothread by default

[pve-devel] [PATCH v2 manager 1/2] GUI: HDEdit: check iothread by default

[Matthias Heiserer]

And move it from the advanced section to the always shown area.

Because of the propertyStringSet call in onGetValues, existing VM
show the correct iothread value.

Especially in benchmarks, iothreads significantly improve IO performance.

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

Changes from v1:
set true by default instead of binding it

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

diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
index c643ee73..b9c5cc5c 100644
--- a/www/manager6/qemu/HDEdit.js
+++ b/www/manager6/qemu/HDEdit.js
@@ -251,6 +251,16 @@ Ext.define('PVE.qemu.HDInputPanel', {
 		reference: 'discard',
 		name: 'discard',
 	    },
+	    {
+		xtype: 'proxmoxcheckbox',
+		name: 'iothread',
+		fieldLabel: 'IO thread',
+		clearOnDisable: true,
+		bind: {
+		    disabled: '{!isVirtIO && !isSCSI}',
+		},
+		value: true,
+	    },
 	);
 
 	advancedColumn1.push(
@@ -263,15 +273,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 v2 manager 2/2] GUI: OS defaults: use SCSI single as default controller

[Matthias Heiserer]

That's the SCSI controller the wizard selects by default.
Existing installs are not changed by this.

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

No changes from v1

 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 v2 qemu-server 1/1] Warn in GUI for unlikely iothread config

[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>
---

No changes from v1

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

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 5366df2..d1f612b 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3952,7 +3952,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";
+		PVE::RESTEnvironment::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 v2 qemu-server 1/1] Warn in GUI for unlikely iothread config

[Fabian Ebner]

Am 28.04.22 um 10:44 schrieb 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>
> ---

Note: requires a dependency bump for libpve-common-perl (it's currently
at 7.1-3 but the static log_warn was added just after that).

And it fixes #3890 :)

Re: [pve-devel] [PATCH v2 qemu-server 1/1] Warn in GUI for unlikely iothread config

[Fabian Ebner]

Am 10.05.22 um 09:26 schrieb Fabian Ebner:
> Am 28.04.22 um 10:44 schrieb 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>
>> ---
> 
> Note: requires a dependency bump for libpve-common-perl (it's currently
> at 7.1-3 but the static log_warn was added just after that).
> 
> And it fixes #3890 :)
> 

Also missing a
use PVE::RESTEnvironment;

Re: [pve-devel] [PATCH v2 manager 1/2] GUI: HDEdit: check iothread by default

[Daniel Tschlatscher]

I applied this patch in a few virtualized Proxmox instances. Existing
VM disk and controller settings were correctly applied, adding new
disks worked as expected.
I tried to find some settings combinations in the Editor panel that
would be wrong or nonsensical. Though, I couldn't find any problems
in this regard either.

I also ran a few (rather straightforward) benchmarks using fio and dd.
While fio write commands were only slightly faster, read benchmarks
were nearly twice as fast.
Writing with dd using IOThreads yielded a speed increase of about 50%
compared to a disk not using an IO thread.


Everything worked as advertised.

Tested-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>


On 4/28/22 10:44, Matthias Heiserer wrote:
> And move it from the advanced section to the always shown area.
>
> Because of the propertyStringSet call in onGetValues, existing VM
> show the correct iothread value.
>
> Especially in benchmarks, iothreads significantly improve IO performance.
>
> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---
>
> Changes from v1:
> set true by default instead of binding it
>
>   www/manager6/qemu/HDEdit.js | 19 ++++++++++---------
>   1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
> index c643ee73..b9c5cc5c 100644
> --- a/www/manager6/qemu/HDEdit.js
> +++ b/www/manager6/qemu/HDEdit.js
> @@ -251,6 +251,16 @@ Ext.define('PVE.qemu.HDInputPanel', {
>   		reference: 'discard',
>   		name: 'discard',
>   	    },
> +	    {
> +		xtype: 'proxmoxcheckbox',
> +		name: 'iothread',
> +		fieldLabel: 'IO thread',
> +		clearOnDisable: true,
> +		bind: {
> +		    disabled: '{!isVirtIO && !isSCSI}',
> +		},
> +		value: true,
> +	    },
>   	);
>   
>   	advancedColumn1.push(
> @@ -263,15 +273,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

Re: [pve-devel] [PATCH v2 manager 1/2] GUI: HDEdit: check iothread by default

[Fabian Ebner]

Am 28.04.22 um 10:44 schrieb Matthias Heiserer:
> And move it from the advanced section to the always shown area.
> 
> Because of the propertyStringSet call in onGetValues, existing VM
> show the correct iothread value.
> 
> Especially in benchmarks, iothreads significantly improve IO performance.
> 

It's checked in the creation wizard, but (at least for me) not when
adding a new disk afterwards, which is unexpected.

Can we figure out the controller type here (maybe by passing along from
the parent component)? Then we could auto-select depending on the
controller type and maybe even display a quick note if there's mismatch.
As currently, people selecting an incompatible controller type will
still get the checkbox ticked automatically.