[PATCH storage v12 4/6] fix #7339: lvm: add discard action for removed volumes

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

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 the LV in
ranges and batches discard requests at boundaries aligned with the
discard granularity. This avoids skipping complete discard chunks while
also avoiding allocation of the whole LV with zeroes on thin-provisioned
backing storage. When zeroes are written using `syswrite`, flush the file
handle before issuing the discard to ensure the writes reach the backing
storage first.

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

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index db988fc..60686e1 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -10,6 +10,7 @@ use JSON;
 use List::Util qw(max);
 use Time::HiRes qw(clock_gettime CLOCK_MONOTONIC);
 
+use PVE::Exception qw(raise_param_exc);
 use PVE::Format qw(render_bytes render_duration);
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::RESTEnvironment qw(log_warn);
@@ -23,6 +24,7 @@ use base qw(PVE::Storage::Plugin);
 # lvm helper functions
 
 use constant {
+    BLKDISCARD => 0x1277,
     BLKZEROOUT => 0x127f,
 };
 
@@ -337,11 +339,33 @@ my sub blockdev_ioctl_range {
     ioctl($fh, $ioctl, $range) or die "$!\n";
 }
 
+# Calculate the greatest common divisor using Euclid's algorithm.
+my sub gcd {
+    my ($a, $b) = @_;
+
+    ($a, $b) = ($b, $a % $b) while $b;
+    return abs($a);
+}
+
+# Calculate the least common multiple.
+my sub lcm {
+    my ($a, $b) = @_;
+
+    return 0 if !$a || !$b;
+    return abs(($a / gcd($a, $b)) * $b);
+}
+
 my sub free_lvm_volumes_locked {
     my ($class, $scfg, $storeid, $volnames) = @_;
 
     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 +386,14 @@ 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 $discard_granularity = file_read_firstline("$sysdir/queue/discard_granularity") // 0;
+        ($discard_granularity) = $discard_granularity =~ m/^(\d+)$/; #untaint
+
+        # Discard support is normally checked during configuration validation, but
+        # unsupported configurations can still be introduced by editing the config.
+        die "invalid discard granularity for '$lvmpath'\n"
+            if $on_remove_opts->{discard} && !$discard_granularity;
+
         my $size = file_read_firstline("$sysdir/size")
             or die "size from $sysdir cannot be read\n";
         ($size) = $size =~ m/^(\d+)$/; # untaint
@@ -398,6 +430,13 @@ my sub free_lvm_volumes_locked {
                 . " $write_zeroes_max_bytes bytes\n";
             $stepsize = $write_zeroes_max_bytes;
         }
+
+        # LVM only discards chunks fully covered by a discard request. Batch the
+        # requests so that their boundaries are aligned to both the zeroing step
+        # size and the discard granularity.
+        my $discard_stepsize = lcm($stepsize, $discard_granularity);
+        my $discard_offset = 0;
+
         open(my $fh, '+<', $lvmpath) or die "can't open '$lvmpath' - $!\n";
 
         # eval block, so filehandle is closed even if something fails below
@@ -407,6 +446,9 @@ my sub free_lvm_volumes_locked {
             my $lastprint = -1;
             my $written;
 
+            my $discard_attempts = 0;
+            my $discard_failures = 0;
+
             for (my $offset = 0; $offset < $size; $offset += $written) {
 
                 if ($offset + $stepsize > $size) {
@@ -442,6 +484,33 @@ my sub free_lvm_volumes_locked {
                 }
                 $written_total += $written;
 
+                if (
+                    $on_remove_opts->{discard}
+                    && ($written_total % $discard_stepsize == 0 || $written_total == $size)
+                ) {
+                    if ($zeroout_variant eq 'syswrite') {
+                        # Flush zeroes written using syswrite before discarding the
+                        # corresponding range
+                        $fh->sync()
+                            or die "fsync before discard at offset $discard_offset failed: $!\n";
+                    }
+
+                    my $discard_length = $written_total - $discard_offset;
+                    $discard_attempts++;
+
+                    eval {
+                        blockdev_ioctl_range($fh, BLKDISCARD, $discard_offset, $discard_length);
+                    };
+                    if (my $err = $@) {
+                        if ($discard_failures == 0) {
+                            log_warn("blkdiscard for $discard_length bytes at offset"
+                                . " $discard_offset failed: $err");
+                        }
+                        $discard_failures += 1;
+                    }
+                    $discard_offset = $written_total;
+                }
+
                 my $curr_time = clock_gettime(CLOCK_MONOTONIC);
                 if (($curr_time - $lastprint) >= 3) {
                     my $percent_finished = 100 * $written_total / $size;
@@ -467,6 +536,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 = $@;
@@ -475,14 +547,24 @@ my sub free_lvm_volumes_locked {
             die "$err";
         }
     };
-
-    # we need to zero out LVM data for security reasons and to allow thin provisioning
-    my $zero_out_worker = sub {
+    # we need to zero out LVM data for security reasons
+    # and discard images to free storage space to allow
+    # thin provisioning
+    my $cleanup_worker = sub {
 
         my $total_cleanup_errors = 0;
         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";
 
             eval {
                 # pass an errfunc here so that debug information is not by lvm to stderr,
@@ -490,13 +572,14 @@ my sub free_lvm_volumes_locked {
                 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",
                     errfunc => sub { },
+
                 );
                 $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",
                     errfunc => sub { },
                 );
             };
@@ -507,7 +590,17 @@ my sub free_lvm_volumes_locked {
                 next;
             }
 
-            eval { $secure_delete_cmd->($lvmpath); };
+            eval {
+                if ($scfg->{saferemove}) {
+                    $secure_delete_cmd->($lvmpath);
+
+                } elsif ($on_remove_opts->{discard}) {
+                    run_command(
+                        ['/sbin/blkdiscard', $lvmpath],
+                        errmsg => "blkdiscard '$lvmpath' error",
+                    );
+                }
+            };
             if (my $cleanup_err = $@) {
                 print STDERR "ERROR: cleanup failed for lv $name: $cleanup_err";
                 eval { rename_after_failed_cleanup($class, $scfg, $storeid, $vg, $name) };
@@ -532,13 +625,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"];
@@ -561,6 +654,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 => {
@@ -577,6 +700,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.",
@@ -602,6 +732,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 },
@@ -624,6 +755,99 @@ 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 = [
+        'lsblk', '--json', '--bytes', '--discard', '--nodeps', '--output', 'DISC-MAX',
+        $dev_path,
+    ];
+
+    eval {
+        run_command($cmd, outfunc => sub { $output .= "$_[0]\n"; });
+    };
+    if (my $err = $@) {
+        chomp $err;
+        raise_param_exc({
+            'on-volume-remove' => "discard on remove is enabled, but lsblk could not "
+                . "query discard support for the backing device '$dev_path': $err",
+        });
+    }
+
+    my $parsed = eval { decode_json($output) };
+    if (my $err = $@ || ref($parsed) ne 'HASH') {
+        chomp $err if $err;
+        raise_param_exc({
+            'on-volume-remove' => "discard on remove is enabled, but lsblk could not "
+                . "parse discard support for the backing device '$dev_path'"
+                . ($err ? ": $err" : ""),
+        });
+    }
+
+    my $blockdevices = $parsed->{blockdevices};
+    if (ref($blockdevices) ne 'ARRAY' || scalar($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'",
+        });
+
+    }
+
+    if (scalar($blockdevices->@*) > 1) {
+        raise_param_exc({
+            'on-volume-remove' => "discard on remove is enabled, but lsblk returned "
+                . "ambiguous discard support for the backing device '$dev_path'",
+        });
+    }
+
+    return $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/.+)$!) {
+                $dev_path = $1; # untaint
+            } else {
+                raise_param_exc({
+                    'on-volume-remove' => "discard on remove is enabled, but discard support "
+                        . "cannot be resolved for the backing device '$dev_path'",
+                });
+            }
+
+            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) = @_;
 
@@ -645,6 +869,8 @@ sub on_add_hook {
         lvm_create_volume_group($path, $scfg->{vgname}, $scfg->{shared});
     }
 
+    assert_discard_supported($scfg->{vgname}, $scfg->{'on-volume-remove'});
+
     return;
 }
 
@@ -665,6 +891,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