[pve-devel] [PATCH storage 4/5] lvmthin: import/export: implement snapshot exporting

[Dietmar Maurer]

question inline:

> On June 21, 2017 at 2:59 PM Wolfgang Bumiller <w.bumiller at proxmox.com> wrote:
> 
> 
> ---
>  PVE/Storage/LVMPlugin.pm     | 11 ++++++++---
>  PVE/Storage/LvmThinPlugin.pm | 15 +++++++++++++++
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm
> index 9633e4c..ed8afa1 100644
> --- a/PVE/Storage/LVMPlugin.pm
> +++ b/PVE/Storage/LVMPlugin.pm
> @@ -509,15 +509,14 @@ sub volume_export_formats {
>      return volume_import_formats($class, $scfg, $storeid, $volname,
> $base_snapshot, $with_snapshots);
>  }
>  
> -sub volume_export {
> +sub __volume_export {
>      my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot,
> $base_snapshot, $with_snapshots) = @_;
>      die "volume export format $format not available for $class\n"
>  	if $format ne 'raw+size';
>      die "cannot export volumes together with their snapshots in $class\n"
>  	if $with_snapshots;
> -    die "cannot export a snapshot in $class\n" if defined($snapshot);
>      die "cannot export an incremental stream in $class\n" if
> defined($base_snapshot);
> -    my $file = $class->path($scfg, $volname, $storeid);
> +    my $file = $class->path($scfg, $volname, $storeid, $snapshot);
>      my $size;
>      # should be faster than querying LVM, also checks for the device file's
> availability
>      run_command(['/sbin/blockdev', '--getsize64', $file], outfunc => sub {
> @@ -529,6 +528,12 @@ sub volume_export {
>      run_command(['dd', "if=$file", "bs=64k"], output => '>&'.fileno($fh));
>  }

When do we need this functionality (exporting a single snapshot)?

> +sub volume_export {
> +    my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot,
> $base_snapshot, $with_snapshots) = @_;
> +    die "cannot export a snapshot in $class\n" if defined($snapshot);
> +    __volume_export($class, $scfg, $storeid, $fh, $volname, $format,
> $snapshot, $base_snapshot, $with_snapshots);
> +}
> +
>  sub volume_import_formats {
>      my ($class, $scfg, $storeid, $volname, $base_snapshot, $with_snapshots) =
> @_;
>      return () if $with_snapshots; # not supported
> diff --git a/PVE/Storage/LvmThinPlugin.pm b/PVE/Storage/LvmThinPlugin.pm
> index ccf5b7b..3c55db4 100644
> --- a/PVE/Storage/LvmThinPlugin.pm
> +++ b/PVE/Storage/LvmThinPlugin.pm
> @@ -368,4 +368,19 @@ sub volume_has_feature {
>      return undef;
>  }
>  
> +sub volume_export_formats {
> +    my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot,
> $with_snapshots) = @_;
> +    # The difference to LVM-non-thin is that we *can* export snapshots
> +    return $class->SUPER($class, $scfg, $storeid, $volname, $base_snapshot,
> $with_snapshots);
> +}
> +
> +sub volume_export {
> +    my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot,
> $base_snapshot, $with_snapshots) = @_;
> +    # LVMPlugin checks the $snapshot parameter here, we don't.
> +    # But we need to activate them:
> +    $class->activate_volume($storeid, $scfg, $volname, $snapshot) if
> defined($snapshot);

I do not like this, but OK if required.

> +    PVE::Storage::LVMPlugin::__volume_export($class, $scfg, $storeid, $fh,
> $volname, $format, $snapshot, $base_snapshot, $with_snapshots);
> +    $class->deactivate_volume($storeid, $scfg, $volname, $snapshot) if
> defined($snapshot);

But IMHO deactivate is wrong - what if someone else wants to access the
snapshot?