[pve-devel] [PATCH v2 qemu-server 1/2] Refactor update_config_nolock -> write_config

Fabian Grünbichler f.gruenbichler at proxmox.com
Fri Feb 12 11:14:40 CET 2016


The method update_config wrapped update_config_nolock
using lock_config, but to prevent update races the whole
"read config", "do something", "write config" flow was
always protected by lock_config anyway, and update_config
was never called.

Thus, we can safely drop update_config and rename
update_config_nolock to write_config like in PVE::LXC .
---
 PVE/API2/Qemu.pm   | 26 +++++++++++++-------------
 PVE/CLI/qm.pm      |  2 +-
 PVE/QemuMigrate.pm |  6 +++---
 PVE/QemuServer.pm  | 41 +++++++++++++++++------------------------
 4 files changed, 34 insertions(+), 41 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 39bd248..c21287c 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -457,7 +457,7 @@ __PACKAGE__->register_method({
 			$conf->{smbios1} = "uuid=$uuid_str";
 		    }
 
-		    PVE::QemuServer::update_config_nolock($vmid, $conf);
+		    PVE::QemuServer::write_config($vmid, $conf);
 
 		};
 		my $err = $@;
@@ -908,7 +908,7 @@ my $update_vm_api  = sub {
 		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
 		    if (PVE::QemuServer::try_deallocate_drive($storecfg, $vmid, $conf, $opt, $drive, $rpcenv, $authuser)) {
 			delete $conf->{$opt};
-			PVE::QemuServer::update_config_nolock($vmid, $conf, 1);
+			PVE::QemuServer::write_config($vmid, $conf, 1);
 		    }
 		} elsif (PVE::QemuServer::valid_drivename($opt)) {
 		    &$check_protection($conf, "can't remove drive '$opt'");
@@ -916,10 +916,10 @@ my $update_vm_api  = sub {
 		    PVE::QemuServer::vmconfig_register_unused_drive($storecfg, $vmid, $conf, PVE::QemuServer::parse_drive($opt, $conf->{pending}->{$opt}))
 			if defined($conf->{pending}->{$opt});
 		    PVE::QemuServer::vmconfig_delete_pending_option($conf, $opt, $force);
-		    PVE::QemuServer::update_config_nolock($vmid, $conf, 1);
+		    PVE::QemuServer::write_config($vmid, $conf, 1);
 		} else {
 		    PVE::QemuServer::vmconfig_delete_pending_option($conf, $opt, $force);
-		    PVE::QemuServer::update_config_nolock($vmid, $conf, 1);
+		    PVE::QemuServer::write_config($vmid, $conf, 1);
 		}
 	    }
 
@@ -943,13 +943,13 @@ my $update_vm_api  = sub {
 		    $conf->{pending}->{$opt} = $param->{$opt};
 		}
 		PVE::QemuServer::vmconfig_undelete_pending_option($conf, $opt);
-		PVE::QemuServer::update_config_nolock($vmid, $conf, 1);
+		PVE::QemuServer::write_config($vmid, $conf, 1);
 	    }
 
 	    # remove pending changes when nothing changed
 	    $conf = PVE::QemuServer::load_config($vmid); # update/reload
 	    my $changes = PVE::QemuServer::vmconfig_cleanup_pending($conf);
-	    PVE::QemuServer::update_config_nolock($vmid, $conf, 1) if $changes;
+	    PVE::QemuServer::write_config($vmid, $conf, 1) if $changes;
 
 	    return if !scalar(keys %{$conf->{pending}});
 
@@ -2292,11 +2292,11 @@ __PACKAGE__->register_method({
 
 			$newconf->{$opt} = PVE::QemuServer::print_drive($vmid, $newdrive);
 
-			PVE::QemuServer::update_config_nolock($newid, $newconf, 1);
+			PVE::QemuServer::write_config($newid, $newconf, 1);
 		    }
 
 		    delete $newconf->{lock};
-		    PVE::QemuServer::update_config_nolock($newid, $newconf, 1);
+		    PVE::QemuServer::write_config($newid, $newconf, 1);
 
                     if ($target) {
 			# always deactivate volumes - avoid lvm LVs to be active on several nodes
@@ -2455,7 +2455,7 @@ __PACKAGE__->register_method({
 
 		    PVE::QemuServer::add_unused_volume($conf, $old_volid) if !$param->{delete};
 
-		    PVE::QemuServer::update_config_nolock($vmid, $conf, 1);
+		    PVE::QemuServer::write_config($vmid, $conf, 1);
 
 		    eval {
 			# try to deactivate volumes - avoid lvm LVs to be active on several nodes
@@ -2477,7 +2477,7 @@ __PACKAGE__->register_method({
                     if (PVE::QemuServer::is_volume_in_use($storecfg, $conf, undef, $old_volid)) {
 			warn "volume $old_volid still has snapshots, can't delete it\n";
 			PVE::QemuServer::add_unused_volume($conf, $old_volid);
-			PVE::QemuServer::update_config_nolock($vmid, $conf, 1);
+			PVE::QemuServer::write_config($vmid, $conf, 1);
 		    } else {
 			eval { PVE::Storage::vdisk_free($storecfg, $old_volid); };
 			warn $@ if $@;
@@ -2748,7 +2748,7 @@ __PACKAGE__->register_method({
 	    $drive->{size} = $newsize;
 	    $conf->{$disk} = PVE::QemuServer::print_drive($vmid, $drive);
 
-	    PVE::QemuServer::update_config_nolock($vmid, $conf, 1);
+	    PVE::QemuServer::write_config($vmid, $conf, 1);
 	};
 
         PVE::QemuServer::lock_config($vmid, $updatefn);
@@ -2953,7 +2953,7 @@ __PACKAGE__->register_method({
 
 	    $snap->{description} = $param->{description} if defined($param->{description});
 
-	     PVE::QemuServer::update_config_nolock($vmid, $conf, 1);
+	     PVE::QemuServer::write_config($vmid, $conf, 1);
 	};
 
 	PVE::QemuServer::lock_config($vmid, $updatefn);
@@ -3149,7 +3149,7 @@ __PACKAGE__->register_method({
 	    };
 
 	    $conf->{template} = 1;
-	    PVE::QemuServer::update_config_nolock($vmid, $conf, 1);
+	    PVE::QemuServer::write_config($vmid, $conf, 1);
 
 	    return $rpcenv->fork_worker('qmtemplate', $vmid, $authuser, $realcmd);
 	};
diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index 747e540..9a8f839 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -188,7 +188,7 @@ __PACKAGE__->register_method ({
 	    my $conf = PVE::QemuServer::load_config($vmid);
 	    delete $conf->{lock};
 	    delete $conf->{pending}->{lock} if $conf->{pending}; # just to be sure
-	    PVE::QemuServer::update_config_nolock($vmid, $conf, 1);
+	    PVE::QemuServer::write_config($vmid, $conf, 1);
 	});
 
 	return undef;
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 5da62eb..5cacb51 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -277,7 +277,7 @@ sub phase1 {
 
     # set migrate lock in config file
     $conf->{lock} = 'migrate';
-    PVE::QemuServer::update_config_nolock($vmid, $conf, 1);
+    PVE::QemuServer::write_config($vmid, $conf, 1);
 
     sync_disks($self, $vmid);
 
@@ -290,7 +290,7 @@ sub phase1_cleanup {
 
     my $conf = $self->{vmconf};
     delete $conf->{lock};
-    eval { PVE::QemuServer::update_config_nolock($vmid, $conf, 1) };
+    eval { PVE::QemuServer::write_config($vmid, $conf, 1) };
     if (my $err = $@) {
 	$self->log('err', $err);
     }
@@ -546,7 +546,7 @@ sub phase2_cleanup {
 
     my $conf = $self->{vmconf};
     delete $conf->{lock};
-    eval { PVE::QemuServer::update_config_nolock($vmid, $conf, 1) };
+    eval { PVE::QemuServer::write_config($vmid, $conf, 1) };
     if (my $err = $@) {
         $self->log('err', $err);
     }
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 1b5f81f..9dd4777 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2244,7 +2244,7 @@ sub write_vm_config {
     return $raw;
 }
 
-sub update_config_nolock {
+sub write_config {
     my ($vmid, $conf, $skiplock) = @_;
 
     check_lock($conf) if !$skiplock;
@@ -2254,12 +2254,6 @@ sub update_config_nolock {
     PVE::Cluster::cfs_write_file($cfspath, $conf);
 }
 
-sub update_config {
-    my ($vmid, $conf, $skiplock) = @_;
-
-    lock_config($vmid, &update_config_nolock, $conf, $skiplock);
-}
-
 sub load_defaults {
 
     my $res = {};
@@ -3219,7 +3213,7 @@ sub config_to_command {
 	    #if dimm_memory is not aligned to dimm map
 	    if($current_size > $memory) {
 	         $conf->{memory} = $current_size;
-	         update_config_nolock($vmid, $conf, 1);
+	         write_config($vmid, $conf, 1);
 	    }
 	});
     }
@@ -3876,7 +3870,7 @@ sub qemu_memory_hotplug {
 		}
 		#update conf after each succesful module hotplug
 		$conf->{memory} = $current_size;
-		update_config_nolock($vmid, $conf, 1);
+		write_config($vmid, $conf, 1);
 	});
 
     } else {
@@ -3901,7 +3895,7 @@ sub qemu_memory_hotplug {
 		$conf->{memory} = $current_size;
 
 		eval { qemu_objectdel($vmid, "mem-$name"); };
-		update_config_nolock($vmid, $conf, 1);
+		write_config($vmid, $conf, 1);
 	});
     }
 }
@@ -4154,7 +4148,7 @@ sub vmconfig_hotplug_pending {
     }
 
     if ($changes) {
-	update_config_nolock($vmid, $conf, 1);
+	write_config($vmid, $conf, 1);
 	$conf = load_config($vmid); # update/reload
     }
 
@@ -4205,7 +4199,7 @@ sub vmconfig_hotplug_pending {
 	    # save new config if hotplug was successful
 	    delete $conf->{$opt};
 	    vmconfig_undelete_pending_option($conf, $opt);
-	    update_config_nolock($vmid, $conf, 1);
+	    write_config($vmid, $conf, 1);
 	    $conf = load_config($vmid); # update/reload
 	}
     }
@@ -4263,7 +4257,7 @@ sub vmconfig_hotplug_pending {
 	    # save new config if hotplug was successful
 	    $conf->{$opt} = $value;
 	    delete $conf->{pending}->{$opt};
-	    update_config_nolock($vmid, $conf, 1);
+	    write_config($vmid, $conf, 1);
 	    $conf = load_config($vmid); # update/reload
 	}
     }
@@ -4319,16 +4313,16 @@ sub vmconfig_apply_pending {
 	$conf = load_config($vmid); # update/reload
 	if (!defined($conf->{$opt})) {
 	    vmconfig_undelete_pending_option($conf, $opt);
-	    update_config_nolock($vmid, $conf, 1);
+	    write_config($vmid, $conf, 1);
 	} elsif (valid_drivename($opt)) {
 	    vmconfig_delete_or_detach_drive($vmid, $storecfg, $conf, $opt, $force);
 	    vmconfig_undelete_pending_option($conf, $opt);
 	    delete $conf->{$opt};
-	    update_config_nolock($vmid, $conf, 1);
+	    write_config($vmid, $conf, 1);
 	} else {
 	    vmconfig_undelete_pending_option($conf, $opt);
 	    delete $conf->{$opt};
-	    update_config_nolock($vmid, $conf, 1);
+	    write_config($vmid, $conf, 1);
 	}
     }
 
@@ -4348,7 +4342,7 @@ sub vmconfig_apply_pending {
 	}
 
 	delete $conf->{pending}->{$opt};
-	update_config_nolock($vmid, $conf, 1);
+	write_config($vmid, $conf, 1);
     }
 }
 
@@ -5428,7 +5422,7 @@ sub rescan {
 
 	my $changes = update_disksize($vmid, $conf, $vm_volids);
 
-	update_config_nolock($vmid, $conf, 1) if $changes;
+	write_config($vmid, $conf, 1) if $changes;
     };
 
     if (defined($vmid)) {
@@ -5953,8 +5947,7 @@ my $snapshot_prepare = sub {
 	# always overwrite machine if we save vmstate. This makes sure we
 	# can restore it later using correct machine type
 	$snap->{machine} = get_current_qemu_machine($vmid) if $snap->{vmstate};
-
-	update_config_nolock($vmid, $conf, 1);
+	write_config($vmid, $conf, 1);
     };
 
     lock_config($vmid, $updatefn);
@@ -5990,7 +5983,7 @@ my $snapshot_commit = sub {
 
 	$newconf->{parent} = $snapname;
 
-	update_config_nolock($vmid, $newconf, 1);
+	write_config($vmid, $newconf, 1);
     };
 
     lock_config($vmid, $updatefn);
@@ -6069,7 +6062,7 @@ sub snapshot_rollback {
 	    delete $conf->{machine} if $snap->{vmstate} && !$has_machine_config;
 	}
 
- 	update_config_nolock($vmid, $conf, 1);
+	write_config($vmid, $conf, 1);
 
 	if (!$prepare && $snap->{vmstate}) {
 	    my $statefile = PVE::Storage::path($storecfg, $snap->{vmstate});
@@ -6277,7 +6270,7 @@ sub snapshot_delete {
 	    }
 	}
 
-	update_config_nolock($vmid, $conf, 1);
+	write_config($vmid, $conf, 1);
     };
 
     lock_config($vmid, $updatefn);
@@ -6355,7 +6348,7 @@ sub template_create {
 	my $voliddst = PVE::Storage::vdisk_create_base($storecfg, $volid);
 	$drive->{file} = $voliddst;
 	$conf->{$ds} = print_drive($vmid, $drive);
-	update_config_nolock($vmid, $conf, 1);
+	write_config($vmid, $conf, 1);
     });
 }
 
-- 
2.1.4





More information about the pve-devel mailing list