[pve-devel] applied: [PATCH storage] Fix #2050: only provide 'conv=sparse' for LvmThin

[Thomas Lamprecht]

On 1/17/19 4:31 PM, Stoiko Ivanov wrote:
> LVMPlugin->volume_import (used during cross-node offline, storage migration)
> passed 'conv=sparse' to `dd`. This can lead to data-corruption, if the target
> volume is not zero-initialized.
> 
> Dropping the argument fixes the problem, but breaks keeping data sparse for
> LvmThinPlugin.
> This patch adds a sub to both Plugins for running the actual `dd` during
> volume_import.
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> ---
> We had a short discussion off-list with Oguz and Wolfgang - putting the
> complete dd command in a sub of its own enables us to potentially optimise it
> in the future - e.g. to use the actual data_block_size as bs= parameter.
> However for now I went with fixing the breakage and not optimizing the dd.
> 
> Steps for reproducing the issue:
> * create a cluster with (at least) 2 nodes A and B, with a free disk-device (/dev/sdx)
> * write a recognizable pattern to /dev/sdx on B:
>   `dd if=/dev/zero bs=10M | tr '\000' '\255' | dd of=/dev/sdb bs=10M` #all 00 become ad
>   (would be grateful for alternatives to the dd| tr| dd)
> * on both A and B create a lvm-vg (pvcreate, vgcreate)
> * add it as _not_ shared storage, which is available on nodes A and B
> * create a small guest on A
> * fill a file in the guest with zeros
>   `dd if=/dev/zero of=/zerofil bs=10M`
> * stop the guest, migrate it to B
> * start the guest - check that the file `/zerofil` contains `ad` instead of `00`
> 

Applied, thanks! I moved the reproducer into the commit message (valuable info) and
restructured it a bit.

Also, this is not only used for offline migration of VMs with local (LVM thing/thick)
disks but also when doing an online migration, as disks which seem to belong to the
VM, but are not referenced by it will get moved to there.

> 
>  PVE/Storage/LVMPlugin.pm     | 10 ++++++++--
>  PVE/Storage/LvmThinPlugin.pm |  7 +++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm
> index 72d7464..9ad7979 100644
> --- a/PVE/Storage/LVMPlugin.pm
> +++ b/PVE/Storage/LVMPlugin.pm
> @@ -620,8 +620,8 @@ sub volume_import {
>  	}
>  	my $file = $class->path($scfg, $volname, $storeid)
>  	    or die "internal error: failed to get path to newly allocated volume $volname\n";
> -	run_command(['dd', "of=$file", 'conv=sparse', 'bs=64k'],
> -	            input => '<&'.fileno($fh));
> +
> +	$class->volume_import_write($fh, $file);
>      };
>      if (my $err = $@) {
>  	eval { $class->free_image($storeid, $scfg, $volname, 0) };
> @@ -630,4 +630,10 @@ sub volume_import {
>      }
>  }
>  
> +sub volume_import_write {
> +    my ($class, $input_fh, $output_file) = @_;
> +    run_command(['dd', "of=$output_file", 'bs=64k'],
> +	input => '<&'.fileno($input_fh));
> +}
> +
>  1;
> diff --git a/PVE/Storage/LvmThinPlugin.pm b/PVE/Storage/LvmThinPlugin.pm
> index 122fb37..aafc202 100644
> --- a/PVE/Storage/LvmThinPlugin.pm
> +++ b/PVE/Storage/LvmThinPlugin.pm
> @@ -374,4 +374,11 @@ sub volume_has_feature {
>      return undef;
>  }
>  
> +# used in LVMPlugin->volume_import
> +sub volume_import_write {
> +    my ($class, $input_fh, $output_file) = @_;
> +    run_command(['dd', "of=$output_file", 'conv=sparse', 'bs=64k'],
> +	input => '<&'.fileno($input_fh));
> +}
> +
>  1;
>