[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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.


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

> 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 */
        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!

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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]