From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id AEBF991826 for ; Tue, 30 Jan 2024 11:00:52 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 917AE1FC9E for ; Tue, 30 Jan 2024 11:00:22 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 30 Jan 2024 11:00:19 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id B57414930C for ; Tue, 30 Jan 2024 11:00:19 +0100 (CET) Date: Tue, 30 Jan 2024 11:00:12 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20231219140306.188937-1-h.duerr@proxmox.com> <20231219140306.188937-3-h.duerr@proxmox.com> In-Reply-To: <20231219140306.188937-3-h.duerr@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1706608112.tdnyjesknw.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.064 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH pve-storage v4 2/2] fix #1611: implement import of base-images for LVM-thin Storage X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 30 Jan 2024 10:00:52 -0000 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 >=20 > Signed-off-by: Hannes Duerr > --- > src/PVE/Storage/LvmThinPlugin.pm | 50 ++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) >=20 > diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPl= ugin.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; > } > =20 > +sub volume_import { > + my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $bas= e_snapshot, $with_snapshots, $allow_rename) =3D @_; > + > + my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_forma= t) =3D > + $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 =3D $scfg->{vgname}; > + my $lvs =3D 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 differe= nt name\n"; > + > + $tempname =3D $class->find_free_diskname($storeid, $scfg, $vmid); > + } else { > + $tempname =3D $volname; > + $tempname =3D~ 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) =3D PVE::Storage::parse_volume_id($class->SUPER:= :volume_import( > + $scfg, > + $storeid, > + $fh, > + $tempname, > + $format, > + $snapshot, > + $base_snapshot, > + $with_snapshots, > + $allow_rename > + )); > + > + $volname =3D $class->create_base($storeid, $scfg, $newname); > + } > + > + return "$storeid:$volname"; > +} > + > # used in LVMPlugin->volume_import > sub volume_import_write { > my ($class, $input_fh, $output_file) =3D @_; > --=20 > 2.39.2 >=20 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20