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)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 558FD68E97 for ; Fri, 15 Jan 2021 11:58:13 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4D17423C36 for ; Fri, 15 Jan 2021 11:58:13 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (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 id B483223C2E for ; Fri, 15 Jan 2021 11:58:12 +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 7A71F448FD for ; Fri, 15 Jan 2021 11:58:12 +0100 (CET) From: Fabian Ebner To: pve-devel@lists.proxmox.com Date: Fri, 15 Jan 2021 11:58:05 +0100 Message-Id: <20210115105805.19994-1-f.ebner@proxmox.com> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.008 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [storage.pm] Subject: [pve-devel] [PATCH storage] remove lock from is_base_and_used check 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: Fri, 15 Jan 2021 10:58:13 -0000 and squash the __no_lock-variant into it. This lock is not broad enough, because for a caller that plans to do or not do some storage operation based on the result of the check, the following could happen: 1. volume_is_base_and_used is called and the result is used to enter a branch 2. situation on the storage changes in the meantime 3. the branch chosen in 1. might not be the one that should be taken anymore This means that callers are responsible for locking, and luckily the existing callers do use their own locks already: 1. vdisk_free used the __no_lock-variant with a broader lock also covering the free operation. 2. vdisk_clone is not a caller, but is relevant and it does lock the storage 2. the calls during VM migration and VM destruction happen in the context of a locked VM config. Because the clone operation also locks the VM config, it cannot happen that a linked clone is created while the template VM is migrated away or destroyed or vice versa. And even if that were the case, the base disk would not be freed, because of what vdisk_free/vdisk_clone do. Signed-off-by: Fabian Ebner --- PVE/Storage.pm | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/PVE/Storage.pm b/PVE/Storage.pm index 76d17c6..cf80a91 100755 --- a/PVE/Storage.pm +++ b/PVE/Storage.pm @@ -498,8 +498,15 @@ sub check_volume_access { return undef; } -my $volume_is_base_and_used__no_lock = sub { - my ($scfg, $storeid, $plugin, $volname) = @_; +# NOTE: this check does not work for LVM-thin, where the clone -> base +# reference is not encoded in the volume ID. +# see note in PVE::Storage::LvmThinPlugin for details. +sub volume_is_base_and_used { + my ($cfg, $volid) = @_; + + my ($storeid, $volname) = parse_volume_id($volid); + my $scfg = storage_config($cfg, $storeid); + my $plugin = PVE::Storage::Plugin->lookup($scfg->{type}); my ($vtype, $name, $vmid, undef, undef, $isBase, undef) = $plugin->parse_volname($volname); @@ -522,21 +529,6 @@ my $volume_is_base_and_used__no_lock = sub { } } return 0; -}; - -# NOTE: this check does not work for LVM-thin, where the clone -> base -# reference is not encoded in the volume ID. -# see note in PVE::Storage::LvmThinPlugin for details. -sub volume_is_base_and_used { - my ($cfg, $volid) = @_; - - my ($storeid, $volname) = parse_volume_id($volid); - my $scfg = storage_config($cfg, $storeid); - my $plugin = PVE::Storage::Plugin->lookup($scfg->{type}); - - $plugin->cluster_lock_storage($storeid, $scfg->{shared}, undef, sub { - return &$volume_is_base_and_used__no_lock($scfg, $storeid, $plugin, $volname); - }); } # try to map a filesystem path to a volume identifier @@ -920,7 +912,7 @@ sub vdisk_free { $plugin->cluster_lock_storage($storeid, $scfg->{shared}, undef, sub { # LVM-thin allows deletion of still referenced base volumes! die "base volume '$volname' is still in use by linked clones\n" - if &$volume_is_base_and_used__no_lock($scfg, $storeid, $plugin, $volname); + if volume_is_base_and_used($cfg, $volid); my (undef, undef, undef, undef, undef, $isBase, $format) = $plugin->parse_volname($volname); -- 2.20.1