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

[Maximiliano Sandoval <m.sandoval@proxmox.com>]

Fiona Ebner <f.ebner@proxmox.com> writes:

> 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");

nit: If there is a new version, can we use the full --zero=y flag here?

> +        }
> +    }
>  }
>  
>  sub activate_volume {

-- 
Maximiliano