[Qemu-devel] [PATCH] qemu-img: simplify img_convert

[Qemu-devel] [PATCH] qemu-img: simplify img_convert

[Peter Lieven]

img_convert has been around before there was an ImgConvertState or
a block backend, but it has never been modified to directly use
these structs. Change this by parsing parameters directly into
the ImgConvertState and directly use BlockBackend where possible.
Futhermore variable initalization has been reworked and sorted.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 qemu-img.c | 197 +++++++++++++++++++++++++------------------------------------
 1 file changed, 81 insertions(+), 116 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index caa76a7..f271167 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1488,7 +1488,7 @@ typedef struct ImgConvertState {
     int min_sparse;
     size_t cluster_sectors;
     size_t buf_sectors;
-    int num_coroutines;
+    long num_coroutines;
     int running_coroutines;
     Coroutine *co[MAX_COROUTINES];
     int64_t wait_sector_num[MAX_COROUTINES];
@@ -1882,39 +1882,33 @@ static int convert_do_copy(ImgConvertState *s)
 
 static int img_convert(int argc, char **argv)
 {
-    int c, bs_n, bs_i, compress, cluster_sectors, skip_create;
-    int64_t ret = 0;
-    int progress = 0, flags, src_flags;
-    bool writethrough, src_writethrough;
-    const char *fmt, *out_fmt, *cache, *src_cache, *out_baseimg, *out_filename;
+    int c, bs_i;
+    int flags, src_flags = 0;
+    const char *fmt = NULL, *out_fmt = "raw", *cache = "unsafe",
+               *src_cache = BDRV_DEFAULT_CACHE, *out_baseimg = NULL,
+               *out_filename;
     BlockDriver *drv, *proto_drv;
-    BlockBackend **blk = NULL, *out_blk = NULL;
-    BlockDriverState **bs = NULL, *out_bs = NULL;
-    int64_t total_sectors;
-    int64_t *bs_sectors = NULL;
-    size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
     BlockDriverInfo bdi;
+    BlockDriverState *out_bs;
     QemuOpts *opts = NULL;
     QemuOptsList *create_opts = NULL;
     const char *out_baseimg_param;
-    char *options = NULL;
     const char *snapshot_name = NULL;
-    int min_sparse = 8; /* Need at least 4k of zeros for sparse detection */
-    bool quiet = false;
+    char *options = NULL;
     Error *local_err = NULL;
     QemuOpts *sn_opts = NULL;
-    ImgConvertState state;
-    bool image_opts = false;
-    bool wr_in_order = true;
-    long num_coroutines = 8;
+    bool writethrough, src_writethrough, quiet = false, image_opts = false,
+         compress = false, skip_create = false, progress = false;
+    int64_t ret = -EINVAL;
+
+    ImgConvertState s = (ImgConvertState) {
+        /* Need at least 4k of zeros for sparse detection */
+        .min_sparse         = 8,
+        .buf_sectors        = IO_BUF_SIZE / BDRV_SECTOR_SIZE,
+        .wr_in_order        = true,
+        .num_coroutines     = 8,
+    };
 
-    fmt = NULL;
-    out_fmt = "raw";
-    cache = "unsafe";
-    src_cache = BDRV_DEFAULT_CACHE;
-    out_baseimg = NULL;
-    compress = 0;
-    skip_create = 0;
     for(;;) {
         static const struct option long_options[] = {
             {"help", no_argument, 0, 'h'},
@@ -1940,24 +1934,22 @@ static int img_convert(int argc, char **argv)
             break;
         case 'B':
             out_baseimg = optarg;
+            s.target_has_backing = true;
             break;
         case 'c':
-            compress = 1;
+            s.compressed = true;
             break;
         case 'e':
             error_report("option -e is deprecated, please use \'-o "
                   "encryption\' instead!");
-            ret = -1;
             goto fail_getopt;
         case '6':
             error_report("option -6 is deprecated, please use \'-o "
                   "compat6\' instead!");
-            ret = -1;
             goto fail_getopt;
         case 'o':
             if (!is_valid_option_list(optarg)) {
                 error_report("Invalid option list: %s", optarg);
-                ret = -1;
                 goto fail_getopt;
             }
             if (!options) {
@@ -1978,7 +1970,6 @@ static int img_convert(int argc, char **argv)
                 if (!sn_opts) {
                     error_report("Failed in parsing snapshot param '%s'",
                                  optarg);
-                    ret = -1;
                     goto fail_getopt;
                 }
             } else {
@@ -1992,15 +1983,14 @@ static int img_convert(int argc, char **argv)
             sval = cvtnum(optarg);
             if (sval < 0) {
                 error_report("Invalid minimum zero buffer size for sparse output specified");
-                ret = -1;
                 goto fail_getopt;
             }
 
-            min_sparse = sval / BDRV_SECTOR_SIZE;
+            s.min_sparse = sval / BDRV_SECTOR_SIZE;
             break;
         }
         case 'p':
-            progress = 1;
+            progress = true;
             break;
         case 't':
             cache = optarg;
@@ -2012,19 +2002,18 @@ static int img_convert(int argc, char **argv)
             quiet = true;
             break;
         case 'n':
-            skip_create = 1;
+            skip_create = true;
             break;
         case 'm':
-            if (qemu_strtol(optarg, NULL, 0, &num_coroutines) ||
-                num_coroutines < 1 || num_coroutines > MAX_COROUTINES) {
+            if (qemu_strtol(optarg, NULL, 0, &s.num_coroutines) ||
+                s.num_coroutines < 1 || s.num_coroutines > MAX_COROUTINES) {
                 error_report("Invalid number of coroutines. Allowed number of"
                              " coroutines is between 1 and %d", MAX_COROUTINES);
-                ret = -1;
                 goto fail_getopt;
             }
             break;
         case 'W':
-            wr_in_order = false;
+            s.wr_in_order = false;
             break;
         case OPTION_OBJECT:
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -2045,83 +2034,79 @@ static int img_convert(int argc, char **argv)
         goto fail_getopt;
     }
 
-    if (!wr_in_order && compress) {
+    if (!s.wr_in_order && s.compressed) {
         error_report("Out of order write and compress are mutually exclusive");
-        ret = -1;
         goto fail_getopt;
     }
 
-    /* Initialize before goto out */
-    if (quiet) {
-        progress = 0;
-    }
-    qemu_progress_init(progress, 1.0);
-
-    bs_n = argc - optind - 1;
-    out_filename = bs_n >= 1 ? argv[argc - 1] : NULL;
+    s.src_num = argc - optind - 1;
+    out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
 
     if (options && has_help_option(options)) {
         ret = print_block_option_help(out_filename, out_fmt);
-        goto out;
+        goto fail_getopt;
     }
 
-    if (bs_n < 1) {
-        error_exit("Must specify image file name");
+    if (s.src_num < 1) {
+        error_report("Must specify image file name");
+        goto fail_getopt;
     }
 
 
-    if (bs_n > 1 && out_baseimg) {
+    if (s.src_num > 1 && out_baseimg) {
         error_report("-B makes no sense when concatenating multiple input "
                      "images");
-        ret = -1;
-        goto out;
+        goto fail_getopt;
     }
 
-    src_flags = 0;
+    /* ret is still -EINVAL until here */
     ret = bdrv_parse_cache_mode(src_cache, &src_flags, &src_writethrough);
     if (ret < 0) {
         error_report("Invalid source cache option: %s", src_cache);
-        goto out;
+        goto fail_getopt;
     }
 
+    /* Initialize before goto out */
+    if (quiet) {
+        progress = false;
+    }
+    qemu_progress_init(progress, 1.0);
     qemu_progress_print(0, 100);
 
-    blk = g_new0(BlockBackend *, bs_n);
-    bs = g_new0(BlockDriverState *, bs_n);
-    bs_sectors = g_new(int64_t, bs_n);
+    s.src = g_new0(BlockBackend *, s.src_num);
+    s.src_sectors = g_new(int64_t, s.src_num);
 
-    total_sectors = 0;
-    for (bs_i = 0; bs_i < bs_n; bs_i++) {
-        blk[bs_i] = img_open(image_opts, argv[optind + bs_i],
-                             fmt, src_flags, src_writethrough, quiet);
-        if (!blk[bs_i]) {
+    for (bs_i = 0; bs_i < s.src_num; bs_i++) {
+        s.src[bs_i] = img_open(image_opts, argv[optind + bs_i],
+                                fmt, src_flags, src_writethrough, quiet);
+        if (!s.src[bs_i]) {
             ret = -1;
             goto out;
         }
-        bs[bs_i] = blk_bs(blk[bs_i]);
-        bs_sectors[bs_i] = blk_nb_sectors(blk[bs_i]);
-        if (bs_sectors[bs_i] < 0) {
+        s.src_sectors[bs_i] = blk_nb_sectors(s.src[bs_i]);
+        if (s.src_sectors[bs_i] < 0) {
             error_report("Could not get size of %s: %s",
-                         argv[optind + bs_i], strerror(-bs_sectors[bs_i]));
+                         argv[optind + bs_i], strerror(-s.src_sectors[bs_i]));
             ret = -1;
             goto out;
         }
-        total_sectors += bs_sectors[bs_i];
+        s.total_sectors += s.src_sectors[bs_i];
     }
 
     if (sn_opts) {
-        bdrv_snapshot_load_tmp(bs[0],
+        bdrv_snapshot_load_tmp(blk_bs(s.src[0]),
                                qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
                                qemu_opt_get(sn_opts, SNAPSHOT_OPT_NAME),
                                &local_err);
     } else if (snapshot_name != NULL) {
-        if (bs_n > 1) {
+        if (s.src_num > 1) {
             error_report("No support for concatenating multiple snapshot");
             ret = -1;
             goto out;
         }
 
-        bdrv_snapshot_load_tmp_by_id_or_name(bs[0], snapshot_name, &local_err);
+        bdrv_snapshot_load_tmp_by_id_or_name(blk_bs(s.src[0]), snapshot_name,
+                                             &local_err);
     }
     if (local_err) {
         error_reportf_err(local_err, "Failed to load snapshot: ");
@@ -2172,7 +2157,7 @@ static int img_convert(int argc, char **argv)
             }
         }
 
-        qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_sectors * 512,
+        qemu_opt_set_number(opts, BLOCK_OPT_SIZE, s.total_sectors * 512,
                             &error_abort);
         ret = add_old_style_options(out_fmt, opts, out_baseimg, NULL);
         if (ret < 0) {
@@ -2187,7 +2172,7 @@ static int img_convert(int argc, char **argv)
     }
 
     /* Check if compression is supported */
-    if (compress) {
+    if (s.compressed) {
         bool encryption =
             qemu_opt_get_bool(opts, BLOCK_OPT_ENCRYPT, false);
         const char *preallocation =
@@ -2226,7 +2211,7 @@ static int img_convert(int argc, char **argv)
         }
     }
 
-    flags = min_sparse ? (BDRV_O_RDWR | BDRV_O_UNMAP) : BDRV_O_RDWR;
+    flags = s.min_sparse ? (BDRV_O_RDWR | BDRV_O_UNMAP) : BDRV_O_RDWR;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
@@ -2238,37 +2223,36 @@ static int img_convert(int argc, char **argv)
      * the bdrv_create() call which takes different params.
      * Not critical right now, so fix can wait...
      */
-    out_blk = img_open_file(out_filename, out_fmt, flags, writethrough, quiet);
-    if (!out_blk) {
+    s.target = img_open_file(out_filename, out_fmt, flags, writethrough, quiet);
+    if (!s.target) {
         ret = -1;
         goto out;
     }
-    out_bs = blk_bs(out_blk);
+    out_bs = blk_bs(s.target);
 
     /* increase bufsectors from the default 4096 (2M) if opt_transfer
      * or discard_alignment of the out_bs is greater. Limit to 32768 (16MB)
      * as maximum. */
-    bufsectors = MIN(32768,
-                     MAX(bufsectors,
-                         MAX(out_bs->bl.opt_transfer >> BDRV_SECTOR_BITS,
-                             out_bs->bl.pdiscard_alignment >>
-                             BDRV_SECTOR_BITS)));
+    s.buf_sectors = MIN(32768,
+                        MAX(s.buf_sectors,
+                            MAX(out_bs->bl.opt_transfer >> BDRV_SECTOR_BITS,
+                                out_bs->bl.pdiscard_alignment >>
+                                BDRV_SECTOR_BITS)));
 
     if (skip_create) {
-        int64_t output_sectors = blk_nb_sectors(out_blk);
+        int64_t output_sectors = blk_nb_sectors(s.target);
         if (output_sectors < 0) {
             error_report("unable to get output image length: %s",
                          strerror(-output_sectors));
             ret = -1;
             goto out;
-        } else if (output_sectors < total_sectors) {
+        } else if (output_sectors < s.total_sectors) {
             error_report("output file is smaller than input file");
             ret = -1;
             goto out;
         }
     }
 
-    cluster_sectors = 0;
     ret = bdrv_get_info(out_bs, &bdi);
     if (ret < 0) {
         if (compress) {
@@ -2276,26 +2260,11 @@ static int img_convert(int argc, char **argv)
             goto out;
         }
     } else {
-        compress = compress || bdi.needs_compressed_writes;
-        cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE;
-    }
-
-    state = (ImgConvertState) {
-        .src                = blk,
-        .src_sectors        = bs_sectors,
-        .src_num            = bs_n,
-        .total_sectors      = total_sectors,
-        .target             = out_blk,
-        .compressed         = compress,
-        .target_has_backing = (bool) out_baseimg,
-        .min_sparse         = min_sparse,
-        .cluster_sectors    = cluster_sectors,
-        .buf_sectors        = bufsectors,
-        .wr_in_order        = wr_in_order,
-        .num_coroutines     = num_coroutines,
-    };
-    ret = convert_do_copy(&state);
+        s.compressed = s.compressed || bdi.needs_compressed_writes;
+        s.cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE;
+    }
 
+    ret = convert_do_copy(&s);
 out:
     if (!ret) {
         qemu_progress_print(100, 0);
@@ -2304,22 +2273,18 @@ out:
     qemu_opts_del(opts);
     qemu_opts_free(create_opts);
     qemu_opts_del(sn_opts);
-    blk_unref(out_blk);
-    g_free(bs);
-    if (blk) {
-        for (bs_i = 0; bs_i < bs_n; bs_i++) {
-            blk_unref(blk[bs_i]);
+    blk_unref(s.target);
+    if (s.src) {
+        for (bs_i = 0; bs_i < s.src_num; bs_i++) {
+            blk_unref(s.src[bs_i]);
         }
-        g_free(blk);
+        g_free(s.src);
     }
-    g_free(bs_sectors);
+    g_free(s.src_sectors);
 fail_getopt:
     g_free(options);
 
-    if (ret) {
-        return 1;
-    }
-    return 0;
+    return !!ret;
 }
 
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH] qemu-img: simplify img_convert

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]

On 02/28/2017 07:35 AM, Peter Lieven wrote:
> img_convert has been around before there was an ImgConvertState or
> a block backend, but it has never been modified to directly use
> these structs. Change this by parsing parameters directly into
> the ImgConvertState and directly use BlockBackend where possible.
> Futhermore variable initalization has been reworked and sorted.

s/Futhermore/Furthermore/
s/initalization/initialization/

I almost wonder if this should be split into multiple patches for ease
of review.  But since you've already submitted it, I'll review it as-is.

> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qemu-img.c | 197 +++++++++++++++++++++++++------------------------------------
>  1 file changed, 81 insertions(+), 116 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index caa76a7..f271167 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1488,7 +1488,7 @@ typedef struct ImgConvertState {
>      int min_sparse;
>      size_t cluster_sectors;
>      size_t buf_sectors;
> -    int num_coroutines;
> +    long num_coroutines;

Why is this change needed? A hint in the commit message may be
appropriate (I'm especially wary of 'long', as it differs between 32-
and 64-bit platforms).

> @@ -2012,19 +2002,18 @@ static int img_convert(int argc, char **argv)
>              quiet = true;
>              break;
>          case 'n':
> -            skip_create = 1;
> +            skip_create = true;
>              break;
>          case 'm':
> -            if (qemu_strtol(optarg, NULL, 0, &num_coroutines) ||
> -                num_coroutines < 1 || num_coroutines > MAX_COROUTINES) {
> +            if (qemu_strtol(optarg, NULL, 0, &s.num_coroutines) ||
> +                s.num_coroutines < 1 || s.num_coroutines > MAX_COROUTINES) {

At any rate, it looks like you clamp it, so the change in type doesn't
matter too much.

I didn't spot any problems in the conversion, so:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] qemu-img: simplify img_convert

[Peter Lieven]

Am 06.03.2017 um 17:46 schrieb Eric Blake:
> On 02/28/2017 07:35 AM, Peter Lieven wrote:
>> img_convert has been around before there was an ImgConvertState or
>> a block backend, but it has never been modified to directly use
>> these structs. Change this by parsing parameters directly into
>> the ImgConvertState and directly use BlockBackend where possible.
>> Futhermore variable initalization has been reworked and sorted.
> s/Futhermore/Furthermore/
> s/initalization/initialization/
>
> I almost wonder if this should be split into multiple patches for ease
> of review.  But since you've already submitted it, I'll review it as-is.
>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   qemu-img.c | 197 +++++++++++++++++++++++++------------------------------------
>>   1 file changed, 81 insertions(+), 116 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index caa76a7..f271167 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1488,7 +1488,7 @@ typedef struct ImgConvertState {
>>       int min_sparse;
>>       size_t cluster_sectors;
>>       size_t buf_sectors;
>> -    int num_coroutines;
>> +    long num_coroutines;
> Why is this change needed? A hint in the commit message may be
> appropriate (I'm especially wary of 'long', as it differs between 32-
> and 64-bit platforms).

This is long because it is passed as a pointer to qemu_strtol. num_coroutines
was also long.

Peter

Re: [Qemu-devel] [PATCH] qemu-img: simplify img_convert

[Kevin Wolf]

Am 28.02.2017 um 14:35 hat Peter Lieven geschrieben:
> img_convert has been around before there was an ImgConvertState or
> a block backend, but it has never been modified to directly use
> these structs. Change this by parsing parameters directly into
> the ImgConvertState and directly use BlockBackend where possible.
> Futhermore variable initalization has been reworked and sorted.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qemu-img.c | 197 +++++++++++++++++++++++++------------------------------------
>  1 file changed, 81 insertions(+), 116 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index caa76a7..f271167 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1488,7 +1488,7 @@ typedef struct ImgConvertState {
>      int min_sparse;
>      size_t cluster_sectors;
>      size_t buf_sectors;
> -    int num_coroutines;
> +    long num_coroutines;
>      int running_coroutines;
>      Coroutine *co[MAX_COROUTINES];
>      int64_t wait_sector_num[MAX_COROUTINES];
> @@ -1882,39 +1882,33 @@ static int convert_do_copy(ImgConvertState *s)
>  
>  static int img_convert(int argc, char **argv)
>  {
> -    int c, bs_n, bs_i, compress, cluster_sectors, skip_create;
> -    int64_t ret = 0;
> -    int progress = 0, flags, src_flags;
> -    bool writethrough, src_writethrough;
> -    const char *fmt, *out_fmt, *cache, *src_cache, *out_baseimg, *out_filename;
> +    int c, bs_i;
> +    int flags, src_flags = 0;
> +    const char *fmt = NULL, *out_fmt = "raw", *cache = "unsafe",
> +               *src_cache = BDRV_DEFAULT_CACHE, *out_baseimg = NULL,
> +               *out_filename;
>      BlockDriver *drv, *proto_drv;
> -    BlockBackend **blk = NULL, *out_blk = NULL;
> -    BlockDriverState **bs = NULL, *out_bs = NULL;
> -    int64_t total_sectors;
> -    int64_t *bs_sectors = NULL;
> -    size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
>      BlockDriverInfo bdi;
> +    BlockDriverState *out_bs;
>      QemuOpts *opts = NULL;
>      QemuOptsList *create_opts = NULL;
>      const char *out_baseimg_param;
> -    char *options = NULL;
>      const char *snapshot_name = NULL;
> -    int min_sparse = 8; /* Need at least 4k of zeros for sparse detection */
> -    bool quiet = false;
> +    char *options = NULL;
>      Error *local_err = NULL;
>      QemuOpts *sn_opts = NULL;
> -    ImgConvertState state;
> -    bool image_opts = false;
> -    bool wr_in_order = true;
> -    long num_coroutines = 8;
> +    bool writethrough, src_writethrough, quiet = false, image_opts = false,
> +         compress = false, skip_create = false, progress = false;

compress is never set, but tested. Should probably be completely
replaced by s.compressed.

> [...]
> +    /* Initialize before goto out */
> +    if (quiet) {
> +        progress = false;
> +    }
> +    qemu_progress_init(progress, 1.0);
>      qemu_progress_print(0, 100);
>  
> -    blk = g_new0(BlockBackend *, bs_n);
> -    bs = g_new0(BlockDriverState *, bs_n);
> -    bs_sectors = g_new(int64_t, bs_n);
> +    s.src = g_new0(BlockBackend *, s.src_num);
> +    s.src_sectors = g_new(int64_t, s.src_num);
>  
> -    total_sectors = 0;
> -    for (bs_i = 0; bs_i < bs_n; bs_i++) {
> -        blk[bs_i] = img_open(image_opts, argv[optind + bs_i],
> -                             fmt, src_flags, src_writethrough, quiet);
> -        if (!blk[bs_i]) {
> +    for (bs_i = 0; bs_i < s.src_num; bs_i++) {
> +        s.src[bs_i] = img_open(image_opts, argv[optind + bs_i],
> +                                fmt, src_flags, src_writethrough, quiet);

Indentation is off.

The rest looks okay.

Kevin

Re: [Qemu-devel] [PATCH] qemu-img: simplify img_convert

[Fam Zheng]

On Tue, 02/28 14:35, Peter Lieven wrote:
> img_convert has been around before there was an ImgConvertState or
> a block backend, but it has never been modified to directly use
> these structs. Change this by parsing parameters directly into
> the ImgConvertState and directly use BlockBackend where possible.
> Futhermore variable initalization has been reworked and sorted.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>

I see an iotest failure with this patch, in Kevin's block-next tree:

019 1s ... - output mismatch (see 019.out.bad)
--- /stor/work/qemu/tests/qemu-iotests/019.out	2017-04-17 16:19:56.523968474 +0800
+++ 019.out.bad	2017-04-20 22:03:29.868216955 +0800
@@ -1086,8 +1086,8 @@
 
 Checking if backing clusters are allocated when they shouldn't
 
-0/128 sectors allocated at offset 1 MiB
-0/128 sectors allocated at offset 4.001 GiB
+128/128 sectors allocated at offset 1 MiB
+128/128 sectors allocated at offset 4.001 GiB
 Reading
 
 === IO: pattern 42
Failures: 019
Failed 1 of 1 tests

Re: [Qemu-devel] [PATCH] qemu-img: simplify img_convert

[Peter Lieven]

Am 20.04.2017 um 16:05 schrieb Fam Zheng:
> On Tue, 02/28 14:35, Peter Lieven wrote:
>> img_convert has been around before there was an ImgConvertState or
>> a block backend, but it has never been modified to directly use
>> these structs. Change this by parsing parameters directly into
>> the ImgConvertState and directly use BlockBackend where possible.
>> Futhermore variable initalization has been reworked and sorted.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
> I see an iotest failure with this patch, in Kevin's block-next tree:
>
> 019 1s ... - output mismatch (see 019.out.bad)
> --- /stor/work/qemu/tests/qemu-iotests/019.out	2017-04-17 16:19:56.523968474 +0800
> +++ 019.out.bad	2017-04-20 22:03:29.868216955 +0800
> @@ -1086,8 +1086,8 @@
>   
>   Checking if backing clusters are allocated when they shouldn't
>   
> -0/128 sectors allocated at offset 1 MiB
> -0/128 sectors allocated at offset 4.001 GiB
> +128/128 sectors allocated at offset 1 MiB
> +128/128 sectors allocated at offset 4.001 GiB
>   Reading
>   
>   === IO: pattern 42
> Failures: 019
> Failed 1 of 1 tests

Thanks for notifying, Fam.

@Kevin: Can you drop the patch again? I will send a V2 addressing this issue and the other comments

I have got.


Peter

Re: [Qemu-devel] [PATCH] qemu-img: simplify img_convert

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1429 bytes --]

On 04/20/2017 09:05 AM, Fam Zheng wrote:
> On Tue, 02/28 14:35, Peter Lieven wrote:
>> img_convert has been around before there was an ImgConvertState or
>> a block backend, but it has never been modified to directly use
>> these structs. Change this by parsing parameters directly into
>> the ImgConvertState and directly use BlockBackend where possible.
>> Futhermore variable initalization has been reworked and sorted.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
> 
> I see an iotest failure with this patch, in Kevin's block-next tree:
> 
> 019 1s ... - output mismatch (see 019.out.bad)
> --- /stor/work/qemu/tests/qemu-iotests/019.out	2017-04-17 16:19:56.523968474 +0800
> +++ 019.out.bad	2017-04-20 22:03:29.868216955 +0800
> @@ -1086,8 +1086,8 @@
>  
>  Checking if backing clusters are allocated when they shouldn't
>  
> -0/128 sectors allocated at offset 1 MiB
> -0/128 sectors allocated at offset 4.001 GiB
> +128/128 sectors allocated at offset 1 MiB
> +128/128 sectors allocated at offset 4.001 GiB

Hmm - I wonder if my patch to make 'zero with unmap' on an image with no
backing file prefer pure unallocated clusters over a reads-as-zero would
make a difference.
https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg01728.html

Testing now...

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] qemu-img: simplify img_convert

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1891 bytes --]

On 04/20/2017 09:11 AM, Eric Blake wrote:
> On 04/20/2017 09:05 AM, Fam Zheng wrote:
>> On Tue, 02/28 14:35, Peter Lieven wrote:
>>> img_convert has been around before there was an ImgConvertState or
>>> a block backend, but it has never been modified to directly use
>>> these structs. Change this by parsing parameters directly into
>>> the ImgConvertState and directly use BlockBackend where possible.
>>> Futhermore variable initalization has been reworked and sorted.
>>>
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>
>> I see an iotest failure with this patch, in Kevin's block-next tree:
>>
>> 019 1s ... - output mismatch (see 019.out.bad)
>> --- /stor/work/qemu/tests/qemu-iotests/019.out	2017-04-17 16:19:56.523968474 +0800
>> +++ 019.out.bad	2017-04-20 22:03:29.868216955 +0800
>> @@ -1086,8 +1086,8 @@
>>  
>>  Checking if backing clusters are allocated when they shouldn't
>>  
>> -0/128 sectors allocated at offset 1 MiB
>> -0/128 sectors allocated at offset 4.001 GiB
>> +128/128 sectors allocated at offset 1 MiB
>> +128/128 sectors allocated at offset 4.001 GiB
> 
> Hmm - I wonder if my patch to make 'zero with unmap' on an image with no
> backing file prefer pure unallocated clusters over a reads-as-zero would
> make a difference.
> https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg01728.html
> 
> Testing now...

Nope, did not make a difference, although it's certainly related; per my
comment in that thread that:

    Note that technically, we _could_ write a cluster as unallocated
    rather than zero if a backing file exists but the backing file
    also reads as zero, but that's more expensive to determine, so
    this optimization is limited to qcow2 without a backing file.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] qemu-img: simplify img_convert

[Peter Lieven]

Am 20.04.2017 um 16:18 schrieb Eric Blake:
> On 04/20/2017 09:11 AM, Eric Blake wrote:
>> On 04/20/2017 09:05 AM, Fam Zheng wrote:
>>> On Tue, 02/28 14:35, Peter Lieven wrote:
>>>> img_convert has been around before there was an ImgConvertState or
>>>> a block backend, but it has never been modified to directly use
>>>> these structs. Change this by parsing parameters directly into
>>>> the ImgConvertState and directly use BlockBackend where possible.
>>>> Futhermore variable initalization has been reworked and sorted.
>>>>
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> I see an iotest failure with this patch, in Kevin's block-next tree:
>>>
>>> 019 1s ... - output mismatch (see 019.out.bad)
>>> --- /stor/work/qemu/tests/qemu-iotests/019.out	2017-04-17 16:19:56.523968474 +0800
>>> +++ 019.out.bad	2017-04-20 22:03:29.868216955 +0800
>>> @@ -1086,8 +1086,8 @@
>>>   
>>>   Checking if backing clusters are allocated when they shouldn't
>>>   
>>> -0/128 sectors allocated at offset 1 MiB
>>> -0/128 sectors allocated at offset 4.001 GiB
>>> +128/128 sectors allocated at offset 1 MiB
>>> +128/128 sectors allocated at offset 4.001 GiB
>> Hmm - I wonder if my patch to make 'zero with unmap' on an image with no
>> backing file prefer pure unallocated clusters over a reads-as-zero would
>> make a difference.
>> https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg01728.html
>>
>> Testing now...
> Nope, did not make a difference, although it's certainly related; per my
> comment in that thread that:
>
>      Note that technically, we _could_ write a cluster as unallocated
>      rather than zero if a backing file exists but the backing file
>      also reads as zero, but that's more expensive to determine, so
>      this optimization is limited to qcow2 without a backing file.
>
>

I will look into this tomorrow.


Peter

-- 

Mit freundlichen Grüßen

Peter Lieven

...........................................................

   KAMP Netzwerkdienste GmbH
   Vestische Str. 89-91 | 46117 Oberhausen
   Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
   pl@kamp.de | http://www.kamp.de

   Geschäftsführer: Heiner Lante | Michael Lante
   Amtsgericht Duisburg | HRB Nr. 12154
   USt-Id-Nr.: DE 120607556

...........................................................

Re: [Qemu-devel] [PATCH] qemu-img: simplify img_convert

[Kevin Wolf]

Am 20.04.2017 um 16:08 hat Peter Lieven geschrieben:
> Am 20.04.2017 um 16:05 schrieb Fam Zheng:
> >On Tue, 02/28 14:35, Peter Lieven wrote:
> >>img_convert has been around before there was an ImgConvertState or
> >>a block backend, but it has never been modified to directly use
> >>these structs. Change this by parsing parameters directly into
> >>the ImgConvertState and directly use BlockBackend where possible.
> >>Futhermore variable initalization has been reworked and sorted.
> >>
> >>Signed-off-by: Peter Lieven <pl@kamp.de>
> >I see an iotest failure with this patch, in Kevin's block-next tree:
> >
> >019 1s ... - output mismatch (see 019.out.bad)
> >--- /stor/work/qemu/tests/qemu-iotests/019.out	2017-04-17 16:19:56.523968474 +0800
> >+++ 019.out.bad	2017-04-20 22:03:29.868216955 +0800
> >@@ -1086,8 +1086,8 @@
> >  Checking if backing clusters are allocated when they shouldn't
> >-0/128 sectors allocated at offset 1 MiB
> >-0/128 sectors allocated at offset 4.001 GiB
> >+128/128 sectors allocated at offset 1 MiB
> >+128/128 sectors allocated at offset 4.001 GiB
> >  Reading
> >  === IO: pattern 42
> >Failures: 019
> >Failed 1 of 1 tests
> 
> Thanks for notifying, Fam.
> 
> @Kevin: Can you drop the patch again? I will send a V2 addressing this
> issue and the other comments I have got.

I could, but if it doesn't take too long, I'd prefer to only do so when
v2 is there. There are already a few conflicting patches and I asked
their authors to rebase onto yours, so it wouldn't be nice if they
wouldn't apply again because I removed your patch...

Kevin

Re: [Qemu-devel] [PATCH] qemu-img: simplify img_convert

[Peter Lieven]

Am 20.04.2017 um 21:51 schrieb Kevin Wolf:
> Am 20.04.2017 um 16:08 hat Peter Lieven geschrieben:
>> Am 20.04.2017 um 16:05 schrieb Fam Zheng:
>>> On Tue, 02/28 14:35, Peter Lieven wrote:
>>>> img_convert has been around before there was an ImgConvertState or
>>>> a block backend, but it has never been modified to directly use
>>>> these structs. Change this by parsing parameters directly into
>>>> the ImgConvertState and directly use BlockBackend where possible.
>>>> Futhermore variable initalization has been reworked and sorted.
>>>>
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> I see an iotest failure with this patch, in Kevin's block-next tree:
>>>
>>> 019 1s ... - output mismatch (see 019.out.bad)
>>> --- /stor/work/qemu/tests/qemu-iotests/019.out	2017-04-17 16:19:56.523968474 +0800
>>> +++ 019.out.bad	2017-04-20 22:03:29.868216955 +0800
>>> @@ -1086,8 +1086,8 @@
>>>  Checking if backing clusters are allocated when they shouldn't
>>> -0/128 sectors allocated at offset 1 MiB
>>> -0/128 sectors allocated at offset 4.001 GiB
>>> +128/128 sectors allocated at offset 1 MiB
>>> +128/128 sectors allocated at offset 4.001 GiB
>>>  Reading
>>>  === IO: pattern 42
>>> Failures: 019
>>> Failed 1 of 1 tests
>> Thanks for notifying, Fam.
>>
>> @Kevin: Can you drop the patch again? I will send a V2 addressing this
>> issue and the other comments I have got.
> I could, but if it doesn't take too long, I'd prefer to only do so when
> v2 is there. There are already a few conflicting patches and I asked
> their authors to rebase onto yours, so it wouldn't be nice if they
> wouldn't apply again because I removed your patch...

error found, if backing file is passed via -o backing_file, s.target_has_backing was
not set to true.

I will send a V2 shortly including the other comments I got.

Peter

>
> Kevin