[pve-devel] [PATCH storage] remove lock from is_base_and_used check

[pve-devel] [PATCH storage] remove lock from is_base_and_used check

[Fabian Ebner]

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 <f.ebner@proxmox.com>
---
 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

[pve-devel] applied: [PATCH storage] remove lock from is_base_and_used check

[Thomas Lamprecht]

On 15.01.21 11:58, Fabian Ebner wrote:
> 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 <f.ebner@proxmox.com>
> ---
>  PVE/Storage.pm | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)
> 
>

applied, thanks!