[PATCH docs/manager/qemu-server v3 0/6] partially fix #1989: add QEMU qcow2 cache options

[PATCH docs/manager/qemu-server v3 0/6] partially fix #1989: add QEMU qcow2 cache options

[Erik Fastermann]

Add multiple options to configure the qcow2 L2/refcount cache. This
can provide significant performance gains in some cases, mainly for
large disks.

The pve-manager patch requires a qemu-server version that includes
patch 4, since older versions reject the new qcow2-* drive properties.
A v2 of the manager patch was applied and reverted (43b51f49) for that
reason.

Patches 1-3 are new and do not depend on the cache options:

1: re-run proxmox-perltidy, which reformats two calls in QemuServer.pm
   on current master. No functional change.
2: fix the bps, bps_rd and bps_wr throttle options, which were
   converted to 0 mbps (unlimited) instead of their actual value.
3: make the API report why parse_drive rejects a drive. Previously,
   the web UI only showed "unable to parse drive options" when one of
   the new qcow2 checks, or one of the existing checks, failed.

Changes since v2:

qemu-server (patch 4):
* Legacy -drive commandline (machine version < 10.0): prefix the
  options with "file." during a live restore, so they are set on the
  qcow2 node. Previously they ended up on the alloc-track node, which
  rejects them, so the live restore could not start.
* Handle the undefined format of empty CD-ROM drives. This caused an
  uninitialized value warning on every start and broke some cfg2cmd
  tests.
* Automatic sizing uses the virtual size reported by qemu-img info
  instead of the optional size= property. Without size=, the cache
  fell back to its minimum regardless of the disk size.
* Call qemu-img info with the qcow2 format and a 10 second timeout.
* Limit qcow2-cache-clean-interval to 2^32 - 1, since QEMU refuses to
  start with larger values, and describe the option in more detail.
* Report validation errors via the mechanism from patch 3 and use the
  full property names in the messages.
* The QEMU default L2 cache is up to 32 MiB, not always 32 MiB.
* Drop an unused log_warn import from Blockdev.pm.
* Rebase onto current master.

pve-manager (patch 5):
* Keep the underscore in the drive parser's key regex. Without it,
  disks with throttling options (mbps_rd, iops_wr_max, ...) could no
  longer be edited.
* Enabling automatic sizing removes qcow2-l2-cache-size and
  qcow2-refcount-cache-size if they were set via the API or CLI, since
  the backend rejects that combination.
* Parse the automatic sizing flag with PVE.Parser.parseBoolean like
  the neighboring fields.
* Tooltip: the QEMU default L2 cache is up to 32 MiB.
* Only accept whole numbers up to 10240 in the qcow2 cache size field,
  matching what the backend accepts.

pve-docs (patch 6):
* Name the options that are not exposed in the web UI and mention
  qm set.
* Note that QEMU allocates the cache in addition to the guest memory.
* Mention that the options mainly help with large disks.

Known limitations:

* Snapshots as volume chain: every qcow2 node of the backing chain
  gets the same cache options. With automatic sizing, every layer gets
  the cache size computed for the whole disk, so the worst-case memory
  use grows with the number of snapshots (for a 2 TiB disk with three
  snapshots, 4 x 320 MiB instead of 320 MiB), and qemu-img info runs
  once per layer. I would be inclined to leave it this way for now.

* Moving a disk to a storage that does not use qcow2 keeps the qcow2-*
  options, so the move and every later VM start log a warning for each
  of them. The options could be dropped silently on move instead, but
  the warning is also useful to have. The downside is that the warning
  cannot be cleared from the web UI, since the qcow2 fields are
  disabled for non-qcow2 disks, so the options can only be removed via
  the API or CLI.

* The disk dialog only exposes the total cache size. If
  qcow2-l2-cache-size or qcow2-refcount-cache-size were set via the
  API or CLI and are not smaller than the entered total, or both are
  set, saving fails. I think this is acceptable for now, and with patch
  3 the error message states which options conflict.

* With patch 3, problems for which parse_drive previously returned
  undef silently are now logged as warnings for callers that do not
  collect the errors. This includes the early returns for an invalid
  drive key and for a drive string that cannot be parsed. The warning
  adds useful information, so I would keep it.

Thanks to Fiona and Dominik for the feedback on the previous versions!


qemu-server:

Erik Fastermann (4):
  re-tidy source code
  drive: fix conversion of bps throttle limits to mbps
  api: report why drive options are rejected
  partially fix #1989: disk: add qcow2 cache options

 src/PVE/API2/Qemu.pm           |   9 +-
 src/PVE/QemuServer.pm          |  47 +++++-
 src/PVE/QemuServer/Blockdev.pm |   7 +
 src/PVE/QemuServer/Drive.pm    | 263 ++++++++++++++++++++++++++++++---
 4 files changed, 302 insertions(+), 24 deletions(-)


pve-manager:

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

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


pve-docs:

Erik Fastermann (1):
  partially fix #1989: qm: document qcow2 cache size config

 qm.adoc | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)


Summary over all repositories:
  7 files changed, 400 insertions(+), 25 deletions(-)

-- 
Generated by murpp 0.12.1

[PATCH qemu-server v3 1/6] re-tidy source code

[Erik Fastermann]

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

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 135c993f..d00f4e41 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -4973,7 +4973,12 @@ sub vmconfig_update_net {
                         $conf->{name},
                     );
                     PVE::Network::SDN::Vnets::add_next_free_cidr(
-                        $newnet->{bridge}, $conf->{name}, $newnet->{macaddr}, $vmid, undef, 1,
+                        $newnet->{bridge},
+                        $conf->{name},
+                        $newnet->{macaddr},
+                        $vmid,
+                        undef,
+                        1,
                     );
                 }
 
@@ -7912,7 +7917,14 @@ sub clone_disk {
             # No data needs to be copied for the cloudinit drive.
         } else {
             clone_disk_copy_data(
-                $storecfg, $source, $dest, $use_drive_mirror, $jobs, $completion, $qga, $bwlimit,
+                $storecfg,
+                $source,
+                $dest,
+                $use_drive_mirror,
+                $jobs,
+                $completion,
+                $qga,
+                $bwlimit,
             );
         }
     }
-- 
2.47.3

[PATCH qemu-server v3 2/6] drive: fix conversion of bps throttle limits to mbps

[Erik Fastermann]

When translating the bps, bps_rd and bps_wr options to their mbps
counterparts, parse_drive stored the result of defined() instead of
the value itself. Any limit set this way thus became 0 mbps, which
QEMU treats as unlimited, so the limit was silently ignored.

The web UI only uses the mbps options, so only users setting the bps
options via the API or CLI are affected.

Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
---
 src/PVE/QemuServer/Drive.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm
index 0b573717..bccf0371 100644
--- a/src/PVE/QemuServer/Drive.pm
+++ b/src/PVE/QemuServer/Drive.pm
@@ -867,7 +867,7 @@ sub parse_drive {
 
     my $error = 0;
     foreach my $opt (qw(bps bps_rd bps_wr)) {
-        if (my $bps = defined(delete $res->{$opt})) {
+        if (defined(my $bps = delete $res->{$opt})) {
             if (defined($res->{"m$opt"})) {
                 warn "both $opt and m$opt specified\n";
                 ++$error;
-- 
2.47.3

[PATCH qemu-server v3 3/6] api: report why drive options are rejected

[Erik Fastermann]

parse_drive only warned about invalid combinations of drive options
and then returned undef. During an API call these warnings end up in
the journal, so the web UI only showed the generic "unable to parse
drive options" error without any reason.

Collect the problems in parse_drive and hand them to callers that ask
for them, while all other callers keep getting warnings as before.
The drive parameter check for VM creation and config updates now uses
this, so the error states which options conflict.

Checks that previously failed without any message now report a reason
as well. This covers conflicting total and read/write limits, invalid
CD-ROM options, invalid sizes and errors in the drive string itself.
For callers that do not ask for the errors, these are now logged as
warnings instead of being silently ignored.

Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
---
 src/PVE/API2/Qemu.pm        |  9 +++--
 src/PVE/QemuServer/Drive.pm | 66 ++++++++++++++++++++++++++-----------
 2 files changed, 54 insertions(+), 21 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index cfbc1f51..740aec67 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -113,8 +113,13 @@ my $check_drive_param = sub {
     for my $opt (sort keys $param->%*) {
         next if !PVE::QemuServer::is_valid_drivename($opt);
 
-        my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
-        raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
+        my @drive_errors;
+        my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1, \@drive_errors);
+        if (!$drive) {
+            my $msg = 'unable to parse drive options';
+            $msg .= ': ' . join('; ', @drive_errors) if @drive_errors;
+            raise_param_exc({ $opt => $msg });
+        }
 
         if ($drive->{'import-from'}) {
             if ($drive->{file} !~ $PVE::QemuServer::Drive::NEW_DISK_RE || $3 != 0) {
diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm
index bccf0371..9af87ac7 100644
--- a/src/PVE/QemuServer/Drive.pm
+++ b/src/PVE/QemuServer/Drive.pm
@@ -843,16 +843,20 @@ sub parse_drive_interface {
 #        [,aio=native|threads][,discard=ignore|on][,detect_zeroes=on|off]
 #        [,iothread=on][,serial=serial][,model=model]
 
-sub parse_drive {
-    my ($key, $data, $with_alloc) = @_;
+my sub parse_drive_append_errors {
+    my ($key, $data, $with_alloc, $errors) = @_;
 
     my ($interface, $index) = eval { parse_drive_interface($key) };
-    return if $@;
+    if (my $err = $@) {
+        chomp $err;
+        push @$errors, $err;
+        return;
+    }
 
     my $desc_hash = $with_alloc ? $drivedesc_hash_with_alloc : $drivedesc_hash;
 
     if (!defined($desc_hash->{$key})) {
-        warn "invalid drive key: $key\n";
+        push @$errors, "invalid drive key: $key";
         return;
     }
 
@@ -861,16 +865,18 @@ sub parse_drive {
         my $pps_opts = { skip => $DROPPED_PROPERTIES };
         PVE::JSONSchema::parse_property_string($desc, $data, undef, undef, $pps_opts);
     };
-    return if !$res;
+    if (my $err = $@) {
+        chomp $err;
+        push @$errors, $err;
+        return;
+    }
     $res->{interface} = $interface;
     $res->{index} = $index;
 
-    my $error = 0;
     foreach my $opt (qw(bps bps_rd bps_wr)) {
         if (defined(my $bps = delete $res->{$opt})) {
             if (defined($res->{"m$opt"})) {
-                warn "both $opt and m$opt specified\n";
-                ++$error;
+                push @$errors, "both $opt and m$opt specified";
                 next;
             }
             $res->{"m$opt"} = sprintf("%.3f", $bps / (1024 * 1024.0));
@@ -894,25 +900,47 @@ sub parse_drive {
     ) {
         my ($option, $requires) = @$requirement;
         if ($res->{$option} && !$res->{$requires}) {
-            warn "$option requires $requires\n";
-            ++$error;
+            push @$errors, "$option requires $requires";
         }
     }
 
-    return if $error;
-
-    return if $res->{mbps_rd} && $res->{mbps};
-    return if $res->{mbps_wr} && $res->{mbps};
-    return if $res->{iops_rd} && $res->{iops};
-    return if $res->{iops_wr} && $res->{iops};
+    for my $mutually_exclusive_option (
+        ['mbps', 'mbps_rd'],
+        ['mbps', 'mbps_wr'],
+        ['iops', 'iops_rd'],
+        ['iops', 'iops_wr'],
+    ) {
+        my ($total, $directional) = @$mutually_exclusive_option;
+        push @$errors, "both $total and $directional specified"
+            if $res->{$total} && $res->{$directional};
+    }
 
     if ($res->{media} && ($res->{media} eq 'cdrom')) {
-        return if $res->{snapshot} || $res->{format};
-        return if $res->{interface} eq 'virtio';
+        push @$errors, "CD-ROM drives do not support 'snapshot' or 'format'"
+            if $res->{snapshot} || $res->{format};
+        push @$errors, "CD-ROM drives are not supported on VirtIO"
+            if $res->{interface} eq 'virtio';
     }
 
     if (my $size = $res->{size}) {
-        return if !defined($res->{size} = PVE::JSONSchema::parse_size($size));
+        push @$errors, "invalid size '$size'"
+            if !defined($res->{size} = PVE::JSONSchema::parse_size($size));
+    }
+
+    return if @$errors;
+    return $res;
+}
+
+sub parse_drive {
+    my ($key, $data, $with_alloc, $errors) = @_;
+
+    my @local_errors;
+    my $res = parse_drive_append_errors($key, $data, $with_alloc, \@local_errors);
+
+    if ($errors) {
+        push @$errors, @local_errors;
+    } else {
+        warn "$_\n" for @local_errors;
     }
 
     return $res;
-- 
2.47.3

[PATCH qemu-server v3 4/6] partially fix #1989: disk: add qcow2 cache options

[Erik Fastermann]

Add multiple options to configure the qcow2 L2/refcount cache. This
can provide significant 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          |  31 ++++++
 src/PVE/QemuServer/Blockdev.pm |   7 ++
 src/PVE/QemuServer/Drive.pm    | 195 +++++++++++++++++++++++++++++++++
 3 files changed, 233 insertions(+)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index d00f4e41..64639e45 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1295,6 +1295,14 @@ sub print_drive_commandline_full {
         $opts .= ",auto-remove=on";
     }
 
+    my $qcow2_opt_prefix = $live_restore_name ? 'file.' : '';
+    foreach my $option_value (
+        PVE::QemuServer::Drive::qcow2_cache_options($storecfg, $drive, $drive_id, $format)
+    ) {
+        my ($option, $value) = @$option_value;
+        $opts .= ",${qcow2_opt_prefix}$option=$value";
+    }
+
     # my $file_param = $live_restore_name ? "file.file.filename" : "file";
     my $file_param = "file";
     if ($live_restore_name) {
@@ -5081,6 +5089,29 @@ sub vmconfig_update_disk {
                     || safe_string_ne($drive->{ssd}, $old_drive->{ssd})
                     || safe_string_ne($drive->{vendor}, $old_drive->{vendor})
                     || safe_string_ne($drive->{ro}, $old_drive->{ro})
+                    || safe_num_ne(
+                        $drive->{'qcow2-cache-size'}, $old_drive->{'qcow2-cache-size'},
+                    )
+                    || safe_boolean_ne(
+                        $drive->{'qcow2-cache-size-based-on-disk'},
+                        $old_drive->{'qcow2-cache-size-based-on-disk'},
+                    )
+                    || safe_num_ne(
+                        $drive->{'qcow2-l2-cache-size'},
+                        $old_drive->{'qcow2-l2-cache-size'},
+                    )
+                    || safe_num_ne(
+                        $drive->{'qcow2-l2-cache-entry-size'},
+                        $old_drive->{'qcow2-l2-cache-entry-size'},
+                    )
+                    || safe_num_ne(
+                        $drive->{'qcow2-refcount-cache-size'},
+                        $old_drive->{'qcow2-refcount-cache-size'},
+                    )
+                    || safe_num_ne(
+                        $drive->{'qcow2-cache-clean-interval'},
+                        $old_drive->{'qcow2-cache-clean-interval'},
+                    )
                 ) {
                     die "skip\n";
                 }
diff --git a/src/PVE/QemuServer/Blockdev.pm b/src/PVE/QemuServer/Blockdev.pm
index c2989a37..77b003d6 100644
--- a/src/PVE/QemuServer/Blockdev.pm
+++ b/src/PVE/QemuServer/Blockdev.pm
@@ -403,6 +403,13 @@ sub generate_format_blockdev {
         $blockdev->{'discard-no-unref'} = JSON::true if $format eq 'qcow2';
     }
 
+    foreach my $option_value (
+        PVE::QemuServer::Drive::qcow2_cache_options($storecfg, $drive, $drive_id, $format)
+    ) {
+        my ($option, $value) = @$option_value;
+        $blockdev->{$option} = $value;
+    }
+
     return $blockdev;
 }
 
diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm
index 9af87ac7..210fd413 100644
--- a/src/PVE/QemuServer/Drive.pm
+++ b/src/PVE/QemuServer/Drive.pm
@@ -4,7 +4,9 @@ use strict;
 use warnings;
 
 use IO::File;
+use JSON qw(decode_json);
 use List::Util qw(first);
+use POSIX qw(ceil);
 use Storable qw(dclone);
 
 use PVE::JSONSchema qw(get_standard_option);
@@ -257,6 +259,63 @@ my %drivedesc_base = (
         optional => 1,
         default => 0,
     },
+    'qcow2-cache-size' => {
+        type => 'integer',
+        minimum => 1,
+        maximum => 10 * 1024,
+        description =>
+            'Cache size for qcow2 disks in MiB, defaulting up to 32 MiB for the L2 cache'
+            . ' and 4 clusters for the refcount cache (256 KiB with the default cluster size),'
+            . ' assuming qcow2-l2-cache-size and qcow2-refcount-cache-size are not set.'
+            . ' Only two of qcow2-cache-size, qcow2-l2-cache-size, and qcow2-refcount-cache-size'
+            . ' can be set simultaneously. This option is incompatible with'
+            . ' qcow2-cache-size-based-on-disk set to true.',
+        optional => 1,
+    },
+    'qcow2-cache-size-based-on-disk' => {
+        type => 'boolean',
+        description => 'Automatically pick a qcow2 cache size based on the disk image size.'
+            . ' With the default 64 KiB cluster size, 16 refcount bits and no extended'
+            . ' L2 entries this uses at most roughly 160 KiB of memory per GiB of disk.'
+            . ' This option is incompatible with qcow2-cache-size, qcow2-l2-cache-size,'
+            . ' or qcow2-refcount-cache-size being set.',
+        optional => 1,
+    },
+    'qcow2-l2-cache-size' => {
+        type => 'integer',
+        minimum => 1,
+        maximum => 10 * 1024,
+        description => 'L2 cache size for qcow2 disks in MiB. If qcow2-cache-size is set,'
+            . ' this value should be smaller. Only two of qcow2-cache-size, qcow2-l2-cache-size,'
+            . ' and qcow2-refcount-cache-size can be set simultaneously.',
+        optional => 1,
+    },
+    'qcow2-l2-cache-entry-size' => {
+        type => 'integer',
+        minimum => 512,
+        maximum => 2 * 1024 * 1024,
+        description => 'L2 cache entry size for qcow2 disks in bytes. Must be a power of two'
+            . ' and less than or equal to the cluster size.',
+        optional => 1,
+    },
+    'qcow2-refcount-cache-size' => {
+        type => 'integer',
+        minimum => 1,
+        maximum => 10 * 1024,
+        description =>
+            'Refcount cache size for qcow2 disks in MiB. If qcow2-cache-size is set,'
+            . ' this value should be smaller. Only two of qcow2-cache-size, qcow2-l2-cache-size,'
+            . ' and qcow2-refcount-cache-size can be set simultaneously.',
+        optional => 1,
+    },
+    'qcow2-cache-clean-interval' => {
+        type => 'integer',
+        minimum => 0,
+        maximum => 2**32 - 1,
+        description => 'Interval in seconds after which unused qcow2 cache entries are'
+            . ' freed. 0 disables cleaning. QEMU defaults to 600 seconds.',
+        optional => 1,
+    },
 );
 
 my %iothread_fmt = (
@@ -927,6 +986,48 @@ my sub parse_drive_append_errors {
             if !defined($res->{size} = PVE::JSONSchema::parse_size($size));
     }
 
+    my $cache_option_count =
+        defined($res->{'qcow2-cache-size'}) +
+        defined($res->{'qcow2-l2-cache-size'}) +
+        defined($res->{'qcow2-refcount-cache-size'});
+
+    if ($cache_option_count > 2) {
+        push @$errors,
+            "at most two of qcow2-cache-size, qcow2-l2-cache-size, qcow2-refcount-cache-size"
+            . " can be set simultaneously";
+    }
+
+    if (
+        defined($res->{'qcow2-l2-cache-entry-size'})
+        && !is_power_of_two($res->{'qcow2-l2-cache-entry-size'})
+    ) {
+        push @$errors, "qcow2-l2-cache-entry-size must be a power of two";
+    }
+
+    if (
+        defined($res->{'qcow2-cache-size-based-on-disk'})
+        && $res->{'qcow2-cache-size-based-on-disk'}
+    ) {
+        if (
+            defined($res->{'qcow2-cache-size'})
+            || defined($res->{'qcow2-l2-cache-size'})
+            || defined($res->{'qcow2-refcount-cache-size'})
+        ) {
+            push @$errors, "qcow2-cache-size-based-on-disk cannot be combined with"
+                . " qcow2-cache-size, qcow2-l2-cache-size or qcow2-refcount-cache-size";
+        }
+    }
+
+    if (defined($res->{'qcow2-cache-size'})) {
+        if (($res->{'qcow2-l2-cache-size'} // 0) >= $res->{'qcow2-cache-size'}) {
+            push @$errors, "qcow2-l2-cache-size must be smaller than qcow2-cache-size";
+        }
+
+        if (($res->{'qcow2-refcount-cache-size'} // 0) >= $res->{'qcow2-cache-size'}) {
+            push @$errors, "qcow2-refcount-cache-size must be smaller than qcow2-cache-size";
+        }
+    }
+
     return if @$errors;
     return $res;
 }
@@ -946,6 +1047,11 @@ sub parse_drive {
     return $res;
 }
 
+sub is_power_of_two {
+    my ($n) = @_;
+    return $n > 0 && (($n & ($n - 1)) == 0);
+}
+
 sub print_drive {
     my ($drive, $with_alloc) = @_;
     my $skip = ['index', 'interface'];
@@ -1268,4 +1374,93 @@ sub drive_qmp_peer {
     return drive_uses_qsd_fuse($storecfg, $drive) ? qsd_qmp_peer($vmid) : vm_qmp_peer($vmid);
 }
 
+my sub qcow2_cache_size_by_disk_size {
+    # The calculation is adapted from here:
+    # https://gitlab.com/qemu-project/qemu/-/blob/master/docs/qcow2-cache.txt
+    # 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 with the
+    # default cluster_size, refcount_bits and no extended L2 entries.
+
+    my ($storecfg, $drive) = @_;
+
+    my $disk_path = PVE::Storage::abs_filesystem_path($storecfg, $drive->{file}, 1);
+    my $img_json = PVE::Storage::Common::qemu_img_info($disk_path, 'qcow2', 10);
+    die "failed to query file information with qemu-img\n" if !$img_json;
+    my $img_info = eval { decode_json($img_json) };
+    die "could not parse qemu-img info command output for '$disk_path' - $@\n" if $@;
+
+    my $disk_size = $img_info->{'virtual-size'}
+        or die "unable to determine virtual size of '$disk_path'\n";
+    my $cluster_size = $img_info->{'cluster-size'} // 65536;
+    my $refcount_bits = $img_info->{'format-specific'}->{data}->{'refcount-bits'} // 16;
+    my $has_extended_l2 = $img_info->{'format-specific'}->{data}->{'extended-l2'} // 0;
+
+    my $l2_multiplier = $has_extended_l2 ? 16 : 8;
+    my $l2_cache_size = ($disk_size * $l2_multiplier) / $cluster_size;
+
+    $l2_cache_size = 2 * $cluster_size if $l2_cache_size < (2 * $cluster_size);
+    $l2_cache_size = ceil($l2_cache_size / $cluster_size) * $cluster_size;
+
+    my $refcount_cache_size = ($disk_size * $refcount_bits) / (8 * $cluster_size);
+
+    $refcount_cache_size = 4 * $cluster_size if $refcount_cache_size < (4 * $cluster_size);
+    $refcount_cache_size = ceil($refcount_cache_size / $cluster_size) * $cluster_size;
+
+    return $l2_cache_size + $refcount_cache_size;
+}
+
+sub qcow2_cache_options {
+    my ($storecfg, $drive, $drive_id, $format) = @_;
+
+    if (!defined($format) || $format ne 'qcow2') {
+        for my $qcow2_cache_option (
+            'qcow2-cache-size',
+            'qcow2-cache-size-based-on-disk',
+            'qcow2-l2-cache-size',
+            'qcow2-l2-cache-entry-size',
+            'qcow2-refcount-cache-size',
+            'qcow2-cache-clean-interval',
+        ) {
+            if (defined($drive->{$qcow2_cache_option})) {
+                log_warn("$drive_id: $qcow2_cache_option requires disk format qcow2");
+            }
+        }
+
+        return ();
+    }
+
+    my $options = [];
+
+    if (
+        defined($drive->{'qcow2-cache-size-based-on-disk'})
+        && $drive->{'qcow2-cache-size-based-on-disk'}
+    ) {
+        my $cache_size = qcow2_cache_size_by_disk_size($storecfg, $drive);
+        push @$options, ['cache-size', int($cache_size)];
+    } elsif (defined($drive->{'qcow2-cache-size'})) {
+        push @$options, ['cache-size', int($drive->{'qcow2-cache-size'}) * 1024 * 1024];
+    }
+
+    if (defined($drive->{'qcow2-l2-cache-size'})) {
+        push @$options, ['l2-cache-size', int($drive->{'qcow2-l2-cache-size'}) * 1024 * 1024];
+    }
+
+    if (defined($drive->{'qcow2-l2-cache-entry-size'})) {
+        push @$options, ['l2-cache-entry-size', int($drive->{'qcow2-l2-cache-entry-size'})];
+    }
+
+    if (defined($drive->{'qcow2-refcount-cache-size'})) {
+        push @$options, [
+            'refcount-cache-size', int($drive->{'qcow2-refcount-cache-size'}) * 1024 * 1024,
+        ];
+    }
+
+    if (defined($drive->{'qcow2-cache-clean-interval'})) {
+        push @$options, ['cache-clean-interval', int($drive->{'qcow2-cache-clean-interval'})];
+    }
+
+    return @$options;
+}
+
 1;
-- 
2.47.3

[PATCH pve-manager v3 5/6] partially fix #1989: ui: qemu: disk: add qcow2 cache size config

[Erik Fastermann]

Add the option to control the size of the qcow2 cache in the VM disk
create and edit dialogs. This can provide significant 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 | 81 +++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/www/manager6/Parser.js b/www/manager6/Parser.js
index 36df8e9d..cc9c6e9d 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..3592890f 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,
+            qcow2CacheSizeBasedOnDisk: false,
         },
     },
 
@@ -54,6 +56,15 @@ Ext.define('PVE.qemu.HDInputPanel', {
                     vm.set('isSCSISingle', value === 'virtio-scsi-single');
                 },
             },
+            'field[name=diskformat]': {
+                change: 'onDiskFormatChange',
+                // afterrender is needed for setting the initial value
+                afterrender: 'onDiskFormatChange',
+            },
+        },
+
+        onDiskFormatChange: function (field) {
+            this.getViewModel().set('isQcow2', field.getValue() === 'qcow2');
         },
 
         init: function (view) {
@@ -88,6 +99,24 @@ Ext.define('PVE.qemu.HDInputPanel', {
             me.drive.format = values.diskformat;
         }
 
+        if (me.drive?.format === 'qcow2' || me.drive?.file?.endsWith('.qcow2')) {
+            if (values.qcow2CacheSize) {
+                me.drive['qcow2-cache-size'] = values.qcow2CacheSize;
+            } else {
+                delete me.drive['qcow2-cache-size'];
+            }
+        }
+
+        PVE.Utils.propertyStringSet(
+            me.drive,
+            values.qcow2CacheSizeBasedOnDisk,
+            'qcow2-cache-size-based-on-disk',
+        );
+        if (values.qcow2CacheSizeBasedOnDisk) {
+            delete me.drive['qcow2-l2-cache-size'];
+            delete me.drive['qcow2-refcount-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');
@@ -156,6 +185,10 @@ Ext.define('PVE.qemu.HDInputPanel', {
         values.iothread = PVE.Parser.parseBoolean(drive.iothread);
         values.readOnly = PVE.Parser.parseBoolean(drive.ro);
         values.aio = drive.aio || '__default__';
+        values.qcow2CacheSizeBasedOnDisk = PVE.Parser.parseBoolean(
+            drive['qcow2-cache-size-based-on-disk'],
+        );
+        values.qcow2CacheSize = drive['qcow2-cache-size'];
 
         values.mbps_rd = drive.mbps_rd;
         values.mbps_wr = drive.mbps_wr;
@@ -167,6 +200,11 @@ 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'),
+        );
     },
 
     setNodename: function (nodename) {
@@ -355,6 +393,49 @@ Ext.define('PVE.qemu.HDInputPanel', {
                     disabled: '{!isVirtIO && !isSCSI}',
                 },
             },
+            {
+                xtype: 'numberfield',
+                name: 'qcow2CacheSize',
+                minValue: 1,
+                maxValue: 10 * 1024,
+                allowDecimals: false,
+                fieldLabel: gettext('qcow2 cache') + ' (MiB)',
+                emptyText: gettext('Default cache size'),
+                autoEl: {
+                    tag: 'div',
+                    'data-qtip': gettext(
+                        'Total size of the qcow2 cache in MiB. A larger cache can' +
+                            ' improve I/O performance on large images. Leave empty to' +
+                            ' use the QEMU default (up to 32 MiB L2 cache and 256 KiB' +
+                            ' refcount cache with the default disk settings).',
+                    ),
+                },
+                bind: {
+                    disabled: '{!isQcow2 || qcow2CacheSizeBasedOnDisk}',
+                },
+                listeners: {
+                    disable: (field) => field.setValue(null),
+                },
+            },
+            {
+                xtype: 'proxmoxcheckbox',
+                name: 'qcow2CacheSizeBasedOnDisk',
+                defaultValue: 0,
+                fieldLabel: gettext('qcow2 cache based on disk'),
+                clearOnDisable: true,
+                autoEl: {
+                    tag: 'div',
+                    'data-qtip': gettext(
+                        'Automatically size the qcow2 cache to cover the whole disk.' +
+                            ' Uses roughly 160 KiB of memory per GiB of disk with' +
+                            ' the default disk settings.',
+                    ),
+                },
+                bind: {
+                    value: '{qcow2CacheSizeBasedOnDisk}',
+                    disabled: '{!isQcow2}',
+                },
+            },
         );
 
         advancedColumn2.push(
-- 
2.47.3

[PATCH pve-docs v3 6/6] partially fix #1989: qm: document qcow2 cache size config

[Erik Fastermann]

Add a new subsection to document the new qcow2 cache config options.

Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
---
 qm.adoc | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/qm.adoc b/qm.adoc
index 18a4fbe..0c5d91d 100644
--- a/qm.adoc
+++ b/qm.adoc
@@ -350,6 +350,22 @@ benefit is reduced latency (hangs) in the guest for very I/O-intensive host
 workloads, since neither the main thread nor a vCPU thread can be blocked by
 disk I/O.
 
+[[qm_hard_disk_qcow2_cache]]
+Cache Size (qcow2)
+^^^^^^^^^^^^^^^^^^
+Fine-tuning the qcow2 L2/refcount cache behavior can provide significant
+performance gains for large disks. See the
+https://gitlab.com/qemu-project/qemu/-/blob/master/docs/qcow2-cache.txt[QEMU documentation]
+for a detailed explanation of these options. You can either set the cache size
+directly or let {pve} choose it automatically based on the disk image size. When
+sizing the cache automatically, with the default 64 KiB cluster size, 16
+refcount bits and no extended L2 entries, the cache uses at most roughly 160 KiB
+of memory per GiB of disk. The remaining options (`qcow2-l2-cache-size`,
+`qcow2-refcount-cache-size`, `qcow2-l2-cache-entry-size` and
+`qcow2-cache-clean-interval`) can be set with `qm set` or through the API. The
+cache is allocated by the QEMU process in addition to the memory assigned to the
+guest.
+
 [[qm_cpu]]
 CPU
 ~~~
-- 
2.47.3