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

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

Am 13.07.26 um 6:00 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>

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

with two nits ^^

> @@ -279,6 +282,44 @@ sub lvm_list_volumes {
>      return $lvs;
>  }
>  
> +my sub cleanup_failed_lvm_volume {

Nit: The name could be a bit more telling about what it actually does,
maybe rename_after_failed_{cleanup,delete}()?

> @@ -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); };
> +            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:"

Nit: I'd go with something like "unable to rename '$vg/del-$name' after
failed cleanup:"
This makes it clear that the warning happens after the cleanup error
even though it shows up before that in the log.

> +                            . " $cleanup_failed_err",
> +                    );
> +                    die "cleanup failed for lv $name: $cleanup_err\n";
> +                }
>