[PATCH manager/storage v6 0/2] fix #7339: lvmthick: add option to free storage for deleted VMs

[PATCH manager/storage v6 0/2] 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 would be to use 'blkdiscard'. This will issue discard
for all the blocks of the device and therefore free the storage on the
san [3]. As this option does not require changing any LVM metadata it
can be executed with a storage lock.

There is already a setting for  'saferemove', which zeros-out
to-be-deleted LVs using 'blkdiscard --zeroout' as a worker, but it does
not discard the blocks afterwards. This, similarly to just 'blkdiscard',
does not require a cluster-wide lock and therefore can be executed
without running into the 60s timeout.

This series extends the 'saferemove' worker so that it can execute
'blkdiscard', 'blkdiscard --zeroout' or both of them together and adds
an option to select this in the Gui.

Following Fabian's feedback [4] blkdiscard is configured through
the 'on-volume-remove' property string. The frontend serializes the
selected option into that property string, which is then passed to the
backend and parsed there.

This layout leaves room to unify additional volume-removal options in
the future. In a later major release, existing options such as
'saferemove', 'saferemove-stepsize', and 'saferemove_throughput' can be
folded into 'on-volume-remove'. This is not done now in order to avoid
changing existing storage configurations, but the required structure is
introduced by this series.

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/


storage:

Lukas Sichert (1):
  fix #7339: lvmthick: add worker to free space of to be deleted VMs

 src/PVE/Storage/LVMPlugin.pm | 78 ++++++++++++++++++++++++++++++++----
 1 file changed, 71 insertions(+), 7 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 | 48 +++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)


Summary over all repositories:
  3 files changed, 120 insertions(+), 8 deletions(-)

-- 
Generated by murpp 0.10.0

[PATCH storage v6 1/2] fix #7339: lvmthick: add worker to free space of to be deleted VMs

[Lukas Sichert]

Currently when deleting a VM whose disk is stored on a
thinly-provisioned LUN there is no way to also free the storage space
used by the VM. This is because the current implementation only calls
'lvremove'. This command deletes the LVM meta-data for the disk, but it
does not send discards to the SAN. 'lvmremove' can also be used with
'issue_discards', but since LVM meta-data is changed, it needs to be
done under a cluster-wide lock, which can lead to timeouts. There is
already an option to enable 'saferemove', which executes 'blkdiscard
--zeroout' to override the whole storage space allocated to the disk
with zeros. However it does not free the storage space.[1]

To add the functionality that frees the storage space, adjust the worker
in the code that is already there for zeroing out. In the worker parse
the storage config for the 'on-volume-remove' property string and check
if the 'discard' property is set. If the option is enabled execute
'blkdiscard'. This can also be executed in combination with 'blkdiscard
--zeroout' to first zero out the disk and then free the storage space
[1]. To allow the frontend to pass the `on-volume-remove` property
string to the backend, add it to the LVM storage plugin schema with the
appropriate format and description, so it is accepted in API requests.
As key and type validation is already enforced by the JSON schema, only
check that `on-volume-remove` is not empty.

[1] https://man7.org/linux/man-pages/man8/blkdiscard.8.html

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

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 443d292..472176c 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -13,6 +13,7 @@ use PVE::Tools qw(run_command file_read_firstline trim);
 
 use PVE::Storage::Common;
 use PVE::Storage::Plugin;
+use PVE::SafeSyslog;
 
 use base qw(PVE::Storage::Plugin);
 
@@ -349,13 +350,28 @@ my sub free_lvm_volumes_locked {
             warn $@ if $@;
         }
     };
-
+    my $on_remove_opts;
+    if ($scfg->{'on-volume-remove'}) {
+        $on_remove_opts =
+            PVE::JSONSchema::parse_property_string('on-volume-remove', $scfg->{'on-volume-remove'});
+    }
     # 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 data and discard (TRIM)';
+            } elsif ($scfg->{saferemove}) {
+                $discard_action = 'zero-out data on';
+            } elsif ($on_remove_opts->{'discard'}) {
+                $discard_action = 'discard (TRIM)';
+            }
+            print "$discard_action image $name ($lvmpath)\n";
 
             my $cmd_activate = ['/sbin/lvchange', '-aly', $lvmpath];
             run_command(
@@ -367,8 +383,18 @@ my sub free_lvm_volumes_locked {
                 $cmd_activate,
                 errmsg => "can't refresh LV '$lvmpath' to zero-out its data",
             );
+            syslog('info', "starting to $discard_action $name ($lvmpath)")
+                if defined($discard_action);
 
-            $secure_delete_cmd->($lvmpath);
+            if ($scfg->{saferemove}) {
+                print "zero-out data on image $name ($lvmpath)\n";
+                $secure_delete_cmd->($lvmpath);
+            }
+            if ($on_remove_opts->{'discard'}) {
+                print "discard image $name ($lvmpath)\n";
+                eval { run_command(['/sbin/blkdiscard', $lvmpath]); };
+                warn $@ if $@;
+            }
 
             $class->cluster_lock_storage(
                 $storeid,
@@ -383,13 +409,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"];
@@ -412,6 +438,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 => {
@@ -428,6 +484,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.",
@@ -453,6 +516,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 },
-- 
2.47.3

[PATCH manager v6 2/2] fix #7339: lvmthick: ui: add UI option to free storage

[Lukas Sichert]

In the commit 'fix #7339: lvmthick: add worker to free space of to be
deleted VMs' for the 'pve-storage' repo the backend received the
functionality to discard allocated space of a VM disk on a SAN, when a
VM is deleted. The backend checks whether to use this option by parsing
storage.cfg for the 'on-volume-remove' property string and checking if
the 'discard' property is set. The variable 'on-volume-remove' will
automatically be stored into the config file if it is present in the
'PUT' API request.

Expose this option in the GUI by adding a checkbox mapped to the local
form field `on-remove-discard`. Use `cbind` with `isCreate` so the
checkbox is enabled by default only when creating a new LVM storage. Map
`on-remove-discard` to the `discard` property of the `on-volume-remove`
property string. If at least one `on-volume-remove` option is enabled,
serialize the selected options into `on-volume-remove` and include it in
the returned API parameters.
Also rename the 'imgdel' task description from 'Erase data' to 'Destroy
image', since the task can now also discard storage blocks in addition to
zeroing data.

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

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 2ed4e65d..0378ecac 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -2179,7 +2179,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..bdf33ca2 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,20 @@ Ext.define('PVE.storage.LVMInputPanel', {
             uncheckedValue: 0,
             fieldLabel: gettext('Wipe Removed Volumes'),
         },
+        {
+            xtype: 'proxmoxcheckbox',
+            name: 'on-remove-discard',
+            uncheckedValue: 0,
+            cbind: {
+                checked: '{isCreate}',
+            },
+            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