* [PATCH storage v8 1/5] lvm: saferemove: keep LVs where zero-out failed for manual zero-out
2026-07-07 14:32 [PATCH docs/manager/storage v8 0/5] fix #7339: lvmthick: add option to free storage for deleted VMs Lukas Sichert
@ 2026-07-07 14:32 ` Lukas Sichert
2026-07-08 16:25 ` Michael Köppl
2026-07-10 9:26 ` Fiona Ebner
2026-07-07 14:32 ` [PATCH storage v8 2/5] lvm: saferemove: zero out volumes range by range Lukas Sichert
` (4 subsequent siblings)
5 siblings, 2 replies; 12+ messages in thread
From: Lukas Sichert @ 2026-07-07 14:32 UTC (permalink / raw)
To: pve-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH storage v8 1/5] lvm: saferemove: keep LVs where zero-out failed for manual zero-out
2026-07-07 14:32 ` [PATCH storage v8 1/5] lvm: saferemove: keep LVs where zero-out failed for manual zero-out Lukas Sichert
@ 2026-07-08 16:25 ` Michael Köppl
2026-07-10 9:26 ` Fiona Ebner
1 sibling, 0 replies; 12+ messages in thread
From: Michael Köppl @ 2026-07-08 16:25 UTC (permalink / raw)
To: Lukas Sichert, pve-devel
On Tue Jul 7, 2026 at 4:32 PM CEST, Lukas Sichert wrote:
[snip]
>
> @@ -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();
You could pass $vg as a parameter here to limit the lvs command to that
volume group, no?
> + 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 : ()
[snip]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH storage v8 1/5] lvm: saferemove: keep LVs where zero-out failed for manual zero-out
2026-07-07 14:32 ` [PATCH storage v8 1/5] lvm: saferemove: keep LVs where zero-out failed for manual zero-out Lukas Sichert
2026-07-08 16:25 ` Michael Köppl
@ 2026-07-10 9:26 ` Fiona Ebner
1 sibling, 0 replies; 12+ messages in thread
From: Fiona Ebner @ 2026-07-10 9:26 UTC (permalink / raw)
To: Lukas Sichert, pve-devel
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";
> + }
> }
> };
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH storage v8 2/5] lvm: saferemove: zero out volumes range by range
2026-07-07 14:32 [PATCH docs/manager/storage v8 0/5] fix #7339: lvmthick: add option to free storage for deleted VMs Lukas Sichert
2026-07-07 14:32 ` [PATCH storage v8 1/5] lvm: saferemove: keep LVs where zero-out failed for manual zero-out Lukas Sichert
@ 2026-07-07 14:32 ` Lukas Sichert
2026-07-08 16:26 ` Michael Köppl
2026-07-10 10:20 ` Fiona Ebner
2026-07-07 14:32 ` [PATCH storage v8 3/5] fix #7339: lvm: add discard action for removed volumes Lukas Sichert
` (3 subsequent siblings)
5 siblings, 2 replies; 12+ messages in thread
From: Lukas Sichert @ 2026-07-07 14:32 UTC (permalink / raw)
To: pve-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH storage v8 2/5] lvm: saferemove: zero out volumes range by range
2026-07-07 14:32 ` [PATCH storage v8 2/5] lvm: saferemove: zero out volumes range by range Lukas Sichert
@ 2026-07-08 16:26 ` Michael Köppl
2026-07-10 10:20 ` Fiona Ebner
1 sibling, 0 replies; 12+ messages in thread
From: Michael Köppl @ 2026-07-08 16:26 UTC (permalink / raw)
To: Lukas Sichert, pve-devel
On Tue Jul 7, 2026 at 4:32 PM CEST, Lukas Sichert wrote:
[snip]
>
> - my $cmd = [
> - '/usr/bin/cstream',
> - '-i',
> - '/dev/zero',
> - '-o',
> - $lvmpath,
> - '-T',
> - '10',
> - '-v',
> - '1',
> - '-b',
> - '1048576',
> - '-t',
> - "$throughput",
Since this is not used anymore, the schema description for
saferemove_throughput should probably be adapted as well, since it says
"Wipe throughput (cstream -t parameter value)". You adapted the
documentation accordingly, but the schema is still the same. I
unfortunately also missed this in the previous versions.
> - ];
> - 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;
> +
[snip]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH storage v8 2/5] lvm: saferemove: zero out volumes range by range
2026-07-07 14:32 ` [PATCH storage v8 2/5] lvm: saferemove: zero out volumes range by range Lukas Sichert
2026-07-08 16:26 ` Michael Köppl
@ 2026-07-10 10:20 ` Fiona Ebner
1 sibling, 0 replies; 12+ messages in thread
From: Fiona Ebner @ 2026-07-10 10:20 UTC (permalink / raw)
To: Lukas Sichert, pve-devel
Am 07.07.26 um 4:33 PM schrieb 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.
Nit: s/manual writes/syswrites/
>
> 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);
Nit: Not ordered alphabetically
>
> 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);
Nit: Not ordered alphabetically
>
> 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";
Missing newline at the end of error message
s/size of/size from/
> + ($size) = $size =~ m/^(\d+)$/; # untaint
> + $size *= 512; # sysfs size is in 512-byte sectors
> +
> + my $zeroout_variant = 'blkzeroout';
> + my $throughput = -1;
Nit: I'd prefer having it be undef rather than some magic value,
especially since -1 is valid for saferemove_throughput. And unrelated to
the series, saferemove_throughput is type => string, I guess we should
change it to a number/integer. cstream just ignores it if it cannot
parse it, so a bad value is silently ignored right now, but since you
use it for calculations, it will cause errors/warnings, so let's catch
it with the schema already. An already present invalid value would then
cause warnings and be dropped on the next storage.cfg write.
> + 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";
Nit: it's not necessarily in mibs, render_bytes() might choose a
different unit. The 's' seems also a bit off, because otherwise it
wouldn't be mibs/s. Maybe just call it $rendered(_throughput)?
> + }
> +
> + # 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";
Style nit: line too long
> + $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
Please use the standard format:
# FIXME: MAJOR VERSION: increase ...
> + $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";
> + }
I don't think a warning is warranted here. You can just re-issue the
rest of the write. I would do that with an inner loop to keep the
alignment for subsequent non-short writes in-line with the stepsize.
> +
> + }
> + $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";
> }
> };
> -
Style nit: I'd keep the blank here for better separation
> # we need to zero out LVM data for security reasons
> # and to allow thin provisioning
> my $zero_out_worker = sub {
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH storage v8 3/5] fix #7339: lvm: add discard action for removed volumes
2026-07-07 14:32 [PATCH docs/manager/storage v8 0/5] fix #7339: lvmthick: add option to free storage for deleted VMs Lukas Sichert
2026-07-07 14:32 ` [PATCH storage v8 1/5] lvm: saferemove: keep LVs where zero-out failed for manual zero-out Lukas Sichert
2026-07-07 14:32 ` [PATCH storage v8 2/5] lvm: saferemove: zero out volumes range by range Lukas Sichert
@ 2026-07-07 14:32 ` Lukas Sichert
2026-07-10 11:49 ` Fiona Ebner
2026-07-07 14:32 ` [PATCH manager v8 4/5] fix #7339: lvmthick: ui: add UI option to free storage Lukas Sichert
` (2 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Lukas Sichert @ 2026-07-07 14:32 UTC (permalink / raw)
To: pve-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH storage v8 3/5] fix #7339: lvm: add discard action for removed volumes
2026-07-07 14:32 ` [PATCH storage v8 3/5] fix #7339: lvm: add discard action for removed volumes Lukas Sichert
@ 2026-07-10 11:49 ` Fiona Ebner
0 siblings, 0 replies; 12+ messages in thread
From: Fiona Ebner @ 2026-07-10 11:49 UTC (permalink / raw)
To: Lukas Sichert, pve-devel
Am 07.07.26 um 4:33 PM schrieb Lukas Sichert:
> 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);
Nit: not ordered alphabetically
>
> 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 ($@) {
Style nit: usually, we assign immediately to a local variable in such
cases, i.e. if (my $err = $@) {
> + 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}) {
Style nit: I would put it into an elsif
> + run_command(['/sbin/blkdiscard', $lvmpath]);
Nit: should we put an errmsg here?
---snip 8<---
> @@ -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
Style nit: comment fits in one line
> + my $cmd = [
> + '/bin/lsblk',
Nit: use just 'lsblk'
> + '--json',
> + '--bytes',
> + '--discard',
> + '--nodeps',
> + '--output',
> + 'PATH,DISC-MAX',
Nit: The path is not used later
> + $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'",
Please include the error $@ for completeness
> + });
> + }
> +
> + 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;
Nit: We could warn if there is more than one result or are there cases
where that is expected and non-problematic?
> +}
> +
> +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
Style nit: I'd prefer using =~ and switching the branches so the $1 is
closer to where it's obtained
> + }
> +
> + 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 {
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH manager v8 4/5] fix #7339: lvmthick: ui: add UI option to free storage
2026-07-07 14:32 [PATCH docs/manager/storage v8 0/5] fix #7339: lvmthick: add option to free storage for deleted VMs Lukas Sichert
` (2 preceding siblings ...)
2026-07-07 14:32 ` [PATCH storage v8 3/5] fix #7339: lvm: add discard action for removed volumes Lukas Sichert
@ 2026-07-07 14:32 ` Lukas Sichert
2026-07-07 14:32 ` [PATCH docs v8 5/5] fix #7339: lvm: document discard option Lukas Sichert
2026-07-08 16:35 ` [PATCH docs/manager/storage v8 0/5] fix #7339: lvmthick: add option to free storage for deleted VMs Michael Köppl
5 siblings, 0 replies; 12+ messages in thread
From: Lukas Sichert @ 2026-07-07 14:32 UTC (permalink / raw)
To: pve-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH docs v8 5/5] fix #7339: lvm: document discard option
2026-07-07 14:32 [PATCH docs/manager/storage v8 0/5] fix #7339: lvmthick: add option to free storage for deleted VMs Lukas Sichert
` (3 preceding siblings ...)
2026-07-07 14:32 ` [PATCH manager v8 4/5] fix #7339: lvmthick: ui: add UI option to free storage Lukas Sichert
@ 2026-07-07 14:32 ` Lukas Sichert
2026-07-08 16:35 ` [PATCH docs/manager/storage v8 0/5] fix #7339: lvmthick: add option to free storage for deleted VMs Michael Köppl
5 siblings, 0 replies; 12+ messages in thread
From: Lukas Sichert @ 2026-07-07 14:32 UTC (permalink / raw)
To: pve-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH docs/manager/storage v8 0/5] fix #7339: lvmthick: add option to free storage for deleted VMs
2026-07-07 14:32 [PATCH docs/manager/storage v8 0/5] fix #7339: lvmthick: add option to free storage for deleted VMs Lukas Sichert
` (4 preceding siblings ...)
2026-07-07 14:32 ` [PATCH docs v8 5/5] fix #7339: lvm: document discard option Lukas Sichert
@ 2026-07-08 16:35 ` Michael Köppl
5 siblings, 0 replies; 12+ messages in thread
From: Michael Köppl @ 2026-07-08 16:35 UTC (permalink / raw)
To: Lukas Sichert, pve-devel
Thanks for the patches!
For this version I didn't have a SAN setup readily available, so I
tested with a loop-backed LVM thin pool. On top of a thin LV I created a
LVM-thick storage with discard enabled, created a VM on it, wrote some
data (dd), confirmed that the thin pool's allocation grew, and then
removed the VM. I observed the thin pool's usage drop back down again,
as expected. I saw the "discard (TRIM) data on image vm-9999-disk-0
(/dev/testlvm/del-vm-9999-disk-0)" as expected as well.
When also enabling saferemove, I could observe the usage in the thin
pool drop range by range, as well as observe the "zeroed-out ..." logs.
A thing I noticed is that it doesn't print the 100% log. For my case,
the last log line read 99.80% and then Logical volume
"del-vm-9999-disk-0" successfully removed. I don't think it's a
problem, though. Just looked a bit weird at first.
I also tested what happens if there's a failure during
discard+saferemove removal by making the LV readonly, removing the disk
(which shows "cleanup failed for lv vm-9999-disk-0..."), creating a new
disk with the same base name, removing it again, again yielding "cleanup
failed". Then I checked that `lvs testlvm` reports
failed-1-del-vm-9999-disk-0 and failed-2-del-vm-9999-disk-0, as expected.
Haven't had time yet to do a full review of the code, but added 2
comments on the individual patches.
Consider this:
Tested-by: Michael Köppl <m.koeppl@proxmox.com>
On Tue Jul 7, 2026 at 4:32 PM CEST, Lukas Sichert wrote:
> 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
[snip]
^ permalink raw reply [flat|nested] 12+ messages in thread