[PATCH-SERIES docs/storage 0/3] close #7204: document and check that thin pools have zeroing enabled

[PATCH-SERIES docs/storage 0/3] close #7204: document and check that thin pools have zeroing enabled

[Fiona Ebner]

There were a bunch of new reports in the last week. Not sure why,
maybe a bogus tutorial out there or something along those lines.

Thin pools with zeroing during allocation turned off are currently not
supported for VM images. Using such a pool may lead to data
corruption for various operations like drive mirror.

Document this and add a check in the storage plugin upon storage
activation.

In the future, a dedicated storage configuration option could be
introduced that can be checked by volume_has_feature() and enforced
upon storage activation.

docs:

Fiona Ebner (1):
  close #7204: storage: lvm thin: document that thin pools must have
    zeroing enabled

 pve-storage-lvmthin.adoc | 5 +++++
 1 file changed, 5 insertions(+)


storage:

Fiona Ebner (2):
  lvm plugin: list volumes: also query 'zero' property
  close #7204: lvm thin plugin: die if thin pool does not have zeroing
    configured

 src/PVE/Storage/LVMPlugin.pm     |  6 ++++--
 src/PVE/Storage/LvmThinPlugin.pm | 17 ++++++++++++++++-
 2 files changed, 20 insertions(+), 3 deletions(-)


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

-- 
Generated by git-murpp 0.5.0

[PATCH docs 1/3] close #7204: storage: lvm thin: document that thin pools must have zeroing enabled

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 pve-storage-lvmthin.adoc | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/pve-storage-lvmthin.adoc b/pve-storage-lvmthin.adoc
index 83f65a3..48505a9 100644
--- a/pve-storage-lvmthin.adoc
+++ b/pve-storage-lvmthin.adoc
@@ -23,6 +23,11 @@ lvcreate -L 100G -n data pve
 lvconvert --type thin-pool pve/data
 ----
 
+WARNING: The `zero` property for the thin pool must be set or certain operations
+for VM images may lead to data corruption! The property is controlled via the
+`allocation/thin_pool_zero` LVM configuration option and is enabled by default.
+Check with `lvs pve/data -o lv_name,zero` which should indicate `zero` in the
+output below the `Zero` heading.
 
 Configuration
 ~~~~~~~~~~~~~
-- 
2.47.3

[RFC storage 2/3] lvm plugin: list volumes: also query 'zero' property

[Fiona Ebner]

Useful to check whether thin pools are correctly configured.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage/LVMPlugin.pm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 32a8339..ad0d040 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -207,8 +207,8 @@ sub lvm_vgs {
 sub lvm_list_volumes {
     my ($vgname) = @_;
 
-    my $option_list =
-        'vg_name,lv_name,lv_size,lv_attr,pool_lv,data_percent,metadata_percent,snap_percent,uuid,tags,metadata_size,time';
+    my $option_list = 'vg_name,lv_name,lv_size,lv_attr,pool_lv,data_percent,metadata_percent,'
+        . 'snap_percent,uuid,tags,metadata_size,time,zero';
 
     my $cmd = [
         '/sbin/lvs',
@@ -248,6 +248,7 @@ sub lvm_list_volumes {
                 $tags,
                 $meta_size,
                 $ctime,
+                $zero,
             ) = split(':', $line);
             return if !$vg_name;
             return if !$lv_name;
@@ -262,6 +263,7 @@ sub lvm_list_volumes {
             $d->{pool_lv} = $pool_lv if $pool_lv;
             $d->{tags} = $tags if $tags;
             $d->{ctime} = $ctime;
+            $d->{zero} = $zero;
 
             if ($lv_type eq 't') {
                 $data_percent ||= 0;
-- 
2.47.3

[RFC storage 3/3] close #7204: lvm thin plugin: die if thin pool does not have zeroing configured

[Fiona Ebner]

Thin pools with zeroing during allocation turned off are currently not
supported for VM images. Using such a pool may lead to data
corruption for various operations like drive mirror.

In the future, a dedicated storage configuration option could be
introduced that can be checked by volume_has_feature() and enforced
upon storage activation.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

RFC - it is rather noisy since pvestatd will log it every time, OTOH
it may cause data corruption so being noisy can be justified. An
alternative might be to just automatically set it after detecting that
the flag is missing and only warn once, mentioning that that this is
done, but might be too much automagic.

 src/PVE/Storage/LvmThinPlugin.pm | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
index 256ec43..dae40b6 100644
--- a/src/PVE/Storage/LvmThinPlugin.pm
+++ b/src/PVE/Storage/LvmThinPlugin.pm
@@ -6,6 +6,7 @@ use warnings;
 use IO::File;
 
 use PVE::JSONSchema qw(get_standard_option);
+use PVE::RESTEnvironment qw(log_warn);
 use PVE::Tools qw(run_command trim);
 
 use PVE::Storage::Plugin;
@@ -263,7 +264,21 @@ sub activate_storage {
 
     $class->SUPER::activate_storage($storeid, $scfg, $cache);
 
-    $activate_lv->($scfg->{vgname}, $scfg->{thinpool}, $cache);
+    my ($vg_name, $thin_pool) = $scfg->@{qw(vgname thinpool)};
+
+    $activate_lv->($vg_name, $thin_pool, $cache);
+
+    if ($scfg->{content}->{images}) {
+        my $lvs = $cache->{lvs} ||= PVE::Storage::LVMPlugin::lvm_list_volumes();
+        my $thin_pool_zero = $lvs->{$vg_name}->{$thin_pool}->{zero};
+        if (!defined($thin_pool_zero) || $thin_pool_zero ne 'zero') {
+            # TODO Add dedicated storage configuration option to support thin pools without zero to
+            # allow cheap checking in volume_has_feature() and check that it matches here.
+            # TODO PVE 10 - enforce?
+            log_warn("$storeid: thin pool without zeroing is not supported for VM images!"
+                . " Use 'lvchange -Z y $vg_name/$thin_pool' to enable zeroing");
+        }
+    }
 }
 
 sub activate_volume {
-- 
2.47.3