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

[Lukas Sichert <l.sichert@proxmox.com>]

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 | 84 ++++++++++++++++++++++++++++++------
 1 file changed, 71 insertions(+), 13 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index a313ecc..478a90b 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -7,8 +7,11 @@ 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::SafeSyslog;
 use PVE::Tools qw(run_command file_read_firstline trim);
 
 use PVE::Storage::Common;
@@ -279,6 +282,44 @@ sub lvm_list_volumes {
     return $lvs;
 }
 
+my sub cleanup_failed_lvm_volume {
+    my ($class, $scfg, $storeid, $vg, $name) = @_;
+
+    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;
+
+            my $cmd = ['/sbin/lvrename', $vg, "del-$name", $failed_name];
+            run_command(
+                $cmd, errmsg => "lvrename '$vg/del-$name' to '$vg/$failed_name' error",
+            );
+        },
+    );
+
+    return $failed_name;
+}
+
 my sub free_lvm_volumes_locked {
     my ($class, $scfg, $storeid, $volnames) = @_;
 
@@ -327,6 +368,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,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);
         }
     };
 
@@ -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:"
+                            . " $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";
+
+            } 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";
+            }
         }
     };
 
-- 
2.47.3