[libvirt] [PATCH] migration: convert speed from Mb/sec to bytes/sec in drive-mirror jobs

[Rudy Zhang]

On 16/3/31 上午5:47, John Ferlan wrote:
>
>
> On 03/28/2016 12:02 AM, Rudy Zhang wrote:
>> Block migration speed is differect from memory migration speed, because
>> it not convert speed from Mb/sec to bytes/sec in the drive-mirror job.
>>
>
> "different"
>
> Perhaps better stated:
>
> Commit id '7b7600b3' added qemuMigrationDriveMirror to handle NBD
> mirroring, but passed the migrate_speed listed in MiB/s to be used for
> the mirror_speed which expects a bytes/s value.
>
> This patch will convert the migrate_speed value to its mirror_speed
> equivalent.
>

It's better. Thank you.

>> Signed-off-by: Rudy Zhang <rudyflyzhang at gmail.com>
>> ---
>>   src/qemu/qemu_migration.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>
> Not my area of expertise, but since it was sitting here waiting for
> review. Had to dig a bit, but yes I see the passed 'speed' is the
> migration speed which yes, is in MiB/s...
>
> What I didn't dig on was whether the migrate bandwidth using a negative
> value (eg essentially unlimited) is passed around as a -1 or some other
> theoretically maximum. If it is, then the bit shift done here is going
> to have a problem as the value passed to qemuMonitorDriveMirror won't be
> right (look at the qemuDomainBlockRebase and see how it limits things).
>

bandwidth will not be negative value.

>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index 8bc76bf..7648d8c 100644
>> --- a/src/qemu/qemu_migration.c
>> +++ b/src/qemu/qemu_migration.c
>> @@ -2135,8 +2135,8 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
>
> My suggested adjustments:
>
> 1. Adjust name of parameter "speed" to "migrate_speed" and also comments
> into function to indicate new name and that it's MiB/s.
>
> 2. Add a local "unsigned long long mirror_speed = migrate_speed;"
>
> 3. Look at qemuDomainBlockRebase and how it checks to make sure the
> speed will fit when shifted... Not sure we want to error out at this
> point, but some thing like:
>
>      if (mirror_speed > LLONG_MAX >> 20)
>          mirror_speed = 0;  /* means unlimited */
>      else
>          mirror_speed <<= 20;
>

It is usefull, but speed can't be '0', it need return error to callers.

>>
>>           qemuBlockJobSyncBegin(disk);
>>           /* Force "raw" format for NBD export */
>> -        mon_ret = qemuMonitorDriveMirror(priv->mon, diskAlias, nbd_dest,
>> -                                         "raw", speed, 0, 0, mirror_flags);
>
> 4. Change the following to use "mirror_speed", I think for alignment
> stuff it'll be best to be:
>
>   mon_ret = qemuMonitorDriveMirror(priv->mon, diskAlias, nbd_dest, "raw",
>                                    mirror_speed, 0, 0, mirror_flags);
>
>
> Of course I could be totally off base and we just choose to use
> mirror_speed = 0 and forget all the adjustments and we won't need to
> pass migrate_speed either!
>
> John
>> +        mon_ret = qemuMonitorDriveMirror(priv->mon, diskAlias, nbd_dest,"raw",
>> +                        (unsigned long long)speed << 20, 0, 0, mirror_flags);
>>           VIR_FREE(diskAlias);
>>           VIR_FREE(nbd_dest);
>>
>>

I will change into patch v2, thank you for your reply.

-- 
Best regards,
Rudy Zhang