[pve-devel] [PATCH v4 manager 1/3] fix typo

[pve-devel] [PATCH v4 manager 1/3] fix typo

[Matthias Heiserer]

Worked before because if SCSI should be a value, that's set in init.
isScsi is never used.

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

Changes from v3:
New patch

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

diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
index c643ee73..4e4d6aac 100644
--- a/www/manager6/qemu/HDEdit.js
+++ b/www/manager6/qemu/HDEdit.js
@@ -12,7 +12,7 @@ Ext.define('PVE.qemu.HDInputPanel', {
 
     viewModel: {
 	data: {
-	    isScsi: false,
+	    isSCSI: false,
 	    isVirtIO: false,
 	},
     },
-- 
2.30.2

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

[Matthias Heiserer]

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

Changes from v3:
None

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 af258db..2898593 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

Re: [pve-devel] [PATCH v4 qemu-server 1/2] bump pve-common

[Thomas Lamprecht]

This isn't bumping the version of pve-common, but the build-dependency
only (why not the installation dependency too?).

The common commit subject style used for such things, as can be seen by
checking git log on debian/control, is:

"d/control: bump build-dependency of pve-common"

Then a short sentence in the message:

"we require the new <feature/behavior> for XYZ"

On 27/05/2022 13:48, Matthias Heiserer wrote:
> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---
> 
> Changes from v3:
> None
> 
> 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 af258db..2898593 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,

[pve-devel] [PATCH v4 manager] 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 v3:
* remove automatically switching to/from SCSI single when iothread is (un)checked
* iothread will be initially set on
* changing the controller to something other than SCSI single and then back to
  SCSI single will enable iothread for all SCSI disks.

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 | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
index 4e4d6aac..8b460e27 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,
 	},
     },
 
@@ -44,13 +45,10 @@ Ext.define('PVE.qemu.HDInputPanel', {
 	    'field[name=deviceid]': {
 		change: 'fireIdChange',
 	    },
-	    'field[name=iothread]': {
+	    'field[name=scsiController]': {
 		change: function(f, value) {
-		    if (!this.getView().insideWizard) {
-			return;
-		    }
-		    var vmScsiType = value ? 'virtio-scsi-single': 'virtio-scsi-pci';
-		    this.lookupReference('scsiController').setValue(vmScsiType);
+		    let vm = this.getViewModel();
+		    vm.set('isSCSISingle', value === 'virtio-scsi-single');
 		},
 	    },
 	},
@@ -195,6 +193,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 +250,16 @@ Ext.define('PVE.qemu.HDInputPanel', {
 		reference: 'discard',
 		name: 'discard',
 	    },
+	    {
+		xtype: 'proxmoxcheckbox',
+		name: 'iothread',
+		fieldLabel: 'IO thread',
+		clearOnDisable: true,
+		bind: {
+		    disabled: '{!isVirtIO && !isSCSI}',
+		    value: '{isSCSI && isSCSISingle}',
+		},
+	    },
 	);
 
 	advancedColumn1.push(
@@ -263,15 +272,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

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

[Fabian Ebner]

Am 27.05.22 um 13:48 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>

Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>

> ---
> 
> Changes from v3:
> * remove automatically switching to/from SCSI single when iothread is (un)checked
> * iothread will be initially set on
> * changing the controller to something other than SCSI single and then back to
>   SCSI single will enable iothread for all SCSI disks.

I like this version much more. We could still add validation/warning to
the checkbox, but IMHO not a big deal with the new default and improved
warning upon VM start. Just wondering if we should enable iothread by
default for virtio disks too, or is the performance improvement not as
relevant there?

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

[Matthias Heiserer]

On 10.06.2022 11:20, Fabian Ebner wrote:
> Am 27.05.22 um 13:48 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>
> 
> Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>
> 
>> ---
>>
>> Changes from v3:
>> * remove automatically switching to/from SCSI single when iothread is (un)checked
>> * iothread will be initially set on
>> * changing the controller to something other than SCSI single and then back to
>>    SCSI single will enable iothread for all SCSI disks.
> 
> I like this version much more. We could still add validation/warning to
> the checkbox, but IMHO not a big deal with the new default and improved
> warning upon VM start. Just wondering if we should enable iothread by
> default for virtio disks too, or is the performance improvement not as
> relevant there?
Okay great, then I won't change the UI in v5.

Just did a very quick test and on my machine it seems to make read 
benchmarks faster, but write is slightly slower. Will enable it in v5.

fio --ioengine=libaio --direct=1 --sync=1 --rw=read --bs=4k --numjobs=1 
--iodepth=1 --runtime=60 --time_based --name seq_read --filename=/dev/vdb

With iothread:
read: IOPS=27.1k, BW=106MiB/s
write: IOPS=707, BW=2831KiB/s

Without iothread:
read: IOPS=13.4k, BW=52.5MiB/s
write: IOPS=787, BW=3150KiB/s

[pve-devel] [PATCH v4 qemu-server 2/2] fix #3890: 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>
---

Changes from v3:
None

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 e9aa248..2536d73 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 v4 qemu-server 2/2] fix #3890: warn in GUI for unlikely iothread config

[Fabian Ebner]

Am 27.05.22 um 13:48 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>

Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>

> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index e9aa248..2536d73 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);

Nit: not ordered alphabetically

[pve-devel] [PATCH v4 manager 3/3] 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/v3

 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

Re: [pve-devel] [PATCH v4 manager 3/3] OS defaults: use SCSI single as default controller

[Fabian Ebner]

Am 27.05.22 um 13:48 schrieb Matthias Heiserer:
> Existing installs are not changed by this.
> 
> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---

Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>

[pve-devel] applied: [PATCH v4 manager 1/3] fix typo

[Thomas Lamprecht]

On 27/05/2022 13:48, Matthias Heiserer wrote:
> Worked before because if SCSI should be a value, that's set in init.
> isScsi is never used.
> 
> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---
> 
> Changes from v3:
> New patch
> 
>  www/manager6/qemu/HDEdit.js | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, but amended the commit to add an actual telling commit subject
and reworded the message a bit, thanks!