11:02 p.m.
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.
Signed-off-by: Rudy Zhang <rudyflyzhang(a)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);
VIR_FREE(diskAlias);
VIR_FREE(nbd_dest);
--
2.6.4
4:47 p.m.
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(a)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);
8:45 p.m.
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(a)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
12:30 p.m.
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(a)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
3152
days inactive
3156
days old
3 comments
3 participants
participants (3)
-
John Ferlan -
Ján Tomko -
Rudy Zhang