Re: [PATCH storage v9 1/5] lvm: saferemove: keep LVs where zero-out failed for manual zero-out

["Michael Köppl" <m.koeppl@proxmox.com>]

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 {
>              }
>  
>              my $cmd = ['blkdiscard', $lvmpath, '-v', '--zeroout', '--step', "${stepsize}"];
> -            eval { run_command($cmd); };
> -            warn $@ if $@;
> +            run_command($cmd);

[marker]

>          }
>      };
>  
> @@ -368,18 +411,33 @@ my sub free_lvm_volumes_locked {
>                  errmsg => "can't refresh LV '$lvmpath' to zero-out its data",
>              );
>  
> -            $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 = $@) {
> +                my $failed_name =
> +                    eval { cleanup_failed_lvm_volume($class, $scfg, $storeid, $vg, $name) };
> +                if (my $cleanup_failed_err = $@) {
> +                    log_warn(
> +                        "rename failed for '$vg/del-$name' to failed cleanup volume:"
> +                            . " $cleanup_failed_err",
> +                    );
> +                    die "cleanup failed for lv $name: $cleanup_err\n";
> +                }
>  
> -            $class->cluster_lock_storage(
> -                $storeid,
> -                $scfg->{shared},
> -                undef,
> -                sub {
> -                    my $cmd = ['/sbin/lvremove', '-f', "$vg/del-$name"];
> -                    run_command($cmd, errmsg => "lvremove '$vg/del-$name' error");
> -                },
> -            );
> -            print "successfully removed volume $name ($vg/del-$name)\n";
> +                die
> +                    "cleanup failed for lv $name, renamed to '$vg/$failed_name': $cleanup_err\n";

[snip]