[PATCH manager v3] fix #4503: ui: qemu: add detect zeroes disk option

[PATCH manager v3] 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.

Allow configuring the option independently so 'discard' can stay enabled
while zero detection is disabled. Persist the checkbox state explicitly,
as the backend treats an absent value as default-enabled behavior,
resulting in 'detect-zeroes=unmap' when 'discard' is enabled.

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=4503
Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
---

Notes:
    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:
    - added bug-numer to commit header
    - appended bug-link to commit body

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

diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
index 1bb2bfda..2708d452 100644
--- a/www/manager6/qemu/HDEdit.js
+++ b/www/manager6/qemu/HDEdit.js
@@ -96,6 +96,9 @@ 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');
+        // persist an explicit false value, since the backend defaults to enabled
+        // when the option is omitted.
+        me.drive.detect_zeroes = values.detect_zeroes ? 1 : 0;
 
         ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'].forEach((name) => {
             let burst_name = `${name}_max`;
@@ -152,6 +155,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 +321,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

Re: [PATCH manager v3] fix #4503: ui: qemu: add detect zeroes disk option

[David Riley]

On 6/18/26 1:49 PM, Lukas Sichert wrote:
> 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.
>
> Allow configuring the option independently so 'discard' can stay enabled
> while zero detection is disabled. Persist the checkbox state explicitly,
> as the backend treats an absent value as default-enabled behavior,
> resulting in 'detect-zeroes=unmap' when 'discard' is enabled.

Thanks for submitting this patch.

While it clearly addresses the performance issues reported in #4503
[0], I was reading through the Bugzilla thread and noticed a request
to support the third QEMU state for detect_zeroes [1]. Currently, the
underlying parameter supports off, on, and unmap [2].

Since we are already modifying this configuration in the UI, it might
make sense to fully expose all three options to the user rather than
just a binary toggle as there is a difference between "on" and
"unmap".

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=4503
[1] https://bugzilla.proxmox.com/show_bug.cgi?id=4503#c8
[2] https://www.qemu.org/docs/master/interop/qemu-storage-daemon-qmp-ref.html#enum-QSD-block-core.BlockdevDetectZeroesOptions

> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=4503
> Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
> ---
>
> Notes:
>      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:
>      - added bug-numer to commit header
>      - appended bug-link to commit body
>
>   www/manager6/qemu/HDEdit.js | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> [...]
>

Re: [PATCH manager v3] fix #4503: ui: qemu: add detect zeroes disk option

[Daniel Kral]

On Fri Jun 19, 2026 at 1:01 PM CEST, David Riley wrote:
> On 6/18/26 1:49 PM, Lukas Sichert wrote:
>> 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.
>>
>> Allow configuring the option independently so 'discard' can stay enabled
>> while zero detection is disabled. Persist the checkbox state explicitly,
>> as the backend treats an absent value as default-enabled behavior,
>> resulting in 'detect-zeroes=unmap' when 'discard' is enabled.
>
> Thanks for submitting this patch.
>
> While it clearly addresses the performance issues reported in #4503
> [0], I was reading through the Bugzilla thread and noticed a request
> to support the third QEMU state for detect_zeroes [1]. Currently, the
> underlying parameter supports off, on, and unmap [2].
>
> Since we are already modifying this configuration in the UI, it might
> make sense to fully expose all three options to the user rather than
> just a binary toggle as there is a difference between "on" and
> "unmap".

This would need adapting the detect_zeroes option and parsing logic
(it's only boolean right now) so it can take any of the 3 valid values
by QEMU and be backwards compatible to the old values in our config.

This should probably also check the constraint that detect-zeroes=unmap
can only be set if discard=unmap, but the error message seems already be
quite clear for that [0].

[0] https://gitlab.com/qemu-project/qemu/-/blob/master/block.c?ref_type=heads#L1120

>
> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=4503
> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=4503#c8
> [2] https://www.qemu.org/docs/master/interop/qemu-storage-daemon-qmp-ref.html#enum-QSD-block-core.BlockdevDetectZeroesOptions
>
>> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=4503
>> Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
>> ---
>>
>> Notes:
>>      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:
>>      - added bug-numer to commit header
>>      - appended bug-link to commit body
>>
>>   www/manager6/qemu/HDEdit.js | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> [...]
>>

Re: [PATCH manager v3] fix #4503: ui: qemu: add detect zeroes disk option

[Lukas Sichert]

On 2026-06-22 10:51, Daniel Kral <d.kral@proxmox.com> wrote:

> On Fri Jun 19, 2026 at 1:01 PM CEST, David Riley wrote:
>> On 6/18/26 1:49 PM, Lukas Sichert wrote:
>>> 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.
>>>
>>> Allow configuring the option independently so 'discard' can stay enabled
>>> while zero detection is disabled. Persist the checkbox state explicitly,
>>> as the backend treats an absent value as default-enabled behavior,
>>> resulting in 'detect-zeroes=unmap' when 'discard' is enabled.
>>
>> Thanks for submitting this patch.
>>
>> While it clearly addresses the performance issues reported in #4503
>> [0], I was reading through the Bugzilla thread and noticed a request
>> to support the third QEMU state for detect_zeroes [1]. Currently, the
>> underlying parameter supports off, on, and unmap [2].
>>
>> Since we are already modifying this configuration in the UI, it might
>> make sense to fully expose all three options to the user rather than
>> just a binary toggle as there is a difference between "on" and
>> "unmap".
>
> This would need adapting the detect_zeroes option and parsing logic
> (it's only boolean right now) so it can take any of the 3 valid values
> by QEMU and be backwards compatible to the old values in our config.
>
> This should probably also check the constraint that detect-zeroes=unmap
> can only be set if discard=unmap, but the error message seems already be
> quite clear for that [0].

Already in src/PVE/QemuServer/Drive.pm in the qemu-server repo, this is
checked and set accordingly.

Since detect-zeroes=unmap is essentially an extension of
detect-zeroes=on [1], and detect-zeroes=unmap is currently the default
when discard is enabled, I would keep this patch as is to preserve
backward compatibility.

[1]: https://www.qemu.org/docs/master/interop/qemu-storage-daemon-qmp-ref.html#enum-QSD-block-core.BlockdevDetectZeroesOptions

> [0] https://gitlab.com/qemu-project/qemu/-/blob/master/block.c?ref_type=heads#L1120
>
>>
>> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=4503
>> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=4503#c8
>> [2] https://www.qemu.org/docs/master/interop/qemu-storage-daemon-qmp-ref.html#enum-QSD-block-core.BlockdevDetectZeroesOptions
>>
>>> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=4503
>>> Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
>>> ---
>>>
>>> Notes:
>>>      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:
>>>      - added bug-numer to commit header
>>>      - appended bug-link to commit body
>>>
>>>   www/manager6/qemu/HDEdit.js | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> [...]
>>>

Re: [PATCH manager v3] fix #4503: ui: qemu: add detect zeroes disk option

[Fiona Ebner]

Am 23.06.26 um 4:06 PM schrieb Lukas Sichert:
> On 2026-06-22 10:51, Daniel Kral <d.kral@proxmox.com> wrote:
>> On Fri Jun 19, 2026 at 1:01 PM CEST, David Riley wrote:
>>> On 6/18/26 1:49 PM, Lukas Sichert wrote:
>>>> 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.
>>>>
>>>> Allow configuring the option independently so 'discard' can stay enabled
>>>> while zero detection is disabled. Persist the checkbox state explicitly,
>>>> as the backend treats an absent value as default-enabled behavior,
>>>> resulting in 'detect-zeroes=unmap' when 'discard' is enabled.
>>>
>>> Thanks for submitting this patch.
>>>
>>> While it clearly addresses the performance issues reported in #4503
>>> [0], I was reading through the Bugzilla thread and noticed a request
>>> to support the third QEMU state for detect_zeroes [1]. Currently, the
>>> underlying parameter supports off, on, and unmap [2].
>>>
>>> Since we are already modifying this configuration in the UI, it might
>>> make sense to fully expose all three options to the user rather than
>>> just a binary toggle as there is a difference between "on" and
>>> "unmap".
>>
>> This would need adapting the detect_zeroes option and parsing logic
>> (it's only boolean right now) so it can take any of the 3 valid values
>> by QEMU and be backwards compatible to the old values in our config.
>>
>> This should probably also check the constraint that detect-zeroes=unmap
>> can only be set if discard=unmap, but the error message seems already be
>> quite clear for that [0].
> 
> Already in src/PVE/QemuServer/Drive.pm in the qemu-server repo, this is
> checked and set accordingly.
> 
> Since detect-zeroes=unmap is essentially an extension of
> detect-zeroes=on [1], and detect-zeroes=unmap is currently the default
> when discard is enabled, I would keep this patch as is to preserve
> backward compatibility.

I also think it's fine to keep it as-is.

Re: [PATCH manager v3] fix #4503: ui: qemu: add detect zeroes disk option

[Fiona Ebner]

Am 18.06.26 um 1:49 PM schrieb 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.
> 
> Allow configuring the option independently so 'discard' can stay enabled
> while zero detection is disabled. Persist the checkbox state explicitly,
> as the backend treats an absent value as default-enabled behavior,
> resulting in 'detect-zeroes=unmap' when 'discard' is enabled.
> 
> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=4503
> Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
> ---
> 
> Notes:
>     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:
>     - added bug-numer to commit header
>     - appended bug-link to commit body
> 
>  www/manager6/qemu/HDEdit.js | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
> index 1bb2bfda..2708d452 100644
> --- a/www/manager6/qemu/HDEdit.js
> +++ b/www/manager6/qemu/HDEdit.js
> @@ -96,6 +96,9 @@ 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');
> +        // persist an explicit false value, since the backend defaults to enabled
> +        // when the option is omitted.
> +        me.drive.detect_zeroes = values.detect_zeroes ? 1 : 0;

But this will now persist both, an explicit 1 and and explicit 0,
needlessly cluttering the configuration in one case.

Note that we already have a property that is 1 by default rather than 0,
namely 'backup'. It still uses propertyStringSet(), but inverts the
value passed for 'source'. I think this is a hack though, as it only
works because of the explicit 'value' override. We could fix this up
with something like:

>         propertyStringSet: function (target, source, name, value, defaultIsTrue) {
>             if (defaultIsTrue ? !source : source) {

Alternatively, we could use something like:

>         propertyStringSet: function (target, isNotDefault, name, value) {
>             if (isNotDefault) {
>                 target[name] = value;
>             } else {
>                 delete target[name];
>             }
>         },

and always pass an explicit 'value'. The function as a whole becomes
simpler rather than more complex so maybe that's preferable.

And then use it for 'backup' and 'detect_zeroes'.

In fact, there is another one which is true by default, namely
'replicate', but there it's switched around by being 'noreplicate'.
Still, its call to propertyStringSet() might also considered a bit hacky
and only works because a custom 'value' is explicitly specified.

>  
>          ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'].forEach((name) => {
>              let burst_name = `${name}_max`;
> @@ -152,6 +155,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 +321,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',