With a shared LVM storage, parallel imports, which might be done in the context of remote migration, could lead to metadata corruption with unlucky timing, because of missing locking. Add locking around allocation and removal, which are the sections that modify LVM metadata. Note that other plugins suffer from missing locking here as well, but only regarding naming conflicts. Adding locking around the full call to volume_import() would mean locking for much too long. Other plugins could follow the approach here, or there could be a reservation approach like proposed in [0].
[0]: https://lore.proxmox.com/pve-devel/[email protected]/ Signed-off-by: Fiona Ebner <[email protected]> --- Better viewed with "-w" or "-w --word-diff=color --word-diff-regex='\w+'". src/PVE/Storage/LVMPlugin.pm | 37 +++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm index f5a2008..c5f71a2 100644 --- a/src/PVE/Storage/LVMPlugin.pm +++ b/src/PVE/Storage/LVMPlugin.pm @@ -1300,19 +1300,27 @@ sub volume_import { die "cannot import format $format into a file of format $file_format\n" if $file_format ne 'raw'; - my $vg = $scfg->{vgname}; - my $lvs = 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 different name\n"; - $name = undef; - } + my $allocname = $class->cluster_lock_storage( + $storeid, + $scfg->{shared}, + undef, + sub { + my $vg = $scfg->{vgname}; + my $lvs = 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 different name\n"; + $name = undef; + } - my ($size) = PVE::Storage::Plugin::read_common_header($fh); - $size = PVE::Storage::Common::align_size_up($size, 1024) / 1024; + my ($size) = PVE::Storage::Plugin::read_common_header($fh); + $size = PVE::Storage::Common::align_size_up($size, 1024) / 1024; + + return $class->alloc_image($storeid, $scfg, $vmid, 'raw', $name, $size); + }, + ); eval { - my $allocname = $class->alloc_image($storeid, $scfg, $vmid, 'raw', $name, $size); my $oldname = $volname; $volname = $allocname; if (defined($name) && $allocname ne $oldname) { @@ -1324,7 +1332,14 @@ sub volume_import { $class->volume_import_write($fh, $file); }; if (my $err = $@) { - my $cleanup_worker = eval { $class->free_image($storeid, $scfg, $volname, 0) }; + my $cleanup_worker = eval { + return $class->cluster_lock_storage( + $storeid, + $scfg->{shared}, + undef, + sub { return $class->free_image($storeid, $scfg, $volname, 0); }, + ); + }; warn $@ if $@; fork_cleanup_worker($cleanup_worker); die $err; -- 2.47.3 _______________________________________________ pve-devel mailing list [email protected] https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
