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

[Fiona Ebner <f.ebner@proxmox.com>]

Am 07.07.26 um 4:32 PM schrieb Lukas Sichert:
> 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 | 70 +++++++++++++++++++++++++-----------
>  1 file changed, 50 insertions(+), 20 deletions(-)
> 
> diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
> index a313ecc..a0feb5c 100644
> --- a/src/PVE/Storage/LVMPlugin.pm
> +++ b/src/PVE/Storage/LVMPlugin.pm
> @@ -7,6 +7,7 @@ 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::Tools qw(run_command file_read_firstline trim);
> @@ -327,13 +328,10 @@ my sub free_lvm_volumes_locked {
>                  '-t',
>                  "$throughput",
>              ];
> -            eval {
> -                run_command(
> -                    $cmd,
> -                    errmsg => "zero out finished (note: 'No space left on device' is ok here)",
> -                );
> -            };
> -            warn $@ if $@;
> +            run_command(
> +                $cmd,
> +                errmsg => "zero out finished (note: 'No space left on device' is ok here)",
> +            );

Removing the eval is not okay here. Otherwise a successful cleanup,
which is expected to fail with ENOSPC, will also be treated as a
failure. You would need to detect the kind of error (but it's not worth
it, since your following patches replace the 'dd' invocation anyways :)

>          } else {
>              # If the storage supports write_zeroes but stepsize is too big, reduce the stepsize to
>              # the maximum supported by the storage.
> @@ -345,8 +343,7 @@ my sub free_lvm_volumes_locked {
>              }
>  
>              my $cmd = ['blkdiscard', $lvmpath, '-v', '--zeroout', '--step', "${stepsize}"];
> -            eval { run_command($cmd); };
> -            warn $@ if $@;
> +            run_command($cmd);
>          }
>      };
>  
> @@ -368,18 +365,51 @@ 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); };
> +            if (my $cleanup_err = $@) {
> +                my $failed_name;
> +                $class->cluster_lock_storage(
> +                    $storeid,
> +                    $scfg->{shared},
> +                    undef,
> +                    sub {
> +                        my $lvs = lvm_list_volumes();
> +                        my $existing = $lvs->{$vg} // {};
> +
> +                        my $prefix = 'failed-';
> +                        my $suffix = "-del-$name";
> +
> +                        my $last_fail = max(
> +                            0,

Nit: we usually start counting with 0 for disk numbers, so this could
also be -1. But not really important, and both variants are fine by me.

> +                            map {
> +                                /^\Q$prefix\E(\d+)\Q$suffix\E$/ ? $1 : ()
> +                            } keys %$existing,
> +                        );
> +
> +                        $failed_name = 'failed-' . ($last_fail + 1) . $suffix;
> +
> +                        my $cmd = ['/sbin/lvrename', $vg, "del-$name", $failed_name];
> +                        run_command(
> +                            $cmd,
> +                            errmsg => "lvrename '$vg/del-$name' to '$vg/$failed_name' error",
> +                        );
> +                    },
> +                );

Maybe have a private helper function for the stuff in the if branch
here? It's not super long, but together with everything else (also from
the next patches) the free_lvm_volumes_locked() does become rather big.

>  
> -            $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: $cleanup_err\n";
> +
> +            } 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";
> +            }
>          }
>      };
>