[PATCH manager v4 0/3] fix #4503: add configurable detect-zeroes disk option

[PATCH manager v4 0/3] fix #4503: add configurable detect-zeroes disk option

[Lukas Sichert]

When discard is enabled, qemu-server defaults to detect-zeroes=unmap,
which can negatively affect zero-write performance on some storage
setups.

This series allows users to disable zero detection while keeping guest
discard/TRIM support enabled. The first patch extends
propertyStringSet() to support properties which default to true. The
second patch converts the existing backup option to use this handling.
The final patch adds the detect-zeroes option to the disk editor.

Buglink: https://bugzilla.proxmox.com/show_bug.cgi?id=4503

changes from v3 to v4 (thanks @Fiona):
- split the change into a three-patch series
- extend propertyStringSet() to support properties
  which default to true
- use the new handling for the existing backup property
- omit detect_zeroes when enabled and only serialize detect_zeroes=0
  when explicitly disabled

changes from v2 to v3 (thanks @Fabian):
- add a comment explaining why this property is handled differently from
  the others

changes from v1 to v2:
- add the bug number to the commit header
- append the buglink to the commit body


manager:

Lukas Sichert (3):
  ui: utils: support true defaults in propertyStringSet
  ui: qemu: refactor backup property handling
  fix #4503: ui: qemu: add detect zeroes disk option

 www/manager6/Utils.js       |  4 ++--
 www/manager6/qemu/HDEdit.js | 10 +++++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)


Summary over all repositories:
  2 files changed, 11 insertions(+), 3 deletions(-)

-- 
Generated by murpp 0.12.0

[PATCH manager v4 1/3] ui: utils: support true defaults in propertyStringSet

[Lukas Sichert]

Allow callers to specify that a property defaults to true. This is
useful for options where only explicitly disabling the default behavior
needs to be written to the property string.

Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
---
 www/manager6/Utils.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index c86a00c5..cb70965d 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1929,8 +1929,8 @@ Ext.define('PVE.Utils', {
             );
         },
 
-        propertyStringSet: function (target, source, name, value) {
-            if (source) {
+        propertyStringSet: function (target, source, name, value, defaultIsTrue) {
+            if (defaultIsTrue ? !source : source) {
                 if (value === undefined) {
                     target[name] = source;
                 } else {
-- 
2.47.3

[PATCH manager v4 2/3] ui: qemu: refactor backup property handling

[Lukas Sichert]

Use the previously added true-default handling to avoid manually
inverting the backup value.

Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
---
 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 1bb2bfda..3ac40f42 100644
--- a/www/manager6/qemu/HDEdit.js
+++ b/www/manager6/qemu/HDEdit.js
@@ -88,7 +88,7 @@ Ext.define('PVE.qemu.HDInputPanel', {
             me.drive.format = values.diskformat;
         }
 
-        PVE.Utils.propertyStringSet(me.drive, !values.backup, 'backup', '0');
+        PVE.Utils.propertyStringSet(me.drive, values.backup, 'backup', '0', true);
         PVE.Utils.propertyStringSet(me.drive, values.noreplicate, 'replicate', 'no');
         PVE.Utils.propertyStringSet(me.drive, values.discard, 'discard', 'on');
         PVE.Utils.propertyStringSet(me.drive, values.ssd, 'ssd', 'on');
-- 
2.47.3

[PATCH manager v4 3/3] fix #4503: ui: qemu: add detect zeroes disk option

[Lukas Sichert]

When discard is enabled, qemu-server defaults to 'detect-zeroes=unmap'.
This can have a significant performance impact for zero writes on some
setups, while users might still want to keep guest discard/TRIM support
enabled.

Expose the option in the disk editor and default it to enabled to match
the backend behavior. Use the true-default property serialization added
previously so that disabling it explicitly writes 'detect_zeroes=0',
while enabling it omits the property.

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=4503
Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
---
 www/manager6/qemu/HDEdit.js | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
index 3ac40f42..9aae91b2 100644
--- a/www/manager6/qemu/HDEdit.js
+++ b/www/manager6/qemu/HDEdit.js
@@ -96,6 +96,7 @@ Ext.define('PVE.qemu.HDInputPanel', {
         PVE.Utils.propertyStringSet(me.drive, values.readOnly, 'ro', 'on');
         PVE.Utils.propertyStringSet(me.drive, values.cache, 'cache');
         PVE.Utils.propertyStringSet(me.drive, values.aio, 'aio');
+        PVE.Utils.propertyStringSet(me.drive, values.detect_zeroes, 'detect_zeroes', '0', true);
 
         ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'].forEach((name) => {
             let burst_name = `${name}_max`;
@@ -152,6 +153,7 @@ Ext.define('PVE.qemu.HDInputPanel', {
         values.diskformat = drive.format || 'raw';
         values.cache = drive.cache || '__default__';
         values.discard = drive.discard === 'on';
+        values.detect_zeroes = PVE.Parser.parseBoolean(drive.detect_zeroes, 1);
         values.ssd = PVE.Parser.parseBoolean(drive.ssd);
         values.iothread = PVE.Parser.parseBoolean(drive.iothread);
         values.readOnly = PVE.Parser.parseBoolean(drive.ro);
@@ -317,6 +319,12 @@ Ext.define('PVE.qemu.HDInputPanel', {
                 reference: 'discard',
                 name: 'discard',
             },
+            {
+                xtype: 'proxmoxcheckbox',
+                fieldLabel: gettext('Detect zeroes'),
+                reference: 'detect_zeroes',
+                name: 'detect_zeroes',
+            },
             {
                 xtype: 'proxmoxcheckbox',
                 name: 'iothread',
-- 
2.47.3