From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 7A6371FF13B for ; Wed, 06 May 2026 12:11:48 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2559F1C6F6; Wed, 6 May 2026 12:11:46 +0200 (CEST) Message-ID: <8cb01407-870d-4166-9f27-7fc2fe0ae4a6@proxmox.com> Date: Wed, 6 May 2026 12:11:09 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH storage v3 1/2] fix #7339: lvmthick: add worker to free space of to be deleted VMs To: Lukas Sichert , pve-devel@lists.proxmox.com References: <20260423144721.54451-1-l.sichert@proxmox.com> <20260423144721.54451-2-l.sichert@proxmox.com> <24d217bf-5b9f-48d2-8754-9614bbbc5484@proxmox.com> Content-Language: en-US From: Friedrich Weber In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1778062162489 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.013 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [storage.pm] Message-ID-Hash: GHOE4YOJS2L7KRSW7IUCOFRIVKPHXVV6 X-Message-ID-Hash: GHOE4YOJS2L7KRSW7IUCOFRIVKPHXVV6 X-MailFrom: f.weber@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 06/05/2026 11:57, Lukas Sichert wrote: > On 2026-05-05 17:59, Friedrich Weber wrote: > >>> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm >>> index 6e87bac..ef1596f 100755 >>> --- a/src/PVE/Storage.pm >>> +++ b/src/PVE/Storage.pm >>> @@ -1192,7 +1192,7 @@ sub vdisk_free { >>> >>> activate_storage($cfg, $storeid); >>> >>> - my $cleanup_worker; >>> + my $discard_worker; >>> >>> # lock shared storage >>> $plugin->cluster_lock_storage( >>> @@ -1206,16 +1206,24 @@ sub vdisk_free { >>> >>> my (undef, undef, undef, undef, undef, $isBase, $format) = >>> $plugin->parse_volname($volname); >>> - $cleanup_worker = $plugin->free_image($storeid, $scfg, $volname, $isBase, $format); >>> + $discard_worker = $plugin->free_image($storeid, $scfg, $volname, $isBase, $format); >>> }, >>> ); >>> >>> - return if !$cleanup_worker; >>> + return if !$discard_worker; >>> >>> my $rpcenv = PVE::RPCEnvironment::get(); >>> my $authuser = $rpcenv->get_user(); >>> >>> - $rpcenv->fork_worker('imgdel', undef, $authuser, $cleanup_worker); >>> + if ($scfg->{saferemove} && $scfg->{'issue-blkdiscard'}) { >>> + $rpcenv->fork_worker('diskzerodiscard', undef, $authuser, $discard_worker); >>> + } elsif ($scfg->{saferemove}) { >>> + $rpcenv->fork_worker('diskzero', undef, $authuser, $discard_worker); >>> + } elsif ($scfg->{'issue-blkdiscard'}) { >>> + $rpcenv->fork_worker('diskdiscard', undef, $authuser, $discard_worker); >>> + } else { >>> + $rpcenv->fork_worker('imgdel', undef, $authuser, $discard_worker); >>> + } >>> } >> >> Is this change necessary? To me it seems like the fairly general >> PVE::Storage::vdisk_free shouldn't check for plugin-specific config >> options (like saferemove/issue-blkdiscard) -- in a way, it breaks the >> abstraction provided by PVE::Storage. Wouldn't it be nicer to keep >> PVE::Storage::vdisk_free as-is, have it spawn the dedicated 'imgdel' >> task, and the 'imgdel' task returned by the LVM plugin then >> zeroouts+discards/only-zeroouts/only-discards depending on the LVM >> storage settings? > > This patch is there to improve the worker description displayed in the > task log. An 'imgdel' worker is currently displayed as an 'Erase data' > task. This can be misleading if 'saferemove' is not enabled, as no data > is actually erased. With these changes, the description for > 'saferemove' enabled becomes 'Zero out disk', for 'issue-blkdiscard' > enabled it becomes 'Discard disk', and for both enabled it becomes > 'Zero out and discard disk'. OK, good point, I don't find 'Erase data' so bad even if the worker only discards, but I see there could be confusion potential. > That said, this is also suboptimal because if other plugins return > worker tasks from their '$free_image' implementations, then their > descriptions could also be altered if these flags are set. In the > default Proxmox stack this is currently not the case, but it could > affect external plugins. > > Another option would be to rename the displayed name for an 'imgdel' > task in the UI to 'Destroy image'. That would fit the generic worker > task better and would also make the naming consistent with > 'unknownimgdel', which is currently displayed as > 'Destroy image from unknown guest'. Yeah, changing the GUI description of 'imgdel' like that could be an option! Though of course there is also some confusion potential there, especially for users who upgrade. What do others think? > > Does anybody have opinions on how to go forward with this? > Thanks in advance!