From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 2CBEF1FF0E9 for ; Thu, 16 Jul 2026 10:25:21 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 7C83921419; Thu, 16 Jul 2026 10:25:20 +0200 (CEST) Message-ID: <42f032f0-4be2-4bc7-8f60-494d11a04b5f@proxmox.com> Date: Thu, 16 Jul 2026 10:25:13 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH qemu-server 1/3] partially fix #1989: disk: add qcow2 cache options To: Erik Fastermann , pve-devel@lists.proxmox.com References: <20260605153512.265703-1-e.fastermann@proxmox.com> <20260605153512.265703-2-e.fastermann@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20260605153512.265703-2-e.fastermann@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784190294708 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.305 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 2CHJBY6TW5KBBMFM4UA5TGMWJTG2345H X-Message-ID-Hash: 2CHJBY6TW5KBBMFM4UA5TGMWJTG2345H X-MailFrom: d.csapak@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: a few comments inline personally i would have liked the size=0 -> auto approach more than a separate option, but i guess this is personal preference... On 6/5/26 5:35 PM, Erik Fastermann wrote: > 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 > --- > src/PVE/QemuServer.pm | 71 ++++++++++++++++++- > src/PVE/QemuServer/Blockdev.pm | 44 ++++++++++++ > src/PVE/QemuServer/Drive.pm | 122 ++++++++++++++++++++++++++++++++ > src/PVE/QemuServer/QemuImage.pm | 24 +++++++ > 4 files changed, 260 insertions(+), 1 deletion(-) > > diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm > index 55e9f520..211927db 100644 > --- a/src/PVE/QemuServer.pm > +++ b/src/PVE/QemuServer.pm > @@ -78,6 +78,7 @@ use PVE::QemuServer::Drive qw( > parse_drive > print_drive > storage_allows_io_uring_default > + qcow2_cache_size_by_disk_size > ); > use PVE::QemuServer::DriveDevice qw(print_drivedevice_full scsihw_infos); > use PVE::QemuServer::Machine; > @@ -1294,6 +1295,49 @@ sub print_drive_commandline_full { > $opts .= ",auto-remove=on"; > } > > + for my $qcow2_cache_option ( > + 'qcow2-cache-size', > + 'qcow2-l2-cache-size', > + 'qcow2-l2-cache-entry-size', > + 'qcow2-refcount-cache-size', > + 'qcow2-cache-clean-interval', what about the auto-size option? > + ) { > + if (defined($drive->{$qcow2_cache_option}) && $format ne 'qcow2') { > + log_warn("$drive_id: $qcow2_cache_option requires disk format qcow2"); > + } > + } nit: this could avoid some checks by pulling the format comparison out: if ($format ne 'qcow2') { # log warning loop here } else { # assignments here } > + > + if ($format eq 'qcow2') { > + 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); > + $opts .= ",cache-size=" . int($cache_size); > + } > + > + if (defined($drive->{'qcow2-cache-size'})) { > + $opts .= ",cache-size=" . int($drive->{'qcow2-cache-size'}) * 1024 * 1024; > + } > + > + if (defined($drive->{'qcow2-l2-cache-size'})) { > + $opts .= ",l2-cache-size=" . int($drive->{'qcow2-l2-cache-size'}) * 1024 * 1024; > + } > + > + if (defined($drive->{'qcow2-l2-cache-entry-size'})) { > + $opts .= ",l2-cache-entry-size=" . int($drive->{'qcow2-l2-cache-entry-size'}); > + } > + > + if (defined($drive->{'qcow2-refcount-cache-size'})) { > + $opts .= > + ",refcount-cache-size=" . int($drive->{'qcow2-refcount-cache-size'}) * 1024 * 1024; > + } > + > + if (defined($drive->{'qcow2-cache-clean-interval'})) { > + $opts .= ",cache-clean-interval=" . int($drive->{'qcow2-cache-clean-interval'}); > + } > + } > + > # my $file_param = $live_restore_name ? "file.file.filename" : "file"; > my $file_param = "file"; > if ($live_restore_name) { > @@ -5217,6 +5266,26 @@ 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_string_ne( > + $drive->{'qcow2-cache-size'}, > + $old_drive->{'qcow2-cache-size'}, > + ) > + || 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'}, > + ) what about the 'cache-size-based-on-disk' option here? even if it's not necessary to check, some kind of comment would be good to have so we know it was not accidentally missed > ) { > die "skip\n"; > } > diff --git a/src/PVE/QemuServer/Blockdev.pm b/src/PVE/QemuServer/Blockdev.pm > index 101c747c..0dd5067d 100644 > --- a/src/PVE/QemuServer/Blockdev.pm > +++ b/src/PVE/QemuServer/Blockdev.pm > @@ -9,6 +9,7 @@ use File::stat; > use JSON; > > use PVE::JSONSchema qw(json_bool); > +use PVE::RESTEnvironment qw(log_warn); > use PVE::Storage; > > use PVE::QemuServer::Drive qw(drive_is_cdrom); > @@ -403,6 +404,49 @@ sub generate_format_blockdev { > $blockdev->{'discard-no-unref'} = JSON::true if $format eq 'qcow2'; > } > > + for my $qcow2_cache_option ( > + 'qcow2-cache-size', > + 'qcow2-l2-cache-size', > + 'qcow2-l2-cache-entry-size', > + 'qcow2-refcount-cache-size', > + 'qcow2-cache-clean-interval', > + ) { > + if (defined($drive->{$qcow2_cache_option}) && $format ne 'qcow2') { > + log_warn("$drive_id: $qcow2_cache_option requires disk format qcow2"); > + } > + } > + > + if ($format eq 'qcow2') { > + 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); > + $blockdev->{'cache-size'} = int($cache_size); > + } > + > + if (defined($drive->{'qcow2-cache-size'})) { > + $blockdev->{'cache-size'} = int($drive->{'qcow2-cache-size'}) * 1024 * 1024; > + } > + > + if (defined($drive->{'qcow2-l2-cache-size'})) { > + $blockdev->{'l2-cache-size'} = int($drive->{'qcow2-l2-cache-size'}) * 1024 * 1024; > + } > + > + if (defined($drive->{'qcow2-l2-cache-entry-size'})) { > + $blockdev->{'l2-cache-entry-size'} = int($drive->{'qcow2-l2-cache-entry-size'}); > + } > + > + if (defined($drive->{'qcow2-refcount-cache-size'})) { > + $blockdev->{'refcount-cache-size'} = > + int($drive->{'qcow2-refcount-cache-size'}) * 1024 * 1024; > + } > + > + if (defined($drive->{'qcow2-cache-clean-interval'})) { > + $blockdev->{'cache-clean-interval'} = int($drive->{'qcow2-cache-clean-interval'}); > + } > + } > + these checks/calculations are the same as above, just the assignment is different. Maybe we can factor that out and reuse it? > return $blockdev; > } > > diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm > index b80b7dbb..e123947e 100644 > --- a/src/PVE/QemuServer/Drive.pm > +++ b/src/PVE/QemuServer/Drive.pm > @@ -7,6 +7,7 @@ use Storable qw(dclone); > > use IO::File; > use List::Util qw(first); > +use POSIX; > > use PVE::RESTEnvironment qw(log_warn); > use PVE::Storage; > @@ -26,6 +27,7 @@ our @EXPORT_OK = qw( > parse_drive > print_drive > storage_allows_io_uring_default > + qcow2_cache_size_by_disk_size > ); > > my $DROPPED_PROPERTIES = ['cyls', 'heads', 'secs', 'trans']; > @@ -256,6 +258,46 @@ 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', > + optional => 1, i think it would be good to document the default qemu uses (if that does not change too often), so a user can get a hint what that is without having to read qemu documentation > + }, > + 'qcow2-cache-size-based-on-disk' => { > + type => 'boolean', > + description => > + 'Automatically pick a qcow2 cache size based on the configured disk size', > + optional => 1, > + }, > + 'qcow2-l2-cache-size' => { > + type => 'integer', > + minimum => 1, > + maximum => 10 * 1024, > + description => 'L2 cache size for qcow2 disks in MiB', > + 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', > + optional => 1, > + }, > + 'qcow2-refcount-cache-size' => { > + type => 'integer', > + minimum => 1, > + maximum => 10 * 1024, > + description => 'Refcount cache size for qcow2 disks in MiB', > + optional => 1, > + }, > + 'qcow2-cache-clean-interval' => { > + type => 'integer', > + minimum => 0, > + description => 'Cache clean interval for qcow2 disks in seconds', > + optional => 1, > + }, > ); > > my %iothread_fmt = ( > @@ -838,6 +880,52 @@ sub parse_drive { > } > } > > + 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) { > + warn "at most two of qcow2-cache-size, qcow2-l2-cache-size, qcow2-refcount-cache-size" > + . " can be set simultaneously\n"; > + ++$error; > + } i guess this behavior (that cache-size is the overall limit of l2 and refcount cache) should be documented on our side (e.g. api description) > + > + if ( > + defined($res->{'qcow2-l2-cache-entry-size'}) > + && !is_power_of_two($res->{'qcow2-l2-cache-entry-size'}) > + ) { > + warn "qcow2-l2-cache-entry-size must be a power of two\n"; > + ++$error; > + } > + same for this, e.g. an additional sentence in the description: "(Must be a power of two)" would be nice > + 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'}) > + ) { > + warn "qcow2-cache-size-based-on-disk is not compatible with cache-size," > + . " l2-cache-size or refcount-cache-size being set\n"; > + ++$error; > + } > + } same with this > + > + if (defined($res->{'qcow2-cache-size'})) { > + if (($res->{'qcow2-l2-cache-size'} // 0) >= $res->{'qcow2-cache-size'}) { > + warn "qcow2-l2-cache-size is larger than or equal to qcow2-cache-size\n"; > + ++$error; > + } > + > + if (($res->{'qcow2-refcount-cache-size'} // 0) >= $res->{'qcow2-cache-size'}) { > + warn "qcow2-refcount-cache-size is larger than or equal to qcow2-cache-size\n"; > + ++$error; > + } > + } > + and this. Basically the api description should give a good hint on what values to use. As a user, it's always cumbersome to have to test which values are allowed by experimenting. if the api description states the limits and constraints, it's much better (IMO)." > return if $error; > > return if $res->{mbps_rd} && $res->{mbps}; > @@ -857,6 +945,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']; > @@ -1179,4 +1272,33 @@ sub drive_qmp_peer { > return drive_uses_qsd_fuse($storecfg, $drive) ? qsd_qmp_peer($vmid) : vm_qmp_peer($vmid); > } > > +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#L97-98 > + # 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 $img_info = PVE::QemuServer::QemuImage::info($storecfg, $drive->{file}); > + 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 = ($drive->{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 = ($drive->{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; > +} > + > 1; > diff --git a/src/PVE/QemuServer/QemuImage.pm b/src/PVE/QemuServer/QemuImage.pm > index 71be3abb..9c9db4e8 100644 > --- a/src/PVE/QemuServer/QemuImage.pm > +++ b/src/PVE/QemuServer/QemuImage.pm > @@ -15,6 +15,30 @@ use PVE::QemuServer::Blockdev; > use PVE::QemuServer::Drive qw(checked_volume_format); > use PVE::QemuServer::Helpers; > > +sub info { > + my ($storecfg, $volid) = @_; > + > + my $disk_path = PVE::Storage::abs_filesystem_path($storecfg, $volid, 1); > + my $cmd = ['/usr/bin/qemu-img', 'info', '--output=json', $disk_path]; > + > + my $res = eval { > + my $output = ''; > + PVE::Tools::run_command( > + $cmd, > + outfunc => sub { > + my $line = shift; > + $output .= $line; > + }, > + ); > + decode_json($output); > + }; > + > + my $err = $@; > + die "qemu-img info failed: $err" if $err; > + > + return $res; > +} > + there is already `PVE::Storage::Common::qemu_img_info`, please reuse that. > sub convert_iscsi_path { > my ($path) = @_; >