[PATCH docs/manager/storage v8 0/5] fix #7339: lvmthick: add option to free storage for deleted VMs

[PATCH docs/manager/storage v8 0/5] fix #7339: lvmthick: add option to free storage for deleted VMs

[Lukas Sichert]

Logical volumes (LV) in an LVM (thick) volume group (VG) are
thick-provisioned, but the underlying backing storage can be
thin-provisioned. In particular, this can be the case if the VG resides
on a LUN provided by a SAN via iSCSI/FC/SAS [1], where the LUN may be
thin-provisioned on the SAN side.

In such setups, one usually wants that deleting an LV (e.g. VM disk)
frees up space on the SAN side, especially when using
snapshots-as-volume-chains, because snapshot LVs are thick-provisioned
LVs from the LVM point of view, so users may want to over-provision the
LUN on the SAN side.

One option to free up space when deleting an LV is to set
`issue_discards = 1` in the LVM config. With this setting, `lvremove`
will send discards for the regions previously used by the LV, which will
(if the SAN supports it) inform the SAN that the space is not in use
anymore and can be freed up. Since 'lvremove' modifies LVM metadata, it
has to be issued while holding a cluster-wide lock on the storage.
Unfortunately, depending on the setup, 'issue_discards = 1' can make
`lvremove` take very long for big disks (due to the large number of
discards being issued), so that it eventually hits the 60s timeout of
the cluster lock. The 60s are a hard-coded limit and cannot be easily
changed [2].

A better option is to issue discard before the final `lvremove`. This
informs the backing storage that the LV's blocks are no longer in use
without tying the potentially long-running discard operation to the
metadata update done by `lvremove`.

There is already a setting for `saferemove`, which zeroes out
to-be-deleted LVs before removing them. This series reworks that worker
to process the LV range by range instead of zeroing the whole LV in one
separate pass. This allows zero-out and discard to be combined: if both
actions are enabled, the worker zeroes one range, discards that same
range, and only then continues with the next range. This avoids forcing
a thin-provisioned SAN to allocate the whole LV with zeroes before the
space can be reclaimed again.

This series adds a new `on-volume-remove` property string with an
initial `discard` action. Following Fabian's feedback [4], the frontend
serializes the selected option into that property string, which is then
passed to the backend and parsed there. If only discard is enabled, the
renamed LV is discarded before the final remove. If `saferemove` is
enabled too, the worker performs the range-by-range zero-out and discard
described above.

Changes from v7 to v8 (thanks @Fiona, Friedrich and Michael):
-keep renamed LVs as `failed-<N>-del-*` if zeroout or discard fails
-check if the SAN supports discard before allowing the option to be set
in the frontend
-restructure module import order to adhere to the style guidelines
-use 10 MiB/s default, if "write zeroes" is not supported
-print in the frontend, if the throughput limit is applied
-import the SEEK_SET constant instead of 0 for the WHENCE value
-don't die, just warn if a short syswrite happens
-only log first and total blkdiscard failures
-add and rewrite some comments

Changes from v6 to v7 (thanks @Friedrich and Michael):
-rework the cleanup-worker to zero-out range by range
-rework the log and frontend messages
-document new 'saferemove' and 'discard' behavior
-make 'discard' opt-in and not default for newly created storages
-only discard bytes that were actually zeroed out before

Changes from v5 to v6:
-replace old storage config usage in the 'if'/'else' branches with the
new API variables

Changes from v4 to v5 (thanks @Friedrich and Fabian):
-rework the API layout to use a property string for volume-removal
options
-use 'if'/'else' instead of nested '?:' expressions for task log output
-revert renaming 'cleanup worker' to `discard worker`

Changes from v3 to v4 (thanks @Friedrich and Thomas):
-rework the worker-starting logic to avoid breaking abstraction layers
-rewrite the 'imgdel' task description in the UI to better match the
worker's behavior
-extend the code comment to also describe discard handling
-add additional syslog logging

Changes from v2 to v3 (thanks @Michael):
-correct issue_blkdiscard -> 'issue-blkdiscard' in the commit message
for the pve-manager
-replace 'previous commit' with a more obvious reference to first commit
of this series in the commit message for pve-manager

Changes from v1 to v2 (thanks @Michael, Maximiliano, Fabian):
-add more explicit descriptions in front- and backend, specifically
mentioning discard (TRIM)
-add a verbose description in the backend explaining the mechanism and
why it should be used for thin-provisioned storage
-add a forked fallback worker execution to allow other plugins to
issue workers without these config options
-rename variable issue_blkdiscard -> 'issue-blkdiscard' to conform to
newer style

[1] https://pve.proxmox.com/wiki/Migrate_to_Proxmox_VE#Storage_boxes_(SAN/NAS)
[2] https://forum.proxmox.com/threads/175849/post-820043
[3] https://man7.org/linux/man-pages/man8/blkdiscard.8.html
[4] https://lore.proxmox.com/all/177885528916.1932366.10236780530533306479@yuna.proxmox.com/
Link: bugzilla.proxmox.com/show_bug.cgi?id=7339


storage:

Lukas Sichert (3):
  lvm: saferemove: keep LVs where zero-out failed for manual zero-out
  lvm: saferemove: zero out volumes range by range
  fix #7339: lvm: add discard action for removed volumes

 src/PVE/Storage/LVMPlugin.pm | 390 ++++++++++++++++++++++++++++++-----
 1 file changed, 333 insertions(+), 57 deletions(-)


manager:

Lukas Sichert (1):
  fix #7339: lvmthick: ui: add UI option to free storage

 www/manager6/Utils.js           |  2 +-
 www/manager6/storage/LVMEdit.js | 45 +++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 1 deletion(-)


docs:

Lukas Sichert (1):
  fix #7339: lvm: document discard option

 pve-storage-lvm.adoc | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)


Summary over all repositories:
  4 files changed, 406 insertions(+), 65 deletions(-)

-- 
Generated by murpp 0.12.0

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

[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)",
+            );
         } 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,
+                            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",
+                        );
+                    },
+                );
 
-            $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";
+            }
         }
     };
 
-- 
2.47.3

[PATCH storage v8 2/5] lvm: saferemove: zero out volumes range by range

[Lukas Sichert]

'saferemove' currently uses different full-volume zero-out paths:
`blkdiscard --zeroout` for devices with write-zeroes support and
`cstream` otherwise. This makes consistent progress reporting and
throttling difficult and prevents interleaving future discard cleanup
with zeroing. On thin-provisioned backing storage, zeroing the whole LV
first can force unnecessary allocation.

Move zeroing into an explicit range loop. Use BLKZEROOUT when supported,
cap to the device limit, and fall back to manually writing zeroes via
syswrite otherwise. Add progress reporting in the shared loop, and apply
a configured saferemove throughput limit there as well. Without an
explicit limit, keep BLKZEROOUT unthrottled and throttle only manual
writes to 10 MiB/s.

Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
---
 src/PVE/Storage/LVMPlugin.pm | 146 +++++++++++++++++++++++++++--------
 1 file changed, 112 insertions(+), 34 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index a0feb5c..a3adeef 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -8,9 +8,12 @@ use File::Basename;
 use IO::File;
 use JSON;
 use List::Util qw(max);
+use Fcntl qw(SEEK_SET);
 
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::Tools qw(run_command file_read_firstline trim);
+use PVE::SafeSyslog;
+use PVE::Format qw(render_bytes render_duration);
 
 use PVE::Storage::Common;
 use PVE::Storage::Plugin;
@@ -19,6 +22,10 @@ use base qw(PVE::Storage::Plugin);
 
 # lvm helper functions
 
+use constant {
+    BLKZEROOUT => 0x127f,
+};
+
 my $ignore_no_medium_warnings = sub {
     my $line = shift;
     # ignore those, most of the time they're from (virtual) IPMI/iKVM devices
@@ -280,6 +287,13 @@ sub lvm_list_volumes {
     return $lvs;
 }
 
+my sub blockdev_ioctl_range {
+    my ($fh, $ioctl, $offset, $length) = @_;
+
+    my $range = pack('QQ', $offset, $length);
+    ioctl($fh, $ioctl, $range) or die "$!\n";
+}
+
 my sub free_lvm_volumes_locked {
     my ($class, $scfg, $storeid, $volnames) = @_;
 
@@ -305,48 +319,112 @@ my sub free_lvm_volumes_locked {
             file_read_firstline("$sysdir/queue/write_zeroes_max_bytes") // 0;
         ($write_zeroes_max_bytes) = $write_zeroes_max_bytes =~ m/^(\d+)$/; #untaint
 
+        my $size = file_read_firstline("$sysdir/size") or die "size of $sysdir cannot be read";
+        ($size) = $size =~ m/^(\d+)$/; # untaint
+        $size *= 512; # sysfs size is in 512-byte sectors
+
+        my $zeroout_variant = 'blkzeroout';
+        my $throughput = -1;
+        if ($scfg->{saferemove_throughput}) {
+            # use abs as legacy cstream accepted negative values
+            $throughput = abs($scfg->{saferemove_throughput});
+            my $throughput_in_mibs = render_bytes($throughput);
+            print "using saferemove throughput limit: $throughput_in_mibs/s\n";
+        }
+
+        # If the storage does not support write_zeroes fall back to writing zeroes manually using
+        # syswrite. Otherwise if the storage supports write_zeroes but stepsize is too big,
+        # reduce the stepsize to the maximum supported by the storage.
+        my $zeroes;
         if ($write_zeroes_max_bytes == 0) {
-            # If the storage does not support 'write zeroes', we fallback to cstream.
-            # wipe throughput up to 10MB/s by default; may be overwritten with saferemove_throughput
-            my $throughput = '-10485760';
-            if ($scfg->{saferemove_throughput}) {
-                $throughput = $scfg->{saferemove_throughput};
+            print
+                "WRITE_ZEROES operation not supported, falling back to syswrite to zero-out '$lvmpath'\n";
+            $zeroout_variant = 'syswrite';
+            $stepsize = 1024 * 1024; # 1 MiB
+            print "reduce stepsize to 1 MiB for syswrite\n";
+            $zeroes = "\0" x $stepsize;
+            # limit throughput to 10MiB/s for syswrite, if throughput was not set
+            if ($throughput <= 0) {
+                # FIXME: increase to 100 MiB/s with 10.0
+                $throughput = 10485760;
+                print "using default syswrite-saferemove throughput limit: 10 MiB/s\n";
             }
+        } elsif ($stepsize > $write_zeroes_max_bytes) {
+            print "reduce stepsize to the maximum supported by the storage:"
+                . " $write_zeroes_max_bytes bytes\n";
+            $stepsize = $write_zeroes_max_bytes;
+        }
+        open(my $fh, '+<', $lvmpath) or die "can't open '$lvmpath' - $!\n";
 
-            my $cmd = [
-                '/usr/bin/cstream',
-                '-i',
-                '/dev/zero',
-                '-o',
-                $lvmpath,
-                '-T',
-                '10',
-                '-v',
-                '1',
-                '-b',
-                '1048576',
-                '-t',
-                "$throughput",
-            ];
-            run_command(
-                $cmd,
-                errmsg => "zero out finished (note: 'No space left on device' is ok here)",
-            );
-        } else {
-            # If the storage supports write_zeroes but stepsize is too big, reduce the stepsize to
-            # the maximum supported by the storage.
-            if ($write_zeroes_max_bytes > 0 && $stepsize > $write_zeroes_max_bytes) {
-                print "reduce stepsize to the maximum supported by the storage:"
-                    . " $write_zeroes_max_bytes bytes\n";
+        # eval block, so filehandle is closed even if something fails below
+        eval {
+            my $start = time();
+            my $written_total = 0;
+            my $lastprint = -1;
+            my $written;
+
+            for (my $offset = 0; $offset < $size; $offset += $written) {
 
-                $stepsize = $write_zeroes_max_bytes;
+                if ($offset + $stepsize > $size) {
+                    $stepsize = $size - $offset;
+                }
+
+                if ($zeroout_variant eq 'blkzeroout') {
+                    eval { blockdev_ioctl_range($fh, BLKZEROOUT, $offset, $stepsize); };
+                    if ($@) {
+                        die "blkzeroout for $stepsize bytes at offset $offset failed: $@";
+                    }
+                    $written = $stepsize;
+                } elsif ($zeroout_variant eq 'syswrite') {
+                    # if the $offset is 0, sysseek can return 0, therefore use // to only
+                    # throw an error, if it returns undef
+                    sysseek($fh, $offset, SEEK_SET) // die "sysseek failed: $!\n";
+
+                    # use or as we also want to die if no progress was made, i.e. if $written is 0
+                    $written = syswrite($fh, $zeroes, $stepsize)
+                        or die "syswrite failed: $!\n";
+
+                    if ($written != $stepsize) {
+                        warn "short syswrite: wrote $written of $stepsize bytes\n";
+                    }
+
+                }
+                $written_total += $written;
+
+                my $curr_time = time();
+                if (($curr_time - $lastprint) >= 3) {
+                    my $percent_finished = 100 * $written_total / $size;
+                    my $curr_seconds = $curr_time - $start;
+
+                    printf(
+                        "zeroed out %s of %s (%.2f%%) using %s in %s seconds\n",
+                        render_bytes($written_total),
+                        render_bytes($size),
+                        $percent_finished,
+                        $zeroout_variant,
+                        render_duration($curr_seconds),
+                    );
+                    $lastprint = $curr_time;
+                }
+
+                if ($throughput > 0) {
+                    my $expected_elapsed = $written_total / $throughput;
+                    my $actual_elapsed = $curr_time - $start;
+                    my $delay = $expected_elapsed - $actual_elapsed;
+                    if ($delay > 0) {
+                        sleep($delay);
+                    }
+                }
             }
 
-            my $cmd = ['blkdiscard', $lvmpath, '-v', '--zeroout', '--step', "${stepsize}"];
-            run_command($cmd);
+        };
+        # close filehandle before throwing an error
+        my $err = $@;
+        close($fh);
+        if ($err) {
+            die "$err";
         }
     };
-
     # we need to zero out LVM data for security reasons
     # and to allow thin provisioning
     my $zero_out_worker = sub {
-- 
2.47.3

[PATCH storage v8 3/5] fix #7339: lvm: add discard action for removed volumes

[Lukas Sichert]

On LVM storages backed by thin-provisioned SAN LUNs, removing an LV does
not release the allocated space on the backing storage. Using LVM's
`issue_discards` can avoid that, but makes `lvremove` issue the discards
while holding the cluster-wide storage lock, which can hit the lock
timeout for large volumes.

Add an `on-volume-remove` property with an initial `discard` action.
Reject the option if discard is not supported by the backing devices.
The cleanup worker issues the discard for the renamed LV before the
final remove, so the long-running operation happens outside the storage
lock. When combined with `saferemove`, the worker zeroes and discards
the LV range by range, avoiding allocation of the whole LV with zeroes
on thin-provisioned backing storage.

Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
Link: bugzilla.proxmox.com/show_bug.cgi?id=7339
---
 src/PVE/Storage/LVMPlugin.pm | 186 +++++++++++++++++++++++++++++++++--
 1 file changed, 177 insertions(+), 9 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index a3adeef..8653331 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -12,8 +12,10 @@ use Fcntl qw(SEEK_SET);
 
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::Tools qw(run_command file_read_firstline trim);
+use PVE::RESTEnvironment qw(log_warn);
 use PVE::SafeSyslog;
 use PVE::Format qw(render_bytes render_duration);
+use PVE::Exception qw(raise_param_exc);
 
 use PVE::Storage::Common;
 use PVE::Storage::Plugin;
@@ -23,6 +25,7 @@ use base qw(PVE::Storage::Plugin);
 # lvm helper functions
 
 use constant {
+    BLKDISCARD => 0x1277,
     BLKZEROOUT => 0x127f,
 };
 
@@ -299,6 +302,12 @@ my sub free_lvm_volumes_locked {
 
     my $vg = $scfg->{vgname};
 
+    my $on_remove_opts = {};
+    if ($scfg->{'on-volume-remove'}) {
+        $on_remove_opts =
+            PVE::JSONSchema::parse_property_string('on-volume-remove', $scfg->{'on-volume-remove'});
+    }
+
     my $secure_delete_cmd = sub {
         my ($lvmpath) = @_;
 
@@ -362,6 +371,8 @@ my sub free_lvm_volumes_locked {
             my $written_total = 0;
             my $lastprint = -1;
             my $written;
+            my $discard_attempts = 0;
+            my $discard_failures = 0;
 
             for (my $offset = 0; $offset < $size; $offset += $written) {
 
@@ -389,6 +400,18 @@ my sub free_lvm_volumes_locked {
                     }
 
                 }
+                if ($on_remove_opts->{discard}) {
+                    $discard_attempts++;
+
+                    eval { blockdev_ioctl_range($fh, BLKDISCARD, $offset, $written); };
+                    if ($@) {
+                        if ($discard_failures == 0) {
+                            log_warn(
+                                "blkdiscard for $written bytes at offset $offset failed: $@");
+                        }
+                        $discard_failures += 1;
+                    }
+                }
                 $written_total += $written;
 
                 my $curr_time = time();
@@ -416,7 +439,9 @@ my sub free_lvm_volumes_locked {
                     }
                 }
             }
-
+            if ($discard_failures != 0) {
+                die "$discard_failures out of $discard_attempts blkdiscards failed\n";
+            }
         };
         # close filehandle before throwing an error
         my $err = $@;
@@ -426,24 +451,44 @@ my sub free_lvm_volumes_locked {
         }
     };
     # we need to zero out LVM data for security reasons
-    # and to allow thin provisioning
-    my $zero_out_worker = sub {
+    # and discard images to free storage space to allow
+    # thin provisioning
+    my $cleanup_worker = sub {
+
         for my $name (@$volnames) {
             my $lvmpath = "/dev/$vg/del-$name";
-            print "zero-out data on image $name ($lvmpath)\n";
+
+            my $discard_action;
+            if ($scfg->{saferemove} && $on_remove_opts->{discard}) {
+                $discard_action = 'zero-out and discard (TRIM)';
+            } elsif ($scfg->{saferemove}) {
+                $discard_action = 'zero-out';
+            } elsif ($on_remove_opts->{discard}) {
+                $discard_action = 'discard (TRIM)';
+            }
+            print "$discard_action 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",
+                errmsg => "can't activate LV '$lvmpath' to $discard_action its data",
             );
             $cmd_activate = ['/sbin/lvchange', '--refresh', $lvmpath];
             run_command(
                 $cmd_activate,
-                errmsg => "can't refresh LV '$lvmpath' to zero-out its data",
+                errmsg => "can't refresh LV '$lvmpath' to $discard_action its data",
             );
+            syslog('info', "starting to $discard_action $name ($lvmpath)");
 
-            eval { $secure_delete_cmd->($lvmpath); };
+            eval {
+                if ($scfg->{saferemove}) {
+                    $secure_delete_cmd->($lvmpath);
+                }
+
+                if ($on_remove_opts->{discard} && !$scfg->{saferemove}) {
+                    run_command(['/sbin/blkdiscard', $lvmpath]);
+                }
+            };
             if (my $cleanup_err = $@) {
                 my $failed_name;
                 $class->cluster_lock_storage(
@@ -491,13 +536,13 @@ my sub free_lvm_volumes_locked {
         }
     };
 
-    if ($scfg->{saferemove}) {
+    if ($scfg->{saferemove} || $on_remove_opts->{discard}) {
         for my $name (@$volnames) {
             # avoid long running task, so we only rename here
             my $cmd = ['/sbin/lvrename', $vg, $name, "del-$name"];
             run_command($cmd, errmsg => "lvrename '$vg/$name' error");
         }
-        return $zero_out_worker;
+        return $cleanup_worker;
     } else {
         for my $name (@$volnames) {
             my $cmd = ['/sbin/lvremove', '-f', "$vg/$name"];
@@ -520,6 +565,36 @@ sub plugindata {
     };
 }
 
+my $on_volume_remove_format = {
+    discard => {
+        description => "Issue discard (TRIM) requests for LVs before removing them.",
+        type => 'boolean',
+        optional => 1,
+        verbose_description => "If enabled, blkdiscard is issued for the LV before removing it."
+            . " This sends discard (TRIM) requests for the LV's block range, allowing"
+            . " thin-provisioned storage to reclaim previously allocated physical"
+            . " space, provided the storage supports discard.",
+    },
+};
+
+sub verify_on_volume_remove {
+    my ($value, $noerr) = @_;
+
+    return undef if !defined($value);
+
+    if (!keys %$value) {
+        return undef if $noerr;
+        die "at least one on-volume-remove option must be specified if the property is set\n";
+    }
+    return $value;
+}
+
+PVE::JSONSchema::register_format(
+    'on-volume-remove',
+    $on_volume_remove_format,
+    \&verify_on_volume_remove,
+);
+
 sub properties {
     return {
         vgname => {
@@ -536,6 +611,13 @@ sub properties {
             description => "Zero-out data when removing LVs.",
             type => 'boolean',
         },
+        'on-volume-remove' => {
+            description => "Optional actions when removing LVs.",
+            type => 'string',
+            format => 'on-volume-remove',
+            verbose_description => "Configure actions performed before removing an LV."
+                . " Use 'discard=1' to issue discard (TRIM) requests before removal.",
+        },
         'saferemove-stepsize' => {
             description => "Wipe step size in MiB."
                 . " It will be capped to the maximum supported by the storage.",
@@ -561,6 +643,7 @@ sub options {
         shared => { optional => 1 },
         disable => { optional => 1 },
         saferemove => { optional => 1 },
+        'on-volume-remove' => { optional => 1 },
         'saferemove-stepsize' => { optional => 1 },
         saferemove_throughput => { optional => 1 },
         content => { optional => 1 },
@@ -583,6 +666,88 @@ sub get_formats {
     return { default => 'raw', valid => { 'raw' => 1 } };
 }
 
+my sub get_discard_max {
+    my ($dev_path) = @_;
+
+    my $output = '';
+    # use lsblk as it resolves discard support in setups with
+    # nested partitions or lv/vgs
+    my $cmd = [
+        '/bin/lsblk',
+        '--json',
+        '--bytes',
+        '--discard',
+        '--nodeps',
+        '--output',
+        'PATH,DISC-MAX',
+        $dev_path,
+    ];
+
+    eval {
+        run_command($cmd, outfunc => sub { $output .= "$_[0]\n"; });
+    };
+    if ($@) {
+        raise_param_exc({
+            'on-volume-remove' => "discard on remove is enabled, but lsblk could not "
+                . "query discard support for the backing device '$dev_path'",
+        });
+    }
+
+    my $parsed = eval { decode_json($output) };
+    if ($@ || !$parsed->{blockdevices} || !$parsed->{blockdevices}->[0]) {
+        raise_param_exc({
+            'on-volume-remove' => "discard on remove is enabled, but lsblk could not "
+                . "parse discard support for the backing device '$dev_path'",
+        });
+
+    }
+
+    return $parsed->{blockdevices}->[0]->{'disc-max'} // 0;
+}
+
+sub assert_discard_supported {
+    my ($vgname, $on_remove) = @_;
+
+    return if !defined($on_remove);
+
+    my $on_remove_opts = PVE::JSONSchema::parse_property_string('on-volume-remove', $on_remove);
+    if ($on_remove_opts->{discard}) {
+
+        my $vgs = lvm_vgs(1);
+        my $vg = $vgs->{$vgname};
+        die "no such volume group '$vgname'\n" if !$vg;
+
+        my $pvs = $vg->{pvs};
+        die "volume group '$vgname' has no physical volumes\n"
+            if !defined($pvs) || scalar($pvs->@*) == 0;
+
+        # check if all the block devices configured to the volume group support discard
+        for my $pv ($vg->{pvs}->@*) {
+
+            my $dev_path = abs_path($pv->{name}) // $pv->{name};
+
+            if ($dev_path !~ m!^(/dev/[A-Za-z0-9_+./=-]+)$!) {
+                raise_param_exc({
+                    'on-volume-remove' => "discard on remove is enabled, but discard support "
+                        . "cannot be resolved for the backing device '$dev_path'",
+                });
+            } else {
+                $dev_path = $1; # untaint
+            }
+
+            my $discard_max_bytes = get_discard_max($dev_path);
+
+            # use discard_max_bytes as indicator if discard is supported
+            if (!$discard_max_bytes) {
+                raise_param_exc({
+                    'on-volume-remove' => "discard on remove is enabled, but discard is not "
+                        . "supported by the backing device '$dev_path'",
+                });
+            }
+        }
+    }
+}
+
 sub on_add_hook {
     my ($class, $storeid, $scfg, %param) = @_;
 
@@ -604,6 +769,8 @@ sub on_add_hook {
         lvm_create_volume_group($path, $scfg->{vgname}, $scfg->{shared});
     }
 
+    assert_discard_supported($scfg->{vgname}, $scfg->{'on-volume-remove'});
+
     return;
 }
 
@@ -624,6 +791,7 @@ sub on_update_hook_full {
         die "$storeid - cannot disable 'snapshot-as-volume-chain' while a qcow2 image exists\n"
             if grep { $_->{format} eq 'qcow2' } $images->@*;
     }
+    assert_discard_supported($scfg->{vgname}, $update->{'on-volume-remove'});
 }
 
 sub parse_volname {
-- 
2.47.3

[PATCH manager v8 4/5] fix #7339: lvmthick: ui: add UI option to free storage

[Lukas Sichert]

Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
---
 www/manager6/Utils.js           |  2 +-
 www/manager6/storage/LVMEdit.js | 45 +++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 040b5ae0..3088e6a7 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -2184,7 +2184,7 @@ Ext.define('PVE.Utils', {
             hastart: ['HA', gettext('Start')],
             hastop: ['HA', gettext('Stop')],
             imgcopy: ['', gettext('Copy data')],
-            imgdel: ['', gettext('Erase data')],
+            imgdel: ['', gettext('Destroy image')],
             lvmcreate: [gettext('LVM Storage'), gettext('Create')],
             lvmremove: ['Volume Group', gettext('Remove')],
             lvmthincreate: [gettext('LVM-Thin Storage'), gettext('Create')],
diff --git a/www/manager6/storage/LVMEdit.js b/www/manager6/storage/LVMEdit.js
index 148f0601..30ae275b 100644
--- a/www/manager6/storage/LVMEdit.js
+++ b/www/manager6/storage/LVMEdit.js
@@ -148,6 +148,39 @@ Ext.define('PVE.storage.LVMInputPanel', {
 
     onlineHelp: 'storage_lvm',
 
+    onGetValues: function (values) {
+        let me = this;
+
+        let onRemove = {};
+        if (values['on-remove-discard']) {
+            onRemove.discard = 1;
+        }
+        delete values['on-remove-discard'];
+
+        let onRemoveString = PVE.Parser.printPropertyString(onRemove);
+        if (onRemoveString !== '') {
+            values['on-volume-remove'] = onRemoveString;
+        } else if (!me.isCreate) {
+            if (!values.delete) {
+                values.delete = [];
+            }
+            values.delete.push('on-volume-remove');
+        }
+
+        return me.callParent([values]);
+    },
+
+    setValues: function (values) {
+        if (values['on-volume-remove']) {
+            let onRemove = PVE.Parser.parsePropertyString(values['on-volume-remove']);
+            values['on-remove-discard'] = onRemove.discard;
+        }
+
+        delete values['on-volume-remove'];
+
+        return this.callParent([values]);
+    },
+
     column1: [
         {
             xtype: 'pveBaseStorageSelector',
@@ -241,5 +274,17 @@ Ext.define('PVE.storage.LVMInputPanel', {
             uncheckedValue: 0,
             fieldLabel: gettext('Wipe Removed Volumes'),
         },
+        {
+            xtype: 'proxmoxcheckbox',
+            name: 'on-remove-discard',
+            uncheckedValue: 0,
+            fieldLabel: gettext('Discard Removed Volumes'),
+            autoEl: {
+                tag: 'div',
+                'data-qtip': gettext(
+                    'Enable to issue discard (TRIM) requests for logical volumes before removing them.',
+                ),
+            },
+        },
     ],
 });
-- 
2.47.3

[PATCH docs v8 5/5] fix #7339: lvm: document discard option

[Lukas Sichert]

Document the new `on-volume-remove` property for LVM storage and its
initial `discard` action.

Also update the `saferemove` description to match the range-based
zero-out worker and avoid referring to the old command-specific
implementation details.

Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
Link: bugzilla.proxmox.com/show_bug.cgi?id=7339
---
 pve-storage-lvm.adoc | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/pve-storage-lvm.adoc b/pve-storage-lvm.adoc
index ba78663..3ddb26c 100644
--- a/pve-storage-lvm.adoc
+++ b/pve-storage-lvm.adoc
@@ -44,18 +44,39 @@ accessed by other LVs created later (which happen to be assigned the same
 physical extents). This is a costly operation, but may be required as a security
 measure in certain environments.
 +
-Storage devices that support the "write zeroes" operation will use `blkdiscard`
-to zero blocks. Otherwise, a fallback to `cstream` is performed.
+Storage devices that support the "write zeroes" operation use it to zero blocks.
+Otherwise, zeroes are written manually. The volume is processed range by range,
+according to `saferemove-stepsize`.
+
+`on-volume-remove`::
+
+Configure additional actions to run before an LV is removed.
++
+Set `discard=1` to issue discard (TRIM) requests for the LV's blocks before the
+LV is removed, so thin-provisioned backing storage, such as a SAN LUN, can
+reclaim space the LV occupied. This is called "Discard Removed Volumes"
+in the web UI. Discard is rejected if any backing device in the
+LV's volume group is detected as not supporting it.
++
+If wiping or discarding fails, the renamed `del-*` LV is kept and renamed to
+`failed-<N>-del-*`, so one can inspect it, retry cleanup manually, or remove
+it explicitly.
++
+If `saferemove` and `discard` are both enabled, the LV is processed range by
+range: one range is zeroed out and then discarded before continuing with the
+next range. This avoids allocating the whole LV with zeroes on thin-provisioned
+backing storage before the space can be reclaimed again.
 
 `saferemove-stepsize`::
 
-Wipe step size in MiB (`blkdiscard -p` parameter value), capped to the maximum
-step size supported by the underlying storage. Up to 32 MiB (maximum) by
-default.
+Wipe step size in MiB, capped to the maximum step size supported by the
+underlying storage. Up to 32 MiB (maximum) by default.
 
 `saferemove_throughput`::
 
-Wipe throughput (`cstream -t` parameter value), up to 10 MiB/s by default.
+Limits wipe throughput. If the backing storage supports the "write zeroes"
+operation, throughput is unlimited by default. Otherwise, manually written
+zeroes are limited to 10 MiB/s by default.
 
 `snapshot-as-volume-chain`::
 
@@ -150,4 +171,3 @@ See Also
 
 endif::wiki[]
 
-
-- 
2.47.3