[PATCH storage v8 3/5] 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 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