[pve-devel] [PATCH] Fix/cleanup disk hotplug

[Alexandre DERUMIER]

Sorry,I'll correct that.


About the recursive sub, do you prefer I simply make a "sleep X" to wait for device ?

I have add the recursive sub for wait as little as possible when we remove a device.
(Sometime I can take 4-5 seconds on removal, depend on guest load)



----- Mail original ----- 

De: "Dietmar Maurer" <dietmar at proxmox.com> 
À: "Derumier Alexandre" <aderumier at odiso.com>, pve-devel at pve.proxmox.com 
Envoyé: Jeudi 13 Octobre 2011 07:31:46 
Objet: RE: [pve-devel] [PATCH] Fix/cleanup disk hotplug 

Please can you fix the following issues. 

Btw, you changes contains trailing white spaces at some lines. 

> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm index 
> d72e07b..a97db4a 100644 
> --- a/PVE/QemuServer.pm 
> +++ b/PVE/QemuServer.pm 
> @@ -1265,7 +1265,7 @@ sub touch_config { } 
> 
> sub create_disks { 
> - my ($storecfg, $vmid, $settings) = @_; 
> + my ($storecfg, $vmid, $settings, $conf) = @_; 

coding style - please use whit space between parameters (and after 'my') 

my ($deviceid, $vmid, $action, $time) = @_; 

> 
> my $vollist = []; 
> 
> @@ -1302,7 +1302,7 @@ sub create_disks { 
> die "image '$path' does not exists\n"; 
> } 
> } 
> - PVE::QemuServer::vm_deviceadd($storecfg, $vmid, $ds, $disk); 
> + PVE::QemuServer::vm_deviceadd($storecfg, $conf, $vmid, $ds, $disk) 
> +if defined($conf); 
> }); 
> }; 
> 
> @@ -2272,47 +2272,75 @@ sub vm_devices_list { 
> return $devices; 
> } 
> 
> +sub vm_waitfordevicecleanup { 
> + my($deviceid,$vmid,$action,$time) = @_; 
> + 
> + return undef if $time > 5; 
> + 
> + my $devices_list = vm_devices_list($vmid); 
> + if($action eq 'add') { 
> + return 1 if defined($devices_list->{$deviceid}); 
> + } 
> + elsif ($action eq 'del') { 
> + return 1 if !defined($devices_list->{$deviceid}); 
> + } 
> + sleep 1; #give a litlle time to os to add the device 
> + $time++; 

Can you remove that recursive function call? 

> + vm_waitfordevicecleanup($deviceid,$vmid,$action,$time); 
> + 
> +} 
> + 
> sub vm_deviceadd { 
> - my ($storecfg, $vmid, $deviceid, $device) = @_; 
> + my ($storecfg, $conf, $vmid, $deviceid, $device) = @_; 
> 
> - my $cfspath = cfs_config_path($vmid); 
> - my $conf = PVE::Cluster::cfs_read_file($cfspath) || {}; 
> 
> - return if !check_running($vmid) || !$conf->{hotplug}; 
> + return if !check_running ($vmid) || $conf->{hotplug} != 1 ; # do 
> + nothing if vm is running or hotplug option not set to 1 

I would not use such inline comment - the code is quite obvious. 

> 
> if($deviceid =~ m/^(virtio)(\d+)$/) { 
> 
> - my $drive = print_drive_full($storecfg, $vmid, $device); 
> - vm_monitor_command($vmid, "drive_add auto $drive", 1); 
> - my $devicefull = print_drivedevice_full($storecfg, $vmid, $device); 
> - vm_monitor_command($vmid, "device_add $devicefull", 1); 
> + my $drive = print_drive_full ($storecfg, $vmid, $device); 

I already fixed the coding style, and you revert that back? Please change. 

> + my $ret = vm_monitor_command ($vmid, "drive_add auto $drive", 1); 
> + # If the command succeeds qemu prints: "OK" 
> + if ($ret !~ m/OK/s) { 
> + die "adding drive failed: $ret"; 
> + } 
> + 
> + my $devicefull = print_drivedevice_full ($storecfg, $vmid, $device); 

no space after function name please. 

> + $ret = vm_monitor_command ($vmid, "device_add $devicefull", 1); 
> + $ret =~ s/^\s+//; 
> + # Otherwise, if the command succeeds, no output is sent. So any non- 
> empty string shows an error 
> + die 'error on hotplug device : $ret' if $ret ne ""; 
> } 
> 
> - #verification 
> - sleep 2; #give a litlle time to os to add the device 
> - my $devices_list = vm_devices_list($vmid); 
> - die "error on hotplug device" if !defined($devices_list->{$deviceid}); 
> - 
> + #need to verify the device is correctly remove as device_add is async and 
> empty result return is not reliable 
> + die "error on hotplug device $deviceid" if (!defined 
> + (vm_waitfordevicecleanup($deviceid, $vmid, 'add', 0))); 
> } 
> 
> sub vm_devicedel { 
> - my ($vmid,$deviceid) = @_; 
> + my ($vmid,$conf,$deviceid) = @_; 

coding stype (white space). 

> 
> - my $cfspath = cfs_config_path($vmid); 
> - my $conf = PVE::Cluster::cfs_read_file($cfspath) || {}; 
> 
> - return if !check_running ($vmid) || !$conf->{hotplug}; 
> + return if !check_running ($vmid) || $conf->{hotplug} != 1 ; 
> 
> if($deviceid =~ m/^(virtio)(\d+)$/){ 
> 
> - vm_monitor_command($vmid, "drive_del drive-$deviceid", 1); 
> - vm_monitor_command($vmid, "device_del $deviceid", 1); 
> + my $ret = vm_monitor_command ($vmid, "drive_del drive-$deviceid",1); 
> + $ret =~ s/^\s+//; 
> + if ($ret =~ m/Device \'.*?\' not found/s) { 
> + # NB: device not found errors mean the drive was auto-deleted and we 
> ignore the error 
> + } 
> + elsif ($ret ne "") { 
> + die "deleting drive $deviceid failed : $ret"; 
> + } 
> + 
> + $ret = vm_monitor_command ($vmid, "device_del $deviceid",1); 
> + $ret =~ s/^\s+//; 
> + die 'detaching device $deviceid failed : $ret' if $ret ne ""; 
> 
> } 
> + #need to verify the device is correctly remove as device_del is async and 
> empty return is not reliable 
> + die "error on hot-unplugging device $deviceid" if (!defined 
> + (vm_waitfordevicecleanup($deviceid, $vmid, 'del', 0))); 
> 
> - sleep 2; 
> - my $devices_list = vm_devices_list($vmid); 
> - die "error on hot-unplugging device " if defined($devices_list->{$deviceid}); 
> } 
> 
> sub vm_start { 
> -- 
> 1.7.2.5 
> 
> _______________________________________________ 
> pve-devel mailing list 
> pve-devel at pve.proxmox.com 
> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel 





-- 

-- 




	
	Alexandre Derumier 
Ingénieur système 
e-mail : aderumier at odiso.com 
Tél : +33 (0)3 20 68 88 90 
Fax : +33 (0)3 20 68 90 81 
45 Bvd du Général Leclerc 
59100 ROUBAIX - FRANCE 













-------------- next part --------------
A non-text attachment was scrubbed...
Name: aderumier.vcf
Type: text/x-vcard
Size: 183 bytes
Desc: not available
URL: <http://lists.proxmox.com/pipermail/pve-devel/attachments/20111013/658d9b80/attachment.vcf>