[pve-devel] [PATCH qemu-server 2/7] bwlimit: add parameter to qemu_drive_mirror

[Thomas Lamprecht]

On 3/29/19 8:28 AM, Stoiko Ivanov wrote:
> used for online drive migrations, move_disk and clone calls.
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> ---
>  PVE/QemuServer.pm | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index e60aa28..b8e96ed 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -6728,7 +6728,7 @@ sub qemu_img_format {
>  }
>  
>  sub qemu_drive_mirror {
> -    my ($vmid, $drive, $dst_volid, $vmiddst, $is_zero_initialized, $jobs, $skipcomplete, $qga) = @_;
> +    my ($vmid, $drive, $dst_volid, $vmiddst, $is_zero_initialized, $jobs, $skipcomplete, $qga, $bwlimit) = @_;
>  
>      $jobs = {} if !$jobs;
>  
> @@ -6755,7 +6755,13 @@ sub qemu_drive_mirror {
>      my $opts = { timeout => 10, device => "drive-$drive", mode => "existing", sync => "full", target => $qemu_target };
>      $opts->{format} = $format if $format;
>  
> -    print "drive mirror is starting for drive-$drive\n";
> +    if (defined($bwlimit)) {
> +	my $bwlimit_bps = $bwlimit*1024;
> +	$opts->{speed} = $bwlimit_bps;

pure nits for above, spaces before and after the multiplaction asteriks
makes things easier to read, also you should be able to do:
my $bwlimit_bps = $opts->{speed} = $bwlimit * 1024;

But we may also want to keep the KBytes/s or even do a "convert to biggest
unit that still has pre-decimal point digits?

PVE::VZDump module from manager has "format_size" which one could take as
base, may we want to add it to PVE::Tools, it'd fit next to the "convert_size"
method there.

> +	print "drive mirror is starting for drive-$drive with speed: $bwlimit_bps bytes/sec\n";

s/speed/bandwidth/ ? speed could indicate that this will be the speed it
runs, but it can be lower thus "bandwidth" or even "bandwidth limit" to
ensure people associated it with bwlimit setting could be a bit better?
No linguist though and just sharing my thoughts, take from it what seems
reasonable, patch looks OK besides that! (I applied a cleanup for the
drive-mirror exception handling below, last line in this diff context,
but rebase with three-way-merge handles that well :)

> +    } else {
> +	print "drive mirror is starting for drive-$drive\n";
> +    }
>  
>      eval { vm_mon_cmd($vmid, "drive-mirror", %$opts); }; #if a job already run for this device,it's throw an error
>  
>