Proxmox/pve-storage
5
0
Fork 0
mirror of git://git.proxmox.com/git/pve-storage.git synced 2025-01-25 06:03:53 +03:00
Code Releases Activity

remove lock from is_base_and_used check

Browse Source 
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>
This commit is contained in:
Fabian Ebner 2021-01-15 11:58:05 +01:00 committed by Thomas Lamprecht
parent 2949c537d9
commit 50853be2c5
1 changed files with 10 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
PVE/Storage.pm
View File

@ -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);