[Qemu-devel] First Patch, Requesting Review

[Qemu-devel] First Patch, Requesting Review

[Varad Gautam]

Hi! I'm new here, and am working on my first bug. I have posted a patch
for Bug#603872 [1]. It's incomplete right now, but please have a look and
tell me if I'm headed in the right direction. (I don't know if I can send
incomplete patches to the mailing list for suggestions or if I run into
some problems.)
 
Usecase: `qemu-img convert` with -p now shows the write speed.
 
I have a few doubts relating to the patch.
 
1. I'm calculating the speed using the time taken to run the for(;;)
at qemu-img.c:1477. I figured that every time this loop runs, n1
sectors are converted, and so I calculate the write_speed
accordingly. Is this correct?
 
2. I have changed qemu-progress.c:qemu_progress_print() to take in a
speed parameter, thinking that it would be the best option. Should I
do it some other way instead (maybe write another function to print
just speed)?
 
Also, what does IO_BUF_SIZE in the same file relate to?
 
Thanks.
Varad
 
[1] https://bugs.launchpad.net/qemu/+bug/603872

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

[Varad Gautam]

From: Varad Gautam <varadgautam@live.com>

Calculate and display write speed when converting image with the
-p parameter. qemu-progress:qemu_progress_print() now takes speed
parameter to print.

Signed-off-by: Varad Gautam <varadgautam@gmail.com>
---
 include/qemu-common.h |    4 ++--
 qemu-img.c            |   31 +++++++++++++++++--------------
 util/qemu-progress.c  |   11 +++++++----
 3 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index 5054836..0e27c68 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -349,9 +349,9 @@ size_t qemu_iovec_memset(QEMUIOVector *qiov, size_t offset,
 
 bool buffer_is_zero(const void *buf, size_t len);
 
-void qemu_progress_init(int enabled, float min_skip);
+void qemu_progress_init(int enabled, float min_skip, float speed);
 void qemu_progress_end(void);
-void qemu_progress_print(float delta, int max);
+void qemu_progress_print(float delta, int max, float speed);
 const char *qemu_get_vm_name(void);
 
 #define QEMU_FILE_TYPE_BIOS   0
diff --git a/qemu-img.c b/qemu-img.c
index bf3fb4f..cf313ed 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -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);
 
     bs1 = bdrv_new_open(filename1, fmt1, BDRV_O_FLAGS, true, quiet);
     if (!bs1) {
@@ -970,7 +970,7 @@ static int img_compare(int argc, char **argv)
     total_sectors = MIN(total_sectors1, total_sectors2);
     progress_base = MAX(total_sectors1, total_sectors2);
 
-    qemu_progress_print(0, 100);
+    qemu_progress_print(0, 100, 0);
 
     if (strict && total_sectors1 != total_sectors2) {
         ret = 1;
@@ -1053,7 +1053,7 @@ static int img_compare(int argc, char **argv)
             }
         }
         sector_num += nb_sectors;
-        qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+        qemu_progress_print(((float) nb_sectors / progress_base)*100, 100, 0);
     }
 
     if (total_sectors1 != total_sectors2) {
@@ -1101,7 +1101,7 @@ static int img_compare(int argc, char **argv)
                 }
             }
             sector_num += nb_sectors;
-            qemu_progress_print(((float) nb_sectors / progress_base)*100, 100);
+            qemu_progress_print(((float) nb_sectors / progress_base)*100, 100, 0);
         }
     }
 
@@ -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);
         }
         /* signal EOF to align */
         bdrv_write_compressed(out_bs, 0, NULL, 0);
@@ -1475,6 +1475,7 @@ static int img_convert(int argc, char **argv)
         }
 
         for(;;) {
+            time = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
             nb_sectors = total_sectors - sector_num;
             if (nb_sectors <= 0) {
                 break;
@@ -1547,7 +1548,9 @@ static int img_convert(int argc, char **argv)
                 n -= n1;
                 buf1 += n1 * 512;
             }
-            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 ;
+            qemu_progress_print(local_progress, 100, write_speed);
         }
     }
 out:
@@ -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);
 
     flags = BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0);
     ret = bdrv_parse_cache_flags(cache, &flags);
@@ -2353,7 +2356,7 @@ static int img_rebase(int argc, char **argv)
 
                 written += pnum;
             }
-            qemu_progress_print(local_progress, 100);
+            qemu_progress_print(local_progress, 100, 0);
         }
 
         qemu_vfree(buf_old);
@@ -2379,7 +2382,7 @@ static int img_rebase(int argc, char **argv)
             out_baseimg, strerror(-ret));
     }
 
-    qemu_progress_print(100, 0);
+    qemu_progress_print(100, 0, 0);
     /*
      * TODO At this point it is possible to check if any clusters that are
      * allocated in the COW file are the same in the backing file. If so, they
diff --git a/util/qemu-progress.c b/util/qemu-progress.c
index 9a3f96c..386f42d 100644
--- a/util/qemu-progress.c
+++ b/util/qemu-progress.c
@@ -31,6 +31,7 @@ struct progress_state {
     float current;
     float last_print;
     float min_skip;
+    float speed;
     void (*print)(void);
     void (*end)(void);
 };
@@ -45,7 +46,7 @@ static volatile sig_atomic_t print_pending;
  */
 static void progress_simple_print(void)
 {
-    printf("    (%3.2f/100%%)\r", state.current);
+    printf("%f kB/s    (%3.2f/100%%)\r", state.speed, state.current);
     fflush(stdout);
 }
 
@@ -70,7 +71,7 @@ static void sigusr_print(int signal)
 static void progress_dummy_print(void)
 {
     if (print_pending) {
-        fprintf(stderr, "    (%3.2f/100%%)\n", state.current);
+        fprintf(stderr, "%f    (%3.2f/100%%)\n", state.speed, state.current);
         print_pending = 0;
     }
 }
@@ -102,9 +103,10 @@ static void progress_dummy_init(void)
  * Reports are also suppressed unless we've had at least @min_skip
  * percent progress since the last report.
  */
-void qemu_progress_init(int enabled, float min_skip)
+void qemu_progress_init(int enabled, float min_skip, float speed)
 {
     state.min_skip = min_skip;
+    state.speed = speed;
     if (enabled) {
         progress_simple_init();
     } else {
@@ -128,7 +130,7 @@ void qemu_progress_end(void)
  * a function might be considered 40% of the full job if used from
  * bdrv_img_create() but only 20% if called from img_convert().
  */
-void qemu_progress_print(float delta, int max)
+void qemu_progress_print(float delta, int max, float speed)
 {
     float current;
 
@@ -141,6 +143,7 @@ void qemu_progress_print(float delta, int max)
         current = 100;
     }
     state.current = current;
+    state.speed = speed;
 
     if (current > (state.last_print + state.min_skip) ||
         (current == 100) || (current == 0)) {
-- 
1.7.10.4

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?