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 CACE31FF0A7 for ; Mon, 17 Aug 2026 10:12:00 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id D8D7E238CF; Mon, 17 Aug 2026 10:11:57 +0200 (CEST) Message-ID: Date: Mon, 17 Aug 2026 10:11:43 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 qemu-server] cloud-init: query volume size via vdisk_list() to fix regression with qcow2 on LVM To: Dominik Csapak , pve-devel@lists.proxmox.com References: <20260812111316.85049-1-f.ebner@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1786954288872 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.242 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: QNNOIDDNSTBZ4BLZP3Z723S5G2UGXZS5 X-Message-ID-Hash: QNNOIDDNSTBZ4BLZP3Z723S5G2UGXZS5 X-MailFrom: f.ebner@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: 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. > > 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; >