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.
Signed-off-by: Rudy Zhang <rudyflyzhang@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).
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;
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);