From: Fabian Ebner Date: Thu, 27 Jan 2022 14:01:54 +0000 (+0100) Subject: api: move disk: fork before locking X-Git-Url: https://git.proxmox.com/?a=commitdiff_plain;h=bdf6ba1e7dafe469630c2f721de3d44482279b62;p=qemu-server.git api: move disk: fork before locking using the familiar early+repeated checks pattern from other API calls. Only intended functional changes are with regard to locking/forking. Signed-off-by: Fabian Ebner --- diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm index 38e08c88..59e083eb 100644 --- a/PVE/API2/Qemu.pm +++ b/PVE/API2/Qemu.pm @@ -3396,7 +3396,7 @@ __PACKAGE__->register_method({ my $storecfg = PVE::Storage::config(); - my $move_updatefn = sub { + my $load_and_check_move = sub { my $conf = PVE::QemuConfig->load_config($vmid); PVE::QemuConfig->check_lock($conf); @@ -3416,8 +3416,8 @@ __PACKAGE__->register_method({ $oldfmt = $1; } - die "you can't move to the same storage with same format\n" if $oldstoreid eq $storeid && - (!$format || !$oldfmt || $oldfmt eq $format); + die "you can't move to the same storage with same format\n" + if $oldstoreid eq $storeid && (!$format || !$oldfmt || $oldfmt eq $format); # this only checks snapshots because $disk is passed! my $snapshotted = PVE::QemuServer::Drive::is_volume_in_use( @@ -3429,6 +3429,13 @@ __PACKAGE__->register_method({ die "you can't move a disk with snapshots and delete the source\n" if $snapshotted && $param->{delete}; + return ($conf, $drive, $oldstoreid, $snapshotted); + }; + + my $move_updatefn = sub { + my ($conf, $drive, $oldstoreid, $snapshotted) = $load_and_check_move->(); + my $old_volid = $drive->{file}; + PVE::Cluster::log_msg( 'info', $authuser, @@ -3439,83 +3446,79 @@ __PACKAGE__->register_method({ PVE::Storage::activate_volumes($storecfg, [ $drive->{file} ]); - my $realcmd = sub { - my $newvollist = []; + my $newvollist = []; - eval { - local $SIG{INT} = - local $SIG{TERM} = - local $SIG{QUIT} = - local $SIG{HUP} = sub { die "interrupted by signal\n"; }; - - warn "moving disk with snapshots, snapshots will not be moved!\n" - if $snapshotted; - - my $bwlimit = extract_param($param, 'bwlimit'); - my $movelimit = PVE::Storage::get_bandwidth_limit( - 'move', - [$oldstoreid, $storeid], - $bwlimit - ); + eval { + local $SIG{INT} = + local $SIG{TERM} = + local $SIG{QUIT} = + local $SIG{HUP} = sub { die "interrupted by signal\n"; }; - my $newdrive = PVE::QemuServer::clone_disk( - $storecfg, - $vmid, - $running, - $disk, - $drive, - undef, - $vmid, - $storeid, - $format, - 1, - $newvollist, - undef, - undef, - undef, - $movelimit, - $conf, - ); - $conf->{$disk} = PVE::QemuServer::print_drive($newdrive); + warn "moving disk with snapshots, snapshots will not be moved!\n" + if $snapshotted; - PVE::QemuConfig->add_unused_volume($conf, $old_volid) if !$param->{delete}; + my $bwlimit = extract_param($param, 'bwlimit'); + my $movelimit = PVE::Storage::get_bandwidth_limit( + 'move', + [$oldstoreid, $storeid], + $bwlimit + ); - # convert moved disk to base if part of template - PVE::QemuServer::template_create($vmid, $conf, $disk) - if PVE::QemuConfig->is_template($conf); + my $newdrive = PVE::QemuServer::clone_disk( + $storecfg, + $vmid, + $running, + $disk, + $drive, + undef, + $vmid, + $storeid, + $format, + 1, + $newvollist, + undef, + undef, + undef, + $movelimit, + $conf, + ); + $conf->{$disk} = PVE::QemuServer::print_drive($newdrive); - PVE::QemuConfig->write_config($vmid, $conf); + PVE::QemuConfig->add_unused_volume($conf, $old_volid) if !$param->{delete}; - my $do_trim = PVE::QemuServer::get_qga_key($conf, 'fstrim_cloned_disks'); - if ($running && $do_trim && PVE::QemuServer::qga_check_running($vmid)) { - eval { mon_cmd($vmid, "guest-fstrim") }; - } + # convert moved disk to base if part of template + PVE::QemuServer::template_create($vmid, $conf, $disk) + if PVE::QemuConfig->is_template($conf); - eval { - # try to deactivate volumes - avoid lvm LVs to be active on several nodes - PVE::Storage::deactivate_volumes($storecfg, [ $newdrive->{file} ]) - if !$running; - }; - warn $@ if $@; - }; - if (my $err = $@) { - foreach my $volid (@$newvollist) { - eval { PVE::Storage::vdisk_free($storecfg, $volid) }; - warn $@ if $@; - } - die "storage migration failed: $err"; - } + PVE::QemuConfig->write_config($vmid, $conf); - if ($param->{delete}) { - eval { - PVE::Storage::deactivate_volumes($storecfg, [$old_volid]); - PVE::Storage::vdisk_free($storecfg, $old_volid); - }; - warn $@ if $@; + my $do_trim = PVE::QemuServer::get_qga_key($conf, 'fstrim_cloned_disks'); + if ($running && $do_trim && PVE::QemuServer::qga_check_running($vmid)) { + eval { mon_cmd($vmid, "guest-fstrim") }; } + + eval { + # try to deactivate volumes - avoid lvm LVs to be active on several nodes + PVE::Storage::deactivate_volumes($storecfg, [ $newdrive->{file} ]) + if !$running; + }; + warn $@ if $@; }; + if (my $err = $@) { + foreach my $volid (@$newvollist) { + eval { PVE::Storage::vdisk_free($storecfg, $volid) }; + warn $@ if $@; + } + die "storage migration failed: $err"; + } - return $rpcenv->fork_worker('qmmove', $vmid, $authuser, $realcmd); + if ($param->{delete}) { + eval { + PVE::Storage::deactivate_volumes($storecfg, [$old_volid]); + PVE::Storage::vdisk_free($storecfg, $old_volid); + }; + warn $@ if $@; + } }; my $load_and_check_reassign_configs = sub { @@ -3695,7 +3698,14 @@ __PACKAGE__->register_method({ die "cannot move disk '$disk', only configured disks can be moved to another storage\n" if $disk =~ m/^unused\d+$/; - return PVE::QemuConfig->lock_config($vmid, $move_updatefn); + + $load_and_check_move->(); # early checks before forking/locking + + my $realcmd = sub { + PVE::QemuConfig->lock_config($vmid, $move_updatefn); + }; + + return $rpcenv->fork_worker('qmmove', $vmid, $authuser, $realcmd); } else { my $msg = "both 'storage' and 'target-vmid' missing, either needs to be set"; raise_param_exc({ 'target-vmid' => $msg, 'storage' => $msg });