Re: [Qemu-devel] [PATCH/RFC] qemu-img: show image conversion speed

[Stefan Hajnoczi]

On Mon, Nov 18, 2013 at 07:25:09PM +0530, Varad Gautam wrote:
> Calculate and display write speed when converting image with the
> -p parameter. qemu-progress:qemu_progress_print() now takes speed
> parameter to print.

How well does this approach work in your testing?  Calculating a new
speed for every I/O request could lead to very volatile output.  If the
value jumps around too much it becomes unusable; "moving averages" solve
this problem.

Adding speed with hardcoded 'kB/s' units in qemu-progress.c is
unfortunate.  qemu-progress.c does not know about units.  Perhaps the
speed units should be passed in when providing speed values.

> @@ -945,7 +945,7 @@ static int img_compare(int argc, char **argv)
>      filename2 = argv[optind++];
>  
>      /* Initialize before goto out */
> -    qemu_progress_init(progress, 2.0);
> +    qemu_progress_init(progress, 2.0, 0);

Perhaps compare should report read speed.

> @@ -1127,7 +1127,7 @@ static int img_convert(int argc, char **argv)
>      const char *fmt, *out_fmt, *cache, *out_baseimg, *out_filename;
>      BlockDriver *drv, *proto_drv;
>      BlockDriverState **bs = NULL, *out_bs = NULL;
> -    int64_t total_sectors, nb_sectors, sector_num, bs_offset;
> +    int64_t total_sectors, nb_sectors, sector_num, bs_offset, time;
>      uint64_t bs_sectors;
>      uint8_t * buf = NULL;
>      const uint8_t *buf1;
> @@ -1136,7 +1136,7 @@ static int img_convert(int argc, char **argv)
>      QEMUOptionParameter *out_baseimg_param;
>      char *options = NULL;
>      const char *snapshot_name = NULL;
> -    float local_progress = 0;
> +    float local_progress = 0, write_speed = 0;
>      int min_sparse = 8; /* Need at least 4k of zeros for sparse detection */
>      bool quiet = false;
>      Error *local_err = NULL;
> @@ -1223,7 +1223,7 @@ static int img_convert(int argc, char **argv)
>      out_filename = argv[argc - 1];
>  
>      /* Initialize before goto out */
> -    qemu_progress_init(progress, 2.0);
> +    qemu_progress_init(progress, 2.0, write_speed);
>  
>      if (options && is_help_option(options)) {
>          ret = print_block_option_help(out_filename, out_fmt);
> @@ -1237,7 +1237,7 @@ static int img_convert(int argc, char **argv)
>          goto out;
>      }
>  
> -    qemu_progress_print(0, 100);
> +    qemu_progress_print(0, 100, write_speed);
>  
>      bs = g_malloc0(bs_n * sizeof(BlockDriverState *));
>  
> @@ -1460,7 +1460,7 @@ static int img_convert(int argc, char **argv)
>                  }
>              }
>              sector_num += n;
> -            qemu_progress_print(local_progress, 100);
> +            qemu_progress_print(local_progress, 100, write_speed);
>          }

Write speed is not calculated for compressed writes.

> -            qemu_progress_print(local_progress, 100);
> +            time = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - time;
> +            write_speed = (sectors_to_bytes(n1) * 1000000000 / (double) time 
> ) / 1048576 ;

Please use constants for these magic numbers (e.g.
NANOSECONDS_PER_SECOND and MEGABYTE).  By the way, shouldn't the speed
be in KB/s?

> @@ -2174,8 +2177,8 @@ static int img_rebase(int argc, char **argv)
>      }
>      filename = argv[optind++];
>  
> -    qemu_progress_init(progress, 2.0);
> -    qemu_progress_print(0, 100);
> +    qemu_progress_init(progress, 2.0, 0);
> +    qemu_progress_print(0, 100, 0);

Perhaps we should provide the write speed?