From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id C71501FF0A7 for ; Mon, 17 Aug 2026 10:27:51 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 206AD238FE; Mon, 17 Aug 2026 10:27:51 +0200 (CEST) Message-ID: Date: Mon, 17 Aug 2026 10:27:45 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH v2 qemu-server] cloud-init: query volume size via vdisk_list() to fix regression with qcow2 on LVM To: Fiona Ebner , pve-devel@lists.proxmox.com References: <20260812111316.85049-1-f.ebner@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1786955245392 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.332 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_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust RDNS_NONE 1.274 Delivered to internal network by a host with no rDNS 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: EBAT3HMZWCPXJAJWVTYZ4HEBGRT3PJSZ X-Message-ID-Hash: EBAT3HMZWCPXJAJWVTYZ4HEBGRT3PJSZ 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: On 8/17/26 10:11 AM, Fiona Ebner wrote: > Am 17.08.26 um 8:59 AM schrieb Dominik Csapak: >> generally works for the issue that is described, but see my comment >> inline >> >> On 8/12/26 1:13 PM, Fiona Ebner wrote: >>> Since pve-storage commit 05032c5 ("fix #7811: storage: lvm: reject >>> allocation on format and volume name mismatch") and its follow-ups, >>> cloud-init disks are named with a '.qcow2' extension on LVM storages >>> with snapshot-as-volume-chain enabled. >>> >>> This causes a regression, because volume_size_info() fails and returns >>> undef when the qcow2 volume is not active. When the size cannot be >>> determined, commit_cloudinit_disk() function assumes that the disk >>> does not yet exist and tries to allocate new disk with the same name, >>> which fails. >>> >>>   # qm start 100 >>>   failed to stat '/dev/lvm/vm-100-cloudinit.qcow2' >>>     Rounding up size to full physical extent 8.00 MiB >>>   lvcreate 'lvm/vm-100-cloudinit.qcow2' error:   Logical Volume >>>    "vm-100-cloudinit.qcow2" already exists in volume group "lvm" >>> >>> Fix the issue by checking the existence/size with vdisk_list(), which >>> also works for deactivated volumes. >>> >>> Activating the volume earlier is an alternative, but would need to be >>> inside an eval block that silently ignores failure, since the volume >>> might not exist yet, which should not be logged. And in case where the >>> qcow2 on LVM volume does exist, but activation fails, there would be >>> an attempt to allocate a new volume with the same name, which also >>> seems less than ideal. So that approach is a bit hacky. >>> >>> Signed-off-by: Fiona Ebner >>> --- >>> >>> Changes in v2: >>> * don't include full shell prompt in commit message >>> * fix commit title >>> >>>   src/PVE/QemuServer/Cloudinit.pm | 13 ++++++++++++- >>>   1 file changed, 12 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/PVE/QemuServer/Cloudinit.pm b/src/PVE/QemuServer/ >>> Cloudinit.pm >>> index c1311da8..5af6b608 100644 >>> --- a/src/PVE/QemuServer/Cloudinit.pm >>> +++ b/src/PVE/QemuServer/Cloudinit.pm >>> @@ -39,7 +39,18 @@ sub commit_cloudinit_disk { >>>       my $scfg = PVE::Storage::storage_config($storecfg, $storeid); >>>       my $format = checked_volume_format($storecfg, $drive->{file}); >>>   -    my $size = eval { PVE::Storage::volume_size_info($storecfg, >>> $drive->{file}) }; >>> +    my $volumes = PVE::Storage::vdisk_list($storecfg, $storeid, >>> $vmid, [$drive->{file}], 'images'); >> >> for lvm this looks alright, but for other storages this is now a >> relatively large performance impact? >> >> vdisk_list calls list_images of the plugin, and for e.g. nfs/dir/etc. >> this calls 'file_size_info' for *all* found volumes (since that >> does not filter early for vollist) > > Good catch! This is a bit unfortunate. The reason is that the volid > looks different if it's a linked clone. > >> >> on nfs this could maybe be problematic with many qcow2 files? >> >> also, this is not eval'd, so any plugin that dies during list_images >> (e.g. because of permissions, or some other error condition) >> >> blocks the vm start now.. >> >> IMHO this should be in an eval and the storage plugins >> should filter early on vollist to avoid unnecessary work. >> > > I'm not fully convinced it should be eval'd, because if listing the > volume you are interested in fails, that's a good reason to fail. But > since some plugins don't properly just query the actual volume yet, I'll > just add it to be sure. > >> alternatively we could drop the vollist and just give the $vmid >> which filters early in most (?) storages. > > I do already specify the $vmid as well ;) But why drop vollist? If the > plugin respects it, it's better to have it. If the plugin doesn't, it > doesn't hurt. yes but e.g. the dir storage explicitly does not use vmid for skipping early if vollist is given: ``` next if !$vollist && defined($vmid) && ($owner ne $vmid); ``` > >> >> a third thing i noticed is that vdisk_list actives the storage, which >> volume_size_info does not (AFAICS). This is probably correct >> anyway but worth noting in the commit message IMO > > I can mention it. > >> >>> +    if (scalar($volumes->{$storeid}->@*) > 1) { >>> +        # just to be sure, since this is the first caller with >>> $vollist outside of the storage tests >>> +        print "bug: storage plugin for '$storeid' does not honor \ >>> $vollist for list_images()\n"; >>> +    } >>> +    my $size; >>> +    for my $volume_info ($volumes->{$storeid}->@*) { >>> +        next if $volume_info->{volid} ne $drive->{file}; >>> +        $size = $volume_info->{size} // $volume_info->{'approximate- >>> size'}; >>> +        last; >>> +    } >>> + >>>       if (!defined($size) || $size <= 0) { >>>           $volname =~ m/(vm-$vmid-cloudinit(.\Q$format\E)?)/; >>>           my $name = $1; >> >