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

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

[Fiona Ebner]

Changes in v2:
* Suggest more generic command for checking whether zeroing is enabled
  in docs.
* Use full '--zero=y' in suggested command for activating zeroing.
* Expand comment about a potential future configuration option to
  support thin pools without 'zero'.

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 | 6 ++++++
 1 file changed, 6 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 | 19 ++++++++++++++++++-
 2 files changed, 22 insertions(+), 3 deletions(-)


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

-- 
Generated by git-murpp 0.5.0

[PATCH docs v2 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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/pve-storage-lvmthin.adoc b/pve-storage-lvmthin.adoc
index 83f65a3..03169e0 100644
--- a/pve-storage-lvmthin.adoc
+++ b/pve-storage-lvmthin.adoc
@@ -23,6 +23,12 @@ 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.
+Use `lvs --select 'lv_attr =~ ^t' --options lv_name,zero` and check that there
+is a value `zero` in the output below the `Zero` heading, for each thin pool
+that is used as a {pve} storage.
 
 Configuration
 ~~~~~~~~~~~~~
-- 
2.47.3

[RFC storage v2 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 5ebe811..f2fc2e8 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 v2 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 | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
index cadf343..6593799 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,23 @@ 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') {
+            # Should there be a dedicated storage configuration option to support thin pools without
+            # 'zero'? Upon activation, it could be enforced that the configuration setting and the
+            # effective setting for the pool match. That would allow cheap checking for 'zeroinit'
+            # support in volume_has_feature().
+            # FIXME: MAJOR VERSION: enforce?
+            log_warn("$storeid: thin pool without zeroing is not supported for VM images!"
+                . " Use 'lvchange --zero=y $vg_name/$thin_pool' to enable zeroing");
+        }
+    }
 }
 
 sub activate_volume {
-- 
2.47.3