Re: [pve-devel] [PATCH pve-storage v4 2/2] fix #1611: implement import of base-images for LVM-thin Storage

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On December 19, 2023 3:03 pm, Hannes Duerr wrote:
> for base images we call the volume_import of the parent plugin and pass
> it as vm-image instead of base-image, then convert it back as base-image
> 
> Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
> ---
>  src/PVE/Storage/LvmThinPlugin.pm | 50 ++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
> index 1d2e37c..96f619b 100644
> --- a/src/PVE/Storage/LvmThinPlugin.pm
> +++ b/src/PVE/Storage/LvmThinPlugin.pm
> @@ -383,6 +383,56 @@ sub volume_has_feature {
>      return undef;
>  }
>  
> +sub volume_import {
> +    my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots, $allow_rename) = @_;
> +
> +    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_format) =
> +	$class->parse_volname($volname);
> +
> +    if (!$isBase) {
> +	return $class->SUPER::volume_import(
> +	    $scfg,
> +	    $storeid,
> +	    $fh,
> +	    $volname,
> +	    $format,
> +	    $snapshot,
> +	    $base_snapshot,
> +	    $with_snapshots,
> +	    $allow_rename
> +	);
> +    } else {
> +	my $tempname;
> +	my $vg = $scfg->{vgname};
> +	my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg);
> +	if ($lvs->{$vg}->{$volname}) {
> +	    die "volume $vg/$volname already exists\n" if !$allow_rename;
> +	    warn "volume $vg/$volname already exists - importing with a different name\n";
> +
> +	    $tempname = $class->find_free_diskname($storeid, $scfg, $vmid);
> +	} else {
> +	    $tempname = $volname;
> +	    $tempname =~ s/base/vm/;

couldn't we simply do this part unconditionally, and then call the SUPER
method? that one already checks whether $volname exists, does the same
die/warn and then unsets $volname in case of a collision, so that
alloc_image gets to pick a free one? then we just need to create_base on
the returned $volid and be done, making this a very thin wrapper..

but, I just realized a bigger issue with all of this code here - AFAICT
nothing ensures the storage is actually locked, so two parallel
imports (or an import racing with an alloc/..) could actually end up
trying to use the same volname. but we don't want to hold the lock for
the full duration of the import, just for the part covering the "find
free slot -> alloc_image" time span. this is not limited to LVM, but
properly handling it would require splitting up volume_import into two
plugin methods (breaking the storage plugin ABI) which might not even
work for all storage types.

> +	}
> +
> +	($storeid,my $newname) = PVE::Storage::parse_volume_id($class->SUPER::volume_import(
> +	    $scfg,
> +	    $storeid,
> +	    $fh,
> +	    $tempname,
> +	    $format,
> +	    $snapshot,
> +	    $base_snapshot,
> +	    $with_snapshots,
> +	    $allow_rename
> +	));
> +
> +	$volname = $class->create_base($storeid, $scfg, $newname);
> +    }
> +
> +    return "$storeid:$volname";
> +}
> +
>  # used in LVMPlugin->volume_import
>  sub volume_import_write {
>      my ($class, $input_fh, $output_file) = @_;
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>