[RFC common/manager/qemu-server 0/3] fix #1989: qemu: add qcow2 cache options

[RFC common/manager/qemu-server 0/3] fix #1989: qemu: add qcow2 cache options

[Erik Fastermann]

Hi all,

I'm interested in feedback for my patches fixing #1989 [0], which add multiple
options to configure the qcow2 L2/refcount cache. This enables huge performance
gains in some cases.

I tested this for the current and older QEMU machine versions and while a VM was
running. The add disk (vm creation and otherwise), import disk and edit disk
dialogs were considered.

I also have the following questions:

- Currently I also check the file extension is qcow2, because in many cases the
  disk format is currently not available (especially on the frontend). Is this
  good enough in general?

- The cache size input on the frontend is currently in bytes, because fine
  control might be required in some cases, but this is a little bit ugly.
  Is there a better way to do this?

- Should the ability to control cluster_size and refcount_bits be included in
  this patch or should a separate bug be created? There was some discussion
  about this on Bugzilla [0].

- Should minimum / maximum values be defined for all options and in what way?

- Should we validate that the cache sizes are multiples of the cluster size /
  cache entry size?

- I'm using oneOf for the cache_size to support the 'based-on-disk' variant, but
  this does not play nice with the generated Rust API types (currently set the
  outer type to string, but this probably does not work). Should this be changed
  or the Rust generation extended or can I use a custom enum on the Rust side?

- Should any extra documentation be added for this feature?

Thanks for your feedback!

Best,
Erik

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=1989


pve-common:

Erik Fastermann (1):
  json schema: display nested oneOf error messages

 src/PVE/JSONSchema.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


qemu-server:

Erik Fastermann (1):
  fix #1989: disk: add qcow2 cache options

 src/PVE/QemuServer.pm          | 33 ++++++++++++
 src/PVE/QemuServer/Blockdev.pm | 33 ++++++++++++
 src/PVE/QemuServer/Drive.pm    | 99 ++++++++++++++++++++++++++++++++++
 3 files changed, 165 insertions(+)


pve-manager:

Erik Fastermann (1):
  fix #1989: qemu: disk: add cache size config

 www/manager6/Parser.js      |  2 +-
 www/manager6/qemu/HDEdit.js | 70 +++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletion(-)


Summary over all repositories:
  6 files changed, 238 insertions(+), 3 deletions(-)

-- 
Generated by murpp 0.11.0

[PATCH pve-common 1/3] json schema: display nested oneOf error messages

[Erik Fastermann]

Correctly display the error message for each oneOf case, not just the
empty string.

Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
---
 src/PVE/JSONSchema.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index 1997c70..fd10338 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -1409,13 +1409,13 @@ sub check_prop {
                 }
 
                 for my $inner_path (keys $inner_errors->%*) {
-                    add_error($collected_errors, $inner_path, $inner_errors->{$path});
+                    add_error($collected_errors, $inner_path, $inner_errors->{$inner_path});
                 }
             }
 
             if (!$is_valid) {
                 for my $inner_path (keys $collected_errors->%*) {
-                    add_error($errors, $inner_path, $collected_errors->{$path});
+                    add_error($errors, $inner_path, $collected_errors->{$inner_path});
                 }
             }
         }
-- 
2.47.3

[PATCH qemu-server 2/3] fix #1989: disk: add qcow2 cache options

[Erik Fastermann]

Add multiple options to configure the qcow2 L2/refcount cache. This
enables huge performance gains in some cases.

For a detailed explanation of the options see the QEMU docs [0].
Additionally the cache size can be configured based on the size of the
disk image automatically.

Both blockdev and the older drive commandline are supported, which makes
the feature accessible to all QEMU machine versions supported by PVE.
Options which only apply to disk creation (cluster_size, refcount_bits)
are not considered in this patch.

[0] https://gitlab.com/qemu-project/qemu/-/blob/master/docs/qcow2-cache.txt

Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
---
 src/PVE/QemuServer.pm          | 33 ++++++++++++
 src/PVE/QemuServer/Blockdev.pm | 33 ++++++++++++
 src/PVE/QemuServer/Drive.pm    | 99 ++++++++++++++++++++++++++++++++++
 3 files changed, 165 insertions(+)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 118f26bc..e53ae760 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1294,6 +1294,39 @@ sub print_drive_commandline_full {
         $opts .= ",auto-remove=on";
     }
 
+    if (defined($drive->{cache_size})) {
+        if ($drive->{cache_size} eq 'based-on-disk') {
+            # Automatically set the cache-size based on the disk size. The formula
+            # is adapted from here:
+            # https://gitlab.com/qemu-project/qemu/-/blob/master/docs/qcow2-cache.txt#L97-98
+            # Assumes the default values for cluster_size and refcount_bits and
+            # that extended L2 entries are not used. As this combines the refcount
+            # and L2 cache sizes, QEMU is free to choose a different value for each
+            # of them, which means the L2 cache is not necessarily 4 times bigger
+            # than the refcount cache.
+            my $cache_size = $drive->{size} / 1024 / 1024 / 1024 * (131072 + 32768);
+            $opts .= ",cache-size=" . int($cache_size);
+        } else {
+            $opts .= ",cache-size=" . int($drive->{cache_size});
+        }
+    }
+
+    if (defined($drive->{l2_cache_size})) {
+        $opts .= ",cache-size=" . int($drive->{l2_cache_size});
+    }
+
+    if (defined($drive->{l2_cache_entry_size})) {
+        $opts .= ",l2-cache-entry-size=" . int($drive->{l2_cache_entry_size});
+    }
+
+    if (defined($drive->{refcount_cache_size})) {
+        $opts .= ",refcount-cache-size=" . int($drive->{refcount_cache_size});
+    }
+
+    if (defined($drive->{cache_clean_interval})) {
+        $opts .= ",cache-clean-interval=" . int($drive->{cache_clean_interval});
+    }
+
     # my $file_param = $live_restore_name ? "file.file.filename" : "file";
     my $file_param = "file";
     if ($live_restore_name) {
diff --git a/src/PVE/QemuServer/Blockdev.pm b/src/PVE/QemuServer/Blockdev.pm
index 101c747c..7cb7dccb 100644
--- a/src/PVE/QemuServer/Blockdev.pm
+++ b/src/PVE/QemuServer/Blockdev.pm
@@ -403,6 +403,39 @@ sub generate_format_blockdev {
         $blockdev->{'discard-no-unref'} = JSON::true if $format eq 'qcow2';
     }
 
+    if (defined($drive->{cache_size})) {
+        if ($drive->{cache_size} eq 'based-on-disk') {
+            # Automatically set the cache-size based on the disk size. The formula
+            # is adapted from here:
+            # https://gitlab.com/qemu-project/qemu/-/blob/master/docs/qcow2-cache.txt#L97-98
+            # Assumes the default values for cluster_size and refcount_bits and
+            # that extended L2 entries are not used. As this combines the refcount
+            # and L2 cache sizes, QEMU is free to choose a different value for each
+            # of them, which means the L2 cache is not necessarily 4 times bigger
+            # than the refcount cache.
+            my $cache_size = $drive->{size} / 1024 / 1024 / 1024 * (131072 + 32768);
+            $blockdev->{'cache-size'} = int($cache_size);
+        } else {
+            $blockdev->{'cache-size'} = int($drive->{cache_size});
+        }
+    }
+
+    if (defined($drive->{l2_cache_size})) {
+        $blockdev->{'l2-cache-size'} = int($drive->{l2_cache_size});
+    }
+
+    if (defined($drive->{l2_cache_entry_size})) {
+        $blockdev->{'l2-cache-entry-size'} = int($drive->{l2_cache_entry_size});
+    }
+
+    if (defined($drive->{refcount_cache_size})) {
+        $blockdev->{'refcount-cache-size'} = int($drive->{refcount_cache_size});
+    }
+
+    if (defined($drive->{cache_clean_interval})) {
+        $blockdev->{'cache-clean-interval'} = int($drive->{cache_clean_interval});
+    }
+
     return $blockdev;
 }
 
diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm
index b80b7dbb..ff4be684 100644
--- a/src/PVE/QemuServer/Drive.pm
+++ b/src/PVE/QemuServer/Drive.pm
@@ -256,6 +256,51 @@ my %drivedesc_base = (
         optional => 1,
         default => 0,
     },
+    cache_size => {
+        type => 'string',
+        oneOf => [
+            {
+                type => 'integer',
+                minimum => 1,
+                description => 'Cache size in bytes',
+                optional => 1,
+            },
+            {
+                type => 'string',
+                enum => ['based-on-disk'],
+                description =>
+                    'Automatically pick a cache size based on the configured disk size',
+                optional => 1,
+            },
+        ],
+        description => 'Cache size for qcow2 disks in bytes',
+        typetext => '(<bytes> | based-on-disk)',
+        optional => 1,
+    },
+    l2_cache_size => {
+        type => 'integer',
+        minimum => 1,
+        description => 'L2 cache size for qcow2 disks in bytes',
+        optional => 1,
+    },
+    l2_cache_entry_size => {
+        type => 'integer',
+        minimum => 512,
+        description => 'L2 cache entry size for qcow2 disks in bytes',
+        optional => 1,
+    },
+    refcount_cache_size => {
+        type => 'integer',
+        minimum => 1,
+        description => 'Refcount cache size for qcow2 disks in bytes',
+        optional => 1,
+    },
+    cache_clean_interval => {
+        type => 'integer',
+        minimum => 0,
+        description => 'Cache clean interval for qcow2 disks in seconds',
+        optional => 1,
+    },
 );
 
 my %iothread_fmt = (
@@ -838,6 +883,60 @@ sub parse_drive {
         }
     }
 
+    my $is_qcow2 =
+        (defined($res->{format}) && $res->{format} eq 'qcow2') || $res->{file} =~ /\.qcow2$/;
+
+    my $any_qcow2_cache_option =
+        defined($res->{cache_size})
+        || defined($res->{l2_cache_size})
+        || defined($res->{l2_cache_entry_size})
+        || defined($res->{refcount_cache_size})
+        || defined($res->{cache_clean_interval});
+
+    if ($any_qcow2_cache_option && !$is_qcow2) {
+        warn "qcow2 cache options require disk format qcow2\n";
+        ++$error;
+    }
+
+    my $cache_option_count =
+        defined($res->{cache_size}) +
+        defined($res->{l2_cache_size}) +
+        defined($res->{refcount_cache_size});
+
+    if ($cache_option_count > 2) {
+        warn "at most two of {,l2_,refcount_}cache_size can be set simultaneously\n";
+        ++$error;
+    }
+
+    sub is_power_of_two {
+        my ($n) = @_;
+        return $n > 0 && (($n & ($n - 1)) == 0);
+    }
+
+    if (defined($res->{l2_cache_entry_size}) && !is_power_of_two($res->{l2_cache_entry_size})) {
+        warn "l2_cache_entry_size must be a power of two\n";
+        ++$error;
+    }
+
+    if (defined($res->{cache_size}) && $res->{cache_size} eq 'based-on-disk') {
+        if (defined($res->{l2_cache_size}) || defined($res->{refcount_cache_size})) {
+            warn "cache_size 'based-on-disk' not compatible with l2 or refcount cache size set\n";
+            ++$error;
+        }
+    }
+
+    if (defined($res->{cache_size}) && $res->{cache_size} ne 'based-on-disk') {
+        if (($res->{l2_cache_size} // 0) >= $res->{cache_size}) {
+            warn "l2_cache_size is larger than or equal to cache_size\n";
+            ++$error;
+        }
+
+        if (($res->{refcount_cache_size} // 0) >= $res->{cache_size}) {
+            warn "refcount_cache_size is larger than or equal to cache_size\n";
+            ++$error;
+        }
+    }
+
     return if $error;
 
     return if $res->{mbps_rd} && $res->{mbps};
-- 
2.47.3

[PATCH pve-manager 3/3] fix #1989: qemu: disk: add cache size config

[Erik Fastermann]

Add the option to control the size of the qcow2 L2/refcount cache in
the vm disk create and edit dialogs. This enables huge performance
gains in some vm configurations.

For a detailed explanation see the QEMU docs [0]. Currently only the
cache-size is configurable in the frontend, either directly or based on
the size of the disk.

[0] https://gitlab.com/qemu-project/qemu/-/blob/master/docs/qcow2-cache.txt

Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
---
 www/manager6/Parser.js      |  2 +-
 www/manager6/qemu/HDEdit.js | 70 +++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/www/manager6/Parser.js b/www/manager6/Parser.js
index 36df8e9d..90467316 100644
--- a/www/manager6/Parser.js
+++ b/www/manager6/Parser.js
@@ -207,7 +207,7 @@ Ext.define('PVE.Parser', {
                 if (!p || p.match(/^\s*$/)) {
                     return undefined; // continue
                 }
-                let match = p.match(/^([a-z_]+)=(\S+)$/);
+                let match = p.match(/^([0-9a-z_]+)=(\S+)$/);
                 if (!match) {
                     if (!p.match(/[=]/)) {
                         res.file = p;
diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
index 1bb2bfda..401b8032 100644
--- a/www/manager6/qemu/HDEdit.js
+++ b/www/manager6/qemu/HDEdit.js
@@ -18,6 +18,8 @@ Ext.define('PVE.qemu.HDInputPanel', {
             isSCSI: false,
             isVirtIO: false,
             isSCSISingle: false,
+            isQcow2: false,
+            cacheSizeBasedOnDisk: false,
         },
     },
 
@@ -54,6 +56,22 @@ Ext.define('PVE.qemu.HDInputPanel', {
                     vm.set('isSCSISingle', value === 'virtio-scsi-single');
                 },
             },
+            'field[name=diskformat]': {
+                change: function (f, value) {
+                    this.getViewModel().set('isQcow2', value === 'qcow2');
+                },
+                afterrender: function (f) {
+                    this.getViewModel().set('isQcow2', f.getValue() === 'qcow2');
+                },
+            },
+            'field[name=cacheSizeBasedOnDisk]': {
+                change: function (f, value) {
+                    this.getViewModel().set('cacheSizeBasedOnDisk', value);
+                },
+                afterrender: function (f) {
+                    this.getViewModel().set('cacheSizeBasedOnDisk', f.getValue());
+                },
+            },
         },
 
         init: function (view) {
@@ -88,6 +106,16 @@ Ext.define('PVE.qemu.HDInputPanel', {
             me.drive.format = values.diskformat;
         }
 
+        if (me.drive?.format === 'qcow2' || me.drive?.file?.endsWith('.qcow2')) {
+            if (values.cacheSizeBasedOnDisk) {
+                me.drive.cache_size = 'based-on-disk';
+            } else if (values.cacheSize) {
+                me.drive.cache_size = values.cacheSize;
+            } else {
+                delete me.drive.cache_size;
+            }
+        }
+
         PVE.Utils.propertyStringSet(me.drive, !values.backup, 'backup', '0');
         PVE.Utils.propertyStringSet(me.drive, values.noreplicate, 'replicate', 'no');
         PVE.Utils.propertyStringSet(me.drive, values.discard, 'discard', 'on');
@@ -157,6 +185,14 @@ Ext.define('PVE.qemu.HDInputPanel', {
         values.readOnly = PVE.Parser.parseBoolean(drive.ro);
         values.aio = drive.aio || '__default__';
 
+        if (drive.cache_size === 'based-on-disk') {
+            values.cacheSizeBasedOnDisk = true;
+            values.cacheSize = undefined;
+        } else {
+            values.cacheSizeBasedOnDisk = false;
+            values.cacheSize = drive.cache_size;
+        }
+
         values.mbps_rd = drive.mbps_rd;
         values.mbps_wr = drive.mbps_wr;
         values.iops_rd = drive.iops_rd;
@@ -167,6 +203,12 @@ Ext.define('PVE.qemu.HDInputPanel', {
         values.iops_wr_max = drive.iops_wr_max;
 
         me.setValues(values);
+
+        me.getViewModel().set(
+            'isQcow2',
+            values.diskformat === 'qcow2' || values.hdimage?.endsWith('.qcow2'),
+        );
+        this.getViewModel().set('cacheSizeBasedOnDisk', values.cacheSizeBasedOnDisk);
     },
 
     setNodename: function (nodename) {
@@ -355,6 +397,34 @@ Ext.define('PVE.qemu.HDInputPanel', {
                     disabled: '{!isVirtIO && !isSCSI}',
                 },
             },
+            {
+                xtype: 'numberfield',
+                name: 'cacheSize',
+                minValue: 1,
+                step: 1024 * 1024,
+                fieldLabel: gettext('Cache size'),
+                emptyText: gettext('Default cache size'),
+                bind: {
+                    disabled: '{!isQcow2 || cacheSizeBasedOnDisk}',
+                },
+                listeners: {
+                    disable: function (field) {
+                        console.log(field);
+                        field.setValue(null);
+                    },
+                },
+            },
+            {
+                xtype: 'proxmoxcheckbox',
+                name: 'cacheSizeBasedOnDisk',
+                reference: 'cacheSizeBasedOnDisk',
+                defaultValue: 0,
+                fieldLabel: gettext('Cache size based on disk'),
+                clearOnDisable: true,
+                bind: {
+                    disabled: '{!isQcow2}',
+                },
+            },
         );
 
         advancedColumn2.push(
-- 
2.47.3

Re: [PATCH pve-common 1/3] json schema: display nested oneOf error messages

[Fiona Ebner]

Nit: the title could mention that it fixes an existing issue, e.g.
"fix displaying ..."

Am 29.05.26 um 2:58 PM schrieb Erik Fastermann:
> Correctly display the error message for each oneOf case, not just the
> empty string.

Nit: you could elaborate slightly more, saying that it ended up being
the empty string, because the wrong key was used when doing the hash lookup.

For completeness, it could have a
Fixes: 9425834 ("json schema: implement 'oneOf' schema")
tag.

> 
> Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
> ---
>  src/PVE/JSONSchema.pm | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
> index 1997c70..fd10338 100644
> --- a/src/PVE/JSONSchema.pm
> +++ b/src/PVE/JSONSchema.pm
> @@ -1409,13 +1409,13 @@ sub check_prop {
>                  }
>  
>                  for my $inner_path (keys $inner_errors->%*) {

pre-existing, but we could add a sort here for consistency (in its own
patch)

> -                    add_error($collected_errors, $inner_path, $inner_errors->{$path});
> +                    add_error($collected_errors, $inner_path, $inner_errors->{$inner_path});
>                  }
>              }
>  
>              if (!$is_valid) {
>                  for my $inner_path (keys $collected_errors->%*) {

same here (in the same separate patch)

> -                    add_error($errors, $inner_path, $collected_errors->{$path});
> +                    add_error($errors, $inner_path, $collected_errors->{$inner_path});
>                  }
>              }
>          }

Re: [PATCH qemu-server 2/3] fix #1989: disk: add qcow2 cache options

[Fiona Ebner]

Am 29.05.26 um 2:58 PM schrieb Erik Fastermann:
> Add multiple options to configure the qcow2 L2/refcount cache. This
> enables huge performance gains in some cases.
> 
> For a detailed explanation of the options see the QEMU docs [0].
> Additionally the cache size can be configured based on the size of the
> disk image automatically.
> 
> Both blockdev and the older drive commandline are supported, which makes
> the feature accessible to all QEMU machine versions supported by PVE.
> Options which only apply to disk creation (cluster_size, refcount_bits)
> are not considered in this patch.
> 
> [0] https://gitlab.com/qemu-project/qemu/-/blob/master/docs/qcow2-cache.txt
> 
> Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
> ---
>  src/PVE/QemuServer.pm          | 33 ++++++++++++
>  src/PVE/QemuServer/Blockdev.pm | 33 ++++++++++++
>  src/PVE/QemuServer/Drive.pm    | 99 ++++++++++++++++++++++++++++++++++
>  3 files changed, 165 insertions(+)
> 
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 118f26bc..e53ae760 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -1294,6 +1294,39 @@ sub print_drive_commandline_full {
>          $opts .= ",auto-remove=on";
>      }
>  
> +    if (defined($drive->{cache_size})) {
> +        if ($drive->{cache_size} eq 'based-on-disk') {
> +            # Automatically set the cache-size based on the disk size. The formula
> +            # is adapted from here:
> +            # https://gitlab.com/qemu-project/qemu/-/blob/master/docs/qcow2-cache.txt#L97-98
> +            # Assumes the default values for cluster_size and refcount_bits and
> +            # that extended L2 entries are not used. As this combines the refcount
> +            # and L2 cache sizes, QEMU is free to choose a different value for each
> +            # of them, which means the L2 cache is not necessarily 4 times bigger
> +            # than the refcount cache.

For snapshot-as-volume-chain, for backed disks, we use extended L2
entries and 128k cluster size. Can we query the disk here to get the
actual params (would prepare for the future if we allow changing these
upon creation too, and it's in the context of startup so that should not
be too expensive)?

Same below for blockdev

> +            my $cache_size = $drive->{size} / 1024 / 1024 / 1024 * (131072 + 32768);
> +            $opts .= ",cache-size=" . int($cache_size);
> +        } else {
> +            $opts .= ",cache-size=" . int($drive->{cache_size});
> +        }
> +    }
> +
> +    if (defined($drive->{l2_cache_size})) {
> +        $opts .= ",cache-size=" . int($drive->{l2_cache_size});
> +    }
> +
> +    if (defined($drive->{l2_cache_entry_size})) {
> +        $opts .= ",l2-cache-entry-size=" . int($drive->{l2_cache_entry_size});
> +    }
> +
> +    if (defined($drive->{refcount_cache_size})) {
> +        $opts .= ",refcount-cache-size=" . int($drive->{refcount_cache_size});
> +    }
> +
> +    if (defined($drive->{cache_clean_interval})) {
> +        $opts .= ",cache-clean-interval=" . int($drive->{cache_clean_interval});
> +    }
> +
>      # my $file_param = $live_restore_name ? "file.file.filename" : "file";
>      my $file_param = "file";
>      if ($live_restore_name) {
> diff --git a/src/PVE/QemuServer/Blockdev.pm b/src/PVE/QemuServer/Blockdev.pm
> index 101c747c..7cb7dccb 100644
> --- a/src/PVE/QemuServer/Blockdev.pm
> +++ b/src/PVE/QemuServer/Blockdev.pm
> @@ -403,6 +403,39 @@ sub generate_format_blockdev {
>          $blockdev->{'discard-no-unref'} = JSON::true if $format eq 'qcow2';
>      }
>  
> +    if (defined($drive->{cache_size})) {
> +        if ($drive->{cache_size} eq 'based-on-disk') {
> +            # Automatically set the cache-size based on the disk size. The formula
> +            # is adapted from here:
> +            # https://gitlab.com/qemu-project/qemu/-/blob/master/docs/qcow2-cache.txt#L97-98
> +            # Assumes the default values for cluster_size and refcount_bits and
> +            # that extended L2 entries are not used. As this combines the refcount
> +            # and L2 cache sizes, QEMU is free to choose a different value for each
> +            # of them, which means the L2 cache is not necessarily 4 times bigger
> +            # than the refcount cache.
> +            my $cache_size = $drive->{size} / 1024 / 1024 / 1024 * (131072 + 32768);
> +            $blockdev->{'cache-size'} = int($cache_size);
> +        } else {
> +            $blockdev->{'cache-size'} = int($drive->{cache_size});
> +        }
> +    }
> +
> +    if (defined($drive->{l2_cache_size})) {
> +        $blockdev->{'l2-cache-size'} = int($drive->{l2_cache_size});
> +    }
> +
> +    if (defined($drive->{l2_cache_entry_size})) {
> +        $blockdev->{'l2-cache-entry-size'} = int($drive->{l2_cache_entry_size});
> +    }
> +
> +    if (defined($drive->{refcount_cache_size})) {
> +        $blockdev->{'refcount-cache-size'} = int($drive->{refcount_cache_size});
> +    }
> +
> +    if (defined($drive->{cache_clean_interval})) {
> +        $blockdev->{'cache-clean-interval'} = int($drive->{cache_clean_interval});
> +    }
> +
>      return $blockdev;
>  }
>  
> diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm
> index b80b7dbb..ff4be684 100644
> --- a/src/PVE/QemuServer/Drive.pm
> +++ b/src/PVE/QemuServer/Drive.pm
> @@ -256,6 +256,51 @@ my %drivedesc_base = (
>          optional => 1,
>          default => 0,
>      },
> +    cache_size => {

For new options, please use kebap-case. All options are qcow2-specific,
so I wonder if we should use 'qcow2-' as a prefix for the options.

> +        type => 'string',
> +        oneOf => [
> +            {
> +                type => 'integer',
> +                minimum => 1,
> +                description => 'Cache size in bytes',
> +                optional => 1,
> +            },
> +            {
> +                type => 'string',
> +                enum => ['based-on-disk'],
> +                description =>
> +                    'Automatically pick a cache size based on the configured disk size',
> +                optional => 1,
> +            },
> +        ],
> +        description => 'Cache size for qcow2 disks in bytes',
> +        typetext => '(<bytes> | based-on-disk)',
> +        optional => 1,
> +    },
> +    l2_cache_size => {
> +        type => 'integer',
> +        minimum => 1,
> +        description => 'L2 cache size for qcow2 disks in bytes',
> +        optional => 1,
> +    },
> +    l2_cache_entry_size => {
> +        type => 'integer',
> +        minimum => 512,
> +        description => 'L2 cache entry size for qcow2 disks in bytes',
> +        optional => 1,
> +    },
> +    refcount_cache_size => {
> +        type => 'integer',
> +        minimum => 1,
> +        description => 'Refcount cache size for qcow2 disks in bytes',
> +        optional => 1,
> +    },
> +    cache_clean_interval => {
> +        type => 'integer',
> +        minimum => 0,
> +        description => 'Cache clean interval for qcow2 disks in seconds',
> +        optional => 1,
> +    },
>  );
>  
>  my %iothread_fmt = (
> @@ -838,6 +883,60 @@ sub parse_drive {
>          }
>      }
>  
> +    my $is_qcow2 =
> +        (defined($res->{format}) && $res->{format} eq 'qcow2') || $res->{file} =~ /\.qcow2$/;

Please ask the storage layer what the format is. There is a
checked_volume_format() method for this.

> +
> +    my $any_qcow2_cache_option =
> +        defined($res->{cache_size})
> +        || defined($res->{l2_cache_size})
> +        || defined($res->{l2_cache_entry_size})
> +        || defined($res->{refcount_cache_size})
> +        || defined($res->{cache_clean_interval});
> +
> +    if ($any_qcow2_cache_option && !$is_qcow2) {
> +        warn "qcow2 cache options require disk format qcow2\n";

I think it might be better to iterate and explicitly mention the option
so the user knows right away which one it is. We should also mention the
drive ID and we should use log_warn() so the warning is more prominent.

> +        ++$error;

I'd prefer to just ignore the options if the format is wrong (with a
warning mentioning that this happens) rather than return an undef value
as a result later.

> +    }
> +
> +    my $cache_option_count =
> +        defined($res->{cache_size}) +
> +        defined($res->{l2_cache_size}) +
> +        defined($res->{refcount_cache_size});
> +
> +    if ($cache_option_count > 2) {
> +        warn "at most two of {,l2_,refcount_}cache_size can be set simultaneously\n";

Some users might not be familiar with the {,,} templating

> +        ++$error;
> +    }
> +
> +    sub is_power_of_two {

We usually don't declare such nested subs, but use a closure or separate
private helper sub.

> +        my ($n) = @_;
> +        return $n > 0 && (($n & ($n - 1)) == 0);
> +    }
> +
> +    if (defined($res->{l2_cache_entry_size}) && !is_power_of_two($res->{l2_cache_entry_size})) {
> +        warn "l2_cache_entry_size must be a power of two\n";
> +        ++$error;
> +    }
> +
> +    if (defined($res->{cache_size}) && $res->{cache_size} eq 'based-on-disk') {
> +        if (defined($res->{l2_cache_size}) || defined($res->{refcount_cache_size})) {
> +            warn "cache_size 'based-on-disk' not compatible with l2 or refcount cache size set\n";
> +            ++$error;
> +        }
> +    }
> +
> +    if (defined($res->{cache_size}) && $res->{cache_size} ne 'based-on-disk') {
> +        if (($res->{l2_cache_size} // 0) >= $res->{cache_size}) {
> +            warn "l2_cache_size is larger than or equal to cache_size\n";
> +            ++$error;
> +        }
> +
> +        if (($res->{refcount_cache_size} // 0) >= $res->{cache_size}) {
> +            warn "refcount_cache_size is larger than or equal to cache_size\n";
> +            ++$error;
> +        }
> +    }
> +
>      return if $error;
>  
>      return if $res->{mbps_rd} && $res->{mbps};

Re: [RFC common/manager/qemu-server 0/3] fix #1989: qemu: add qcow2 cache options

[Fiona Ebner]

Am 29.05.26 um 2:58 PM schrieb Erik Fastermann:
> Hi all,
> 
> I'm interested in feedback for my patches fixing #1989 [0], which add multiple
> options to configure the qcow2 L2/refcount cache. This enables huge performance
> gains in some cases.
> 
> I tested this for the current and older QEMU machine versions and while a VM was
> running. The add disk (vm creation and otherwise), import disk and edit disk
> dialogs were considered.
> 
> I also have the following questions:
> 
> - Currently I also check the file extension is qcow2, because in many cases the
>   disk format is currently not available (especially on the frontend). Is this
>   good enough in general?

In the backend, you can use the checked_volume_format() method. In the
front-end, I guess we don't have a better option than to check the
filename right now, but at least for non-third-party plugins it should
be good enough.

> - The cache size input on the frontend is currently in bytes, because fine
>   control might be required in some cases, but this is a little bit ugly.
>   Is there a better way to do this?

What cases do you have in mind? I feel like 1 MiB granularity should be
fine-grained enough in practice. If we really do want to cleanly support
different granularities, there is
https://bugzilla.proxmox.com/show_bug.cgi?id=3760 up for taking (with an
ex-colleague, Noel, having shared some semi-finished patches that would
have to be picked up and finished). But even if we go for that, I'm not
sure less MiB granularity is necessary for the front-end.

> 
> - Should the ability to control cluster_size and refcount_bits be included in
>   this patch or should a separate bug be created? There was some discussion
>   about this on Bugzilla [0].

The bug is mixing different things, yeah. We'd lose the existing
discussion related to the creation options if going for a new entry, so
maybe the best here would be to use "partially fix" in your subject and
update the bug title to what's still missing after the feature has landed.

> - Should minimum / maximum values be defined for all options and in what way?

I do think minimum values would be nice. Nobody complained about wanting
to lower the setting as far as I know, people care about performance
with this feature. The minimum should not be lower than QEMU's required
minimum and not impact disk performance too much. And if somebody
complains later, we can always go lower.

And for the maximum, maybe a few GiB?

I'm wondering if we should require VM.Config.Memory permission (in
addition to VM.Config.Disk) for these new options though.

> - Should we validate that the cache sizes are multiples of the cluster size /
>   cache entry size?

If that's a requirement, sure.

> - I'm using oneOf for the cache_size to support the 'based-on-disk' variant, but
>   this does not play nice with the generated Rust API types (currently set the
>   outer type to string, but this probably does not work). Should this be changed
>   or the Rust generation extended or can I use a custom enum on the Rust side?

If we plan on adding more oneOf schemas, then yes, better Rust support
would be good. I'm not familiar enough with the details here right now
though.

Question is, should we rather go for having an explicit value of 0 be
treated special as 'based-on-disk' instead of using the oneOf schema?
It's not very explicit though, so maybe not. Maybe have a secondary
boolean option to avoid the overloading, something like
proportional_cache_size=true|false, which conflicts with an explicitly
set value? Or rather than boolean, a multiplier? And only expose that
multiplier in the UI, since that should cover the majority of use cases
already? What do you think?

> - Should any extra documentation be added for this feature?

Documentation is always good. A (short) "cache size" subsection in
https://pve.proxmox.com/pve-docs/chapter-qm.html#qm_hard_disk would be nice.

I'll continue with the individual patches next week.