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);

On Mon, Mar 28, 2016 at 12:02:09PM +0800, 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.

It might be worth noting that this was introduced by commit 08cc14f [1], released in libvirt 1.2.9, which removed the conversion from qemuMonitorDriveMirror but did not adjust all the callers.

Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com> --- src/qemu/qemu_migration.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

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,

qemuBlockJobSyncBegin(disk); /* Force "raw" format for NBD export */ - mon_ret = qemuMonitorDriveMirror(priv->mon, diskAlias, nbd_dest, - "raw", speed, 0, 0, mirror_flags); + mon_ret = qemuMonitorDriveMirror(priv->mon, diskAlias, nbd_dest,"raw", + (unsigned long long)speed << 20, 0, 0, mirror_flags);

This could overflow if sizeof(unsigned long) == sizeof(unsigned long long), but we don't seem to mind taking the bandwith passed by virTypedParams in an unsigned long long type and passing it to qemuMigrationRun as unsigned long. This comment of qemuMigrationDriveMirror: * @speed: how much should the copying be limited could also be ajdusted to include the units. I would push the patch but I do not have the time to try it out now. Jan [1] http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=08cc14f