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

[David Riley <d.riley@proxmox.com>]

On 7/21/26 2:37 PM, Lukas Sichert wrote:
> Currently even if 'zeroout' fails, the LV is removed and can't be zeroed
> out manually later.
>
> Let zeroing errors propagate from the secure delete command, and rename
> failed removals to a 'failed-<N>-del-*' LV name instead of immediately
> removing them.
>
> Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
> ---
>   src/PVE/Storage/LVMPlugin.pm | 119 ++++++++++++++++++++++++++++-------
>   1 file changed, 95 insertions(+), 24 deletions(-)
>
> diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
> index a313ecc..4734e11 100644
> --- a/src/PVE/Storage/LVMPlugin.pm
> +++ b/src/PVE/Storage/LVMPlugin.pm
> @@ -7,8 +7,10 @@ use Cwd qw(abs_path);
>   use File::Basename;
>   use IO::File;
>   use JSON;
> +use List::Util qw(max);
>   
>   use PVE::JSONSchema qw(get_standard_option);
> +use PVE::RESTEnvironment qw(log_warn);
>   use PVE::Tools qw(run_command file_read_firstline trim);
>   
>   use PVE::Storage::Common;
> @@ -279,6 +281,49 @@ sub lvm_list_volumes {
>       return $lvs;
>   }
>   
> +my sub rename_after_failed_cleanup {
> +    my ($class, $scfg, $storeid, $vg, $name) = @_;
> +
> +    eval {
> +        my $failed_name;
> +        $class->cluster_lock_storage(
> +            $storeid,
> +            $scfg->{shared},
> +            undef,
> +            sub {
> +                my $vgs = lvm_vgs();
> +                die "volume group '$vg' not found\n"
> +                    if !defined($vgs->{$vg});
> +
> +                my $lvs = lvm_list_volumes($vg);
> +                my $existing = $lvs->{$vg} // {};
> +
> +                my $prefix = 'failed-';
> +                my $suffix = "-del-$name";
> +
> +                my $last_fail = max(
> +                    -1,
> +                    map {
> +                        /^\Q$prefix\E(\d+)\Q$suffix\E$/ ? $1 : ()
> +                    } keys %$existing,
> +                );
> +
> +                $failed_name = 'failed-' . ($last_fail + 1) . $suffix;

nit: You could use the $prefix for the failed_name here.

> +
> +                my $cmd = ['/sbin/lvrename', $vg, "del-$name", $failed_name];
> +                run_command(
> +                    $cmd,
> +                    errmsg => "lvrename '$vg/del-$name' to '$vg/$failed_name' error",
> +                );
> +                print "renamed '$vg/del-$name' to '$vg/$failed_name'\n";
> +            },
> +        );
> +    };
> +    if (my $rename_err = $@) {
> +        print STDERR "ERROR: unable to rename '$vg/del-$name': $rename_err";
> +    }
> +}
> +
>   my sub free_lvm_volumes_locked {
>       my ($class, $scfg, $storeid, $volnames) = @_;
>   
> @@ -327,6 +372,9 @@ my sub free_lvm_volumes_locked {
>                   '-t',
>                   "$throughput",
>               ];
> +            # FIXME: handle cstream's expected ENOSPC failure explicitly and let other
> +            # errors propagate. For now, preserve the old behavior where cstream can
> +            # fail successfully with ENOSPC after writing until the device is full.
>               eval {
>                   run_command(
>                       $cmd,
> @@ -345,41 +393,64 @@ my sub free_lvm_volumes_locked {
>               }
>   
>               my $cmd = ['blkdiscard', $lvmpath, '-v', '--zeroout', '--step', "${stepsize}"];
> -            eval { run_command($cmd); };
> -            warn $@ if $@;
> +            run_command($cmd);
>           }
>       };
>   
>       # we need to zero out LVM data for security reasons
>       # and to allow thin provisioning
>       my $zero_out_worker = sub {
> +
> +        my $total_cleanup_errors = 0;
>           for my $name (@$volnames) {
>               my $lvmpath = "/dev/$vg/del-$name";
>               print "zero-out data on image $name ($lvmpath)\n";
>   
> -            my $cmd_activate = ['/sbin/lvchange', '-aly', $lvmpath];
> -            run_command(
> -                $cmd_activate,
> -                errmsg => "can't activate LV '$lvmpath' to zero-out its data",
> -            );
> -            $cmd_activate = ['/sbin/lvchange', '--refresh', $lvmpath];
> -            run_command(
> -                $cmd_activate,
> -                errmsg => "can't refresh LV '$lvmpath' to zero-out its data",
> -            );
> -
> -            $secure_delete_cmd->($lvmpath);
> +            eval {
> +                # pass an errfunc here so that debug information is not by lvm to stderr,
> +                # but by the print STDERR below with additional information
> +                my $cmd_activate = ['/sbin/lvchange', '-aly', $lvmpath];
> +                run_command(
> +                    $cmd_activate,
> +                    errmsg => "can't activate LV '$lvmpath' to zero-out its data",
> +                    errfunc => sub { },
> +                );
> +                $cmd_activate = ['/sbin/lvchange', '--refresh', $lvmpath];
> +                run_command(
> +                    $cmd_activate,
> +                    errmsg => "can't refresh LV '$lvmpath' to zero-out its data",
> +                    errfunc => sub { },
> +                );
> +            };
> +            if (my $activation_err = $@) {
> +                print STDERR "ERROR: $activation_err";
> +                eval { rename_after_failed_cleanup($class, $scfg, $storeid, $vg, $name) };
> +                $total_cleanup_errors += 1;
> +                next;
> +            }
>   
> -            $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";
> +            eval { $secure_delete_cmd->($lvmpath); };
> +            if (my $cleanup_err = $@) {
> +                print STDERR "ERROR: cleanup failed for lv $name: $cleanup_err";
> +                eval { rename_after_failed_cleanup($class, $scfg, $storeid, $vg, $name) };
> +                $total_cleanup_errors += 1;
> +                next;
> +            } else {
> +                $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";
> +            }
> +        }
> +        if ($total_cleanup_errors != 0) {
> +            my $number_of_vols = scalar @$volnames;
> +            die "cleanup failed for $total_cleanup_errors out of $number_of_vols volumes\n";
>           }
>       };
>