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 B4AA91FF0E9 for ; Thu, 16 Jul 2026 11:37:10 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 27A2A21418; Thu, 16 Jul 2026 11:37:10 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 16 Jul 2026 11:37:07 +0200 Message-Id: From: =?utf-8?q?Michael_K=C3=B6ppl?= To: "Lukas Sichert" , Subject: Re: [PATCH storage v9 1/5] lvm: saferemove: keep LVs where zero-out failed for manual zero-out X-Mailer: aerc 0.21.0 References: <20260713160008.121125-1-l.sichert@proxmox.com> <20260713160008.121125-2-l.sichert@proxmox.com> In-Reply-To: <20260713160008.121125-2-l.sichert@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784194608054 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.342 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: FTZLVAEIUBLGIS2V6G3KFRD5AQ6YEVBQ X-Message-ID-Hash: FTZLVAEIUBLGIS2V6G3KFRD5AQ6YEVBQ X-MailFrom: m.koeppl@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 Mon Jul 13, 2026 at 6:00 PM CEST, Lukas Sichert wrote: [snip] > eval { > run_command( > $cmd, > @@ -345,8 +389,7 @@ my sub free_lvm_volumes_locked { > } > =20 > my $cmd =3D ['blkdiscard', $lvmpath, '-v', '--zeroout', '--s= tep', "${stepsize}"]; > - eval { run_command($cmd); }; > - warn $@ if $@; > + run_command($cmd); [marker] > } > }; > =20 > @@ -368,18 +411,33 @@ my sub free_lvm_volumes_locked { > errmsg =3D> "can't refresh LV '$lvmpath' to zero-out its= data", > ); > =20 > - $secure_delete_cmd->($lvmpath); > + eval { $secure_delete_cmd->($lvmpath); }; I haven't tested this (yet), but I think this is problematic. Since the call at [marker] dies, the if branch below would be entered, which would then die again, exiting the per-volume loop. If the cleanup of a single volume failed, wouldn't this result in all remaining volumes not being touched at all, leaving leftover volumes that have not been cleaned up and that are also not visible in the UI (I think?) because the regex in `list_images` does not match them. So you'd have volumes consuming space while being leftovers from a failed cleanup. Even though 3/5 structures this a bit differently, this problem still remains. Hope I'm not missing anything here. I think a fix could be to accumulate errors from cleanups (so all iterations) and then die once (?) > + if (my $cleanup_err =3D $@) { > + my $failed_name =3D > + eval { cleanup_failed_lvm_volume($class, $scfg, $sto= reid, $vg, $name) }; > + if (my $cleanup_failed_err =3D $@) { > + log_warn( > + "rename failed for '$vg/del-$name' to failed cle= anup volume:" > + . " $cleanup_failed_err", > + ); > + die "cleanup failed for lv $name: $cleanup_err\n"; > + } > =20 > - $class->cluster_lock_storage( > - $storeid, > - $scfg->{shared}, > - undef, > - sub { > - my $cmd =3D ['/sbin/lvremove', '-f', "$vg/del-$name"= ]; > - run_command($cmd, errmsg =3D> "lvremove '$vg/del-$na= me' error"); > - }, > - ); > - print "successfully removed volume $name ($vg/del-$name)\n"; > + die > + "cleanup failed for lv $name, renamed to '$vg/$faile= d_name': $cleanup_err\n"; [snip]