From 50853be2c5e3b23ae2453910e038dd220f92b9d0 Mon Sep 17 00:00:00 2001 From: Fabian Ebner Date: Fri, 15 Jan 2021 11:58:05 +0100 Subject: [PATCH] remove lock from is_base_and_used check 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 d8197c3..22a6ef1 100755 --- a/PVE/Storage.pm +++ b/PVE/Storage.pm @@ -496,8 +496,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); @@ -520,21 +527,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 @@ -918,7 +910,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.39.2