[Qemu-devel] [PATCHv2 1.8 0/9] qemu-img convert optimizations

[Qemu-devel] [PATCHv2 1.8 0/9] qemu-img convert optimizations

[Peter Lieven]

this series adds some optimizations for qemu-img during convert which
have been developed recently:
- skipping input based on get_block_status
- variable I/O buffer size
- align write requests to cluster_size
- show progress in sectors or percent

v1->v2:
  - introduce opt_transfer_length in BlockLimits [Paolo]
  - remove knobs for iobuffer_size and alignment and
    use them unconditionally [Paolo]
  - calculate I/O buffer size by BlockLimits information [Paolo]
  - change the alignment patch to round down to the
    last and not to the next aligned sector [Paolo]
  - limit updates in the sector progress output
  - new patch to increase the default for min_sparse [Paolo]

Peter Lieven (9):
  qemu-img: add support for skipping zeroes in input during convert
  qemu-img: fix usage instruction for qemu-img convert
  block/iscsi: set bdi->cluster_size
  block: add opt_transfer_length to BlockLimits
  block/iscsi: set bs->bl.opt_transfer_length
  qemu-img: dynamically adjust iobuffer size during convert
  qemu-img: round down request length to an aligned sector
  qemu-img: add option to show progress in sectors
  qemu-img: increase min_sparse to 128 sectors (64kb)

 block/iscsi.c             |   10 +++
 include/block/block_int.h |    3 +
 qemu-img-cmds.hx          |    4 +-
 qemu-img.c                |  159 ++++++++++++++++++++++++++++++---------------
 qemu-img.texi             |    6 +-
 5 files changed, 126 insertions(+), 56 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCHv2 1.8 1/9] qemu-img: add support for skipping zeroes in input during convert

[Peter Lieven]

we currently do not check if a sector is allocated during convert.
This means if a sector is unallocated that we allocate a bounce
buffer of zeroes, find out its zero later and do not write it
in the best case. In the worst case this can lead to reading
blocks from a raw device (like iSCSI) altough we could easily
know via get_block_status that they are zero and simply skip them.

This patch also fixes the progress output not being at 100% after
a successful conversion.

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

diff --git a/qemu-img.c b/qemu-img.c
index dc0c2f0..efb744c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1125,13 +1125,15 @@ out3:
 
 static int img_convert(int argc, char **argv)
 {
-    int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size,
+    int c, n, n1, bs_n, bs_i, compress, cluster_size,
         cluster_sectors, skip_create;
+    int64_t ret = 0;
     int progress = 0, flags;
     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,
+            sector_num_next_status = 0;
     uint64_t bs_sectors;
     uint8_t * buf = NULL;
     const uint8_t *buf1;
@@ -1140,7 +1142,6 @@ static int img_convert(int argc, char **argv)
     QEMUOptionParameter *out_baseimg_param;
     char *options = NULL;
     const char *snapshot_name = NULL;
-    float local_progress = 0;
     int min_sparse = 8; /* Need at least 4k of zeros for sparse detection */
     bool quiet = false;
     Error *local_err = NULL;
@@ -1403,10 +1404,6 @@ static int img_convert(int argc, char **argv)
         sector_num = 0;
 
         nb_sectors = total_sectors;
-        if (nb_sectors != 0) {
-            local_progress = (float)100 /
-                (nb_sectors / MIN(nb_sectors, cluster_sectors));
-        }
 
         for(;;) {
             int64_t bs_num;
@@ -1464,7 +1461,7 @@ static int img_convert(int argc, char **argv)
                 }
             }
             sector_num += n;
-            qemu_progress_print(local_progress, 100);
+            qemu_progress_print(100.0 * sector_num / total_sectors, 0);
         }
         /* signal EOF to align */
         bdrv_write_compressed(out_bs, 0, NULL, 0);
@@ -1481,21 +1478,13 @@ static int img_convert(int argc, char **argv)
 
         sector_num = 0; // total number of sectors converted so far
         nb_sectors = total_sectors - sector_num;
-        if (nb_sectors != 0) {
-            local_progress = (float)100 /
-                (nb_sectors / MIN(nb_sectors, IO_BUF_SIZE / 512));
-        }
 
         for(;;) {
             nb_sectors = total_sectors - sector_num;
             if (nb_sectors <= 0) {
+                ret = 0;
                 break;
             }
-            if (nb_sectors >= (IO_BUF_SIZE / 512)) {
-                n = (IO_BUF_SIZE / 512);
-            } else {
-                n = nb_sectors;
-            }
 
             while (sector_num - bs_offset >= bs_sectors) {
                 bs_i ++;
@@ -1507,34 +1496,53 @@ static int img_convert(int argc, char **argv)
                    sector_num, bs_i, bs_offset, bs_sectors); */
             }
 
-            if (n > bs_offset + bs_sectors - sector_num) {
-                n = bs_offset + bs_sectors - sector_num;
-            }
-
-            /* If the output image is being created as a copy on write image,
-               assume that sectors which are unallocated in the input image
-               are present in both the output's and input's base images (no
-               need to copy them). */
-            if (out_baseimg) {
-                ret = bdrv_is_allocated(bs[bs_i], sector_num - bs_offset,
-                                        n, &n1);
+            if ((out_baseimg || has_zero_init) &&
+                sector_num >= sector_num_next_status) {
+                n = nb_sectors > INT_MAX ? INT_MAX : nb_sectors;
+                ret = bdrv_get_block_status(bs[bs_i], sector_num - bs_offset,
+                                            n, &n1);
                 if (ret < 0) {
-                    error_report("error while reading metadata for sector "
-                                 "%" PRId64 ": %s",
-                                 sector_num - bs_offset, strerror(-ret));
+                    error_report("error while reading block status of sector %"
+                                 PRId64 ": %s", sector_num - bs_offset,
+                                 strerror(-ret));
                     goto out;
                 }
-                if (!ret) {
+                /* If the output image is zero initialized, we are not working
+                 * on a shared base and the input is zero we can skip the next
+                 * n1 sectors */
+                if (has_zero_init && !out_baseimg && ret & BDRV_BLOCK_ZERO) {
                     sector_num += n1;
                     continue;
                 }
-                /* The next 'n1' sectors are allocated in the input image. Copy
-                   only those as they may be followed by unallocated sectors. */
-                n = n1;
+                /* If the output image is being created as a copy on write
+                 * image, assume that sectors which are unallocated in the
+                 * input image are present in both the output's and input's
+                 * base images (no need to copy them). */
+                if (out_baseimg) {
+                    if (!(ret & BDRV_BLOCK_DATA)) {
+                        sector_num += n1;
+                        continue;
+                    }
+                    /* The next 'n1' sectors are allocated in the input image.
+                     * Copy only those as they may be followed by unallocated
+                     * sectors. */
+                    nb_sectors = n1;
+                }
+                /* avoid redundant callouts to get_block_status */
+                sector_num_next_status = sector_num + n1;
+            }
+
+            if (nb_sectors >= (IO_BUF_SIZE / 512)) {
+                n = (IO_BUF_SIZE / 512);
             } else {
-                n1 = n;
+                n = nb_sectors;
             }
 
+            if (n > bs_offset + bs_sectors - sector_num) {
+                n = bs_offset + bs_sectors - sector_num;
+            }
+
+            n1 = n;
             ret = bdrv_read(bs[bs_i], sector_num - bs_offset, buf, n);
             if (ret < 0) {
                 error_report("error while reading sector %" PRId64 ": %s",
@@ -1559,10 +1567,13 @@ static int img_convert(int argc, char **argv)
                 n -= n1;
                 buf1 += n1 * 512;
             }
-            qemu_progress_print(local_progress, 100);
+            qemu_progress_print(100.0 * sector_num / total_sectors, 0);
         }
     }
 out:
+    if (!ret) {
+        qemu_progress_print(100, 0);
+    }
     qemu_progress_end();
     free_option_parameters(create_options);
     free_option_parameters(param);
-- 
1.7.9.5

[Qemu-devel] [PATCHv2 1.8 2/9] qemu-img: fix usage instruction for qemu-img convert

[Peter Lieven]

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 qemu-img.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index efb744c..e2d1a0a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -105,7 +105,6 @@ static void help(void)
            "       conversion. If the number of bytes is 0, the source will not be scanned for\n"
            "       unallocated or zero sectors, and the destination image will always be\n"
            "       fully allocated\n"
-           "       images will always be fully allocated\n"
            "  '--output' takes the format in which the output must be done (human or json)\n"
            "  '-n' skips the target volume creation (useful if the volume is created\n"
            "       prior to running qemu-img)\n"
-- 
1.7.9.5

[Qemu-devel] [PATCHv2 1.8 3/9] block/iscsi: set bdi->cluster_size

[Peter Lieven]

this patch aims to set bdi->cluster_size to the internal page size
of the iscsi target so that enabled callers can align requests
properly.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index 93fee6d..75d6b87 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1580,6 +1580,13 @@ static int iscsi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     IscsiLun *iscsilun = bs->opaque;
     bdi->unallocated_blocks_are_zero = !!iscsilun->lbprz;
     bdi->can_write_zeroes_with_unmap = iscsilun->lbprz && iscsilun->lbp.lbpws;
+    /* Guess the internal cluster (page) size of the iscsi target by the means
+     * of opt_unmap_gran. Transfer the unmap granularity only if it has a
+     * reasonable size for bdi->cluster_size */
+    if (iscsilun->bl.opt_unmap_gran * iscsilun->block_size >= 64 * 1024 &&
+        iscsilun->bl.opt_unmap_gran * iscsilun->block_size <= 16 * 1024 * 1024) {
+        bdi->cluster_size = iscsilun->bl.opt_unmap_gran * iscsilun->block_size;
+    }
     return 0;
 }
 
-- 
1.7.9.5

[Qemu-devel] [PATCHv2 1.8 4/9] block: add opt_transfer_length to BlockLimits

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 include/block/block_int.h |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 95140b6..6499516 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -242,6 +242,9 @@ typedef struct BlockLimits {
 
     /* optimal alignment for write zeroes requests in sectors */
     int64_t write_zeroes_alignment;
+    
+    /* optimal transfer length in sectors */
+    int opt_transfer_length;
 } BlockLimits;
 
 /*
-- 
1.7.9.5

[Qemu-devel] [PATCHv2 1.8 5/9] block/iscsi: set bs->bl.opt_transfer_length

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index 75d6b87..1109d8c 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1457,6 +1457,9 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
         }
         bs->bl.write_zeroes_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran,
                                                         iscsilun);
+                                                        
+        bs->bl.opt_transfer_length = sector_lun2qemu(iscsilun->bl.opt_xfer_len,
+                                                     iscsilun);
     }
 
 #if defined(LIBISCSI_FEATURE_NOP_COUNTER)
-- 
1.7.9.5

[Qemu-devel] [PATCHv2 1.8 6/9] qemu-img: dynamically adjust iobuffer size during convert

[Peter Lieven]

since the convert process is basically a sync operation it might
be benificial in some case to change the hardcoded I/O buffer
size to a greater value.

This patch increases the I/O buffer size if the output
driver advertises an optimal transfer length or discard alignment
that is greater than the default buffer size of 2M.

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

diff --git a/qemu-img.c b/qemu-img.c
index e2d1a0a..b076d44 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1135,6 +1135,7 @@ static int img_convert(int argc, char **argv)
             sector_num_next_status = 0;
     uint64_t bs_sectors;
     uint8_t * buf = NULL;
+    size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
     const uint8_t *buf1;
     BlockDriverInfo bdi;
     QEMUOptionParameter *param = NULL, *create_options = NULL;
@@ -1371,7 +1372,16 @@ static int img_convert(int argc, char **argv)
     bs_i = 0;
     bs_offset = 0;
     bdrv_get_geometry(bs[0], &bs_sectors);
-    buf = qemu_blockalign(out_bs, IO_BUF_SIZE);
+
+    /* increase bufsectors from the default 4096 (2M) if opt_transfer_length
+     * 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_length,
+                                         out_bs->bl.discard_alignment))
+                    );
+
+    buf = qemu_blockalign(out_bs, bufsectors * BDRV_SECTOR_SIZE);
 
     if (skip_create) {
         int64_t output_length = bdrv_getlength(out_bs);
@@ -1394,7 +1404,7 @@ static int img_convert(int argc, char **argv)
             goto out;
         }
         cluster_size = bdi.cluster_size;
-        if (cluster_size <= 0 || cluster_size > IO_BUF_SIZE) {
+        if (cluster_size <= 0 || cluster_size > bufsectors * BDRV_SECTOR_SIZE) {
             error_report("invalid cluster size");
             ret = -1;
             goto out;
@@ -1531,8 +1541,8 @@ static int img_convert(int argc, char **argv)
                 sector_num_next_status = sector_num + n1;
             }
 
-            if (nb_sectors >= (IO_BUF_SIZE / 512)) {
-                n = (IO_BUF_SIZE / 512);
+            if (nb_sectors >= bufsectors) {
+                n = bufsectors;
             } else {
                 n = nb_sectors;
             }
-- 
1.7.9.5

[Qemu-devel] [PATCHv2 1.8 7/9] qemu-img: round down request length to an aligned sector

[Peter Lieven]

this patch shortens requests to end at an aligned sector so that
the next request starts aligned.

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

diff --git a/qemu-img.c b/qemu-img.c
index b076d44..1421f0f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1124,8 +1124,7 @@ out3:
 
 static int img_convert(int argc, char **argv)
 {
-    int c, n, n1, bs_n, bs_i, compress, cluster_size,
-        cluster_sectors, skip_create;
+    int c, n, n1, bs_n, bs_i, compress, cluster_sectors, skip_create;
     int64_t ret = 0;
     int progress = 0, flags;
     const char *fmt, *out_fmt, *cache, *out_baseimg, *out_filename;
@@ -1397,19 +1396,21 @@ static int img_convert(int argc, char **argv)
         }
     }
 
+    cluster_sectors = 0;
+    ret = bdrv_get_info(out_bs, &bdi);
+    if (ret < 0 && compress) {
+        error_report("could not get block driver info");
+        goto out;
+    } else {
+        cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE;
+    }
+
     if (compress) {
-        ret = bdrv_get_info(out_bs, &bdi);
-        if (ret < 0) {
-            error_report("could not get block driver info");
-            goto out;
-        }
-        cluster_size = bdi.cluster_size;
-        if (cluster_size <= 0 || cluster_size > bufsectors * BDRV_SECTOR_SIZE) {
+        if (cluster_sectors <= 0 || cluster_sectors > bufsectors) {
             error_report("invalid cluster size");
             ret = -1;
             goto out;
         }
-        cluster_sectors = cluster_size >> 9;
         sector_num = 0;
 
         nb_sectors = total_sectors;
@@ -1547,6 +1548,15 @@ static int img_convert(int argc, char **argv)
                 n = nb_sectors;
             }
 
+            /* round down request length to an aligned sector */
+            if (cluster_sectors > 0 && n >= cluster_sectors) {
+                int64_t next_aligned_sector = (sector_num + n);
+                next_aligned_sector -= next_aligned_sector % cluster_sectors;
+                if (sector_num + n > next_aligned_sector) {
+                    n = next_aligned_sector - sector_num;
+                }
+            }
+
             if (n > bs_offset + bs_sectors - sector_num) {
                 n = bs_offset + bs_sectors - sector_num;
             }
-- 
1.7.9.5

[Qemu-devel] [PATCHv2 1.8 8/9] qemu-img: add option to show progress in sectors

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 qemu-img-cmds.hx |    4 ++--
 qemu-img.c       |   31 ++++++++++++++++++++++++++++---
 qemu-img.texi    |    4 +++-
 3 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index da1d965..6c8183b 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -34,9 +34,9 @@ STEXI
 ETEXI
 
 DEF("convert", img_convert,
-    "convert [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename")
+    "convert [-c] [-p|-pp] [-q] [-n] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename")
 STEXI
-@item convert [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [-c] [-p|-pp] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
 
 DEF("info", img_info,
diff --git a/qemu-img.c b/qemu-img.c
index 1421f0f..cc7540f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -99,6 +99,7 @@ static void help(void)
            "       rebasing in this case (useful for renaming the backing file)\n"
            "  '-h' with or without a command shows this help and lists the supported formats\n"
            "  '-p' show progress of command (only certain commands)\n"
+           "  '-pp' show progress of command in sectors (only convert command)\n"
            "  '-q' use Quiet mode - do not print any output (except errors)\n"
            "  '-S' indicates the consecutive number of bytes (defaults to 4k) that must\n"
            "       contain only zeros for qemu-img to create a sparse image during\n"
@@ -1122,6 +1123,22 @@ out3:
     return ret;
 }
 
+static void print_sector_progress(int progress, int64_t sector_num,
+                                  int64_t total_sectors)
+{
+    static int64_t last_sector = -1;
+    if (progress == 2) {
+        if (sector_num == 0 ||
+            sector_num > last_sector + 0.02 * total_sectors ||
+            sector_num == total_sectors) {
+            printf("%ld of %ld sectors converted.\r", sector_num,
+                   total_sectors);
+            fflush(stdout);
+            last_sector = sector_num;
+        }
+    }
+}
+
 static int img_convert(int argc, char **argv)
 {
     int c, n, n1, bs_n, bs_i, compress, cluster_sectors, skip_create;
@@ -1130,7 +1147,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 = 0, nb_sectors, sector_num, bs_offset,
             sector_num_next_status = 0;
     uint64_t bs_sectors;
     uint8_t * buf = NULL;
@@ -1201,7 +1218,7 @@ static int img_convert(int argc, char **argv)
             break;
         }
         case 'p':
-            progress = 1;
+            progress++;
             break;
         case 't':
             cache = optarg;
@@ -1227,7 +1244,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 == 1, 2.0);
 
     if (options && is_help_option(options)) {
         ret = print_block_option_help(out_filename, out_fmt);
@@ -1258,6 +1275,8 @@ static int img_convert(int argc, char **argv)
         total_sectors += bs_sectors;
     }
 
+    print_sector_progress(progress, 0, total_sectors);
+
     if (snapshot_name != NULL) {
         if (bs_n > 1) {
             error_report("No support for concatenating multiple snapshot");
@@ -1472,6 +1491,7 @@ static int img_convert(int argc, char **argv)
             }
             sector_num += n;
             qemu_progress_print(100.0 * sector_num / total_sectors, 0);
+            print_sector_progress(progress, sector_num, total_sectors);
         }
         /* signal EOF to align */
         bdrv_write_compressed(out_bs, 0, NULL, 0);
@@ -1587,11 +1607,16 @@ static int img_convert(int argc, char **argv)
                 buf1 += n1 * 512;
             }
             qemu_progress_print(100.0 * sector_num / total_sectors, 0);
+            print_sector_progress(progress, sector_num, total_sectors);
         }
     }
 out:
     if (!ret) {
         qemu_progress_print(100, 0);
+        print_sector_progress(progress, total_sectors, total_sectors);
+    }
+    if (progress == 2) {
+        printf("\n");
     }
     qemu_progress_end();
     free_option_parameters(create_options);
diff --git a/qemu-img.texi b/qemu-img.texi
index da36975..cb4a3eb 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -54,6 +54,8 @@ indicates that target image must be compressed (qcow format only)
 with or without a command shows help and lists the supported formats
 @item -p
 display progress bar (convert and rebase commands only)
+@item -pp
+display progress in sectors (convert command only)
 @item -q
 Quiet mode - do not print any output (except errors). There's no progress bar
 in case both @var{-q} and @var{-p} options are used.
@@ -179,7 +181,7 @@ Error on reading data
 
 @end table
 
-@item convert [-c] [-p] [-n] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [-c] [-p|-pp] [-n] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 
 Convert the disk image @var{filename} or a snapshot @var{snapshot_name} to disk image @var{output_filename}
 using format @var{output_fmt}. It can be optionally compressed (@code{-c}
-- 
1.7.9.5

[Qemu-devel] [PATCHv2 1.8 9/9] qemu-img: increase min_sparse to 128 sectors (64kb)

[Peter Lieven]

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 qemu-img.c    |    4 ++--
 qemu-img.texi |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index cc7540f..6f1179b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -101,7 +101,7 @@ static void help(void)
            "  '-p' show progress of command (only certain commands)\n"
            "  '-pp' show progress of command in sectors (only convert command)\n"
            "  '-q' use Quiet mode - do not print any output (except errors)\n"
-           "  '-S' indicates the consecutive number of bytes (defaults to 4k) that must\n"
+           "  '-S' indicates the consecutive number of bytes (defaults to 64k) that must\n"
            "       contain only zeros for qemu-img to create a sparse image during\n"
            "       conversion. If the number of bytes is 0, the source will not be scanned for\n"
            "       unallocated or zero sectors, and the destination image will always be\n"
@@ -1158,7 +1158,7 @@ static int img_convert(int argc, char **argv)
     QEMUOptionParameter *out_baseimg_param;
     char *options = NULL;
     const char *snapshot_name = NULL;
-    int min_sparse = 8; /* Need at least 4k of zeros for sparse detection */
+    int min_sparse = 128; /* Need at least 64k of zeros for sparse detection */
     bool quiet = false;
     Error *local_err = NULL;
 
diff --git a/qemu-img.texi b/qemu-img.texi
index cb4a3eb..c0e86ab 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -195,7 +195,7 @@ Image conversion is also useful to get smaller image when using a
 growable format such as @code{qcow} or @code{cow}: the empty sectors
 are detected and suppressed from the destination image.
 
-@var{sparse_size} indicates the consecutive number of bytes (defaults to 4k)
+@var{sparse_size} indicates the consecutive number of bytes (defaults to 64k)
 that must contain only zeros for qemu-img to create a sparse image during
 conversion. If @var{sparse_size} is 0, the source will not be scanned for
 unallocated or zero sectors, and the destination image will always be
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 1.8 2/9] qemu-img: fix usage instruction for qemu-img convert

[Paolo Bonzini]

Il 26/11/2013 09:56, Peter Lieven ha scritto:
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qemu-img.c |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index efb744c..e2d1a0a 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -105,7 +105,6 @@ static void help(void)
>             "       conversion. If the number of bytes is 0, the source will not be scanned for\n"
>             "       unallocated or zero sectors, and the destination image will always be\n"
>             "       fully allocated\n"
> -           "       images will always be fully allocated\n"
>             "  '--output' takes the format in which the output must be done (human or json)\n"
>             "  '-n' skips the target volume creation (useful if the volume is created\n"
>             "       prior to running qemu-img)\n"
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCHv2 1.8 4/9] block: add opt_transfer_length to BlockLimits

[Paolo Bonzini]

Il 26/11/2013 09:56, Peter Lieven ha scritto:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  include/block/block_int.h |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 95140b6..6499516 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -242,6 +242,9 @@ typedef struct BlockLimits {
>  
>      /* optimal alignment for write zeroes requests in sectors */
>      int64_t write_zeroes_alignment;
> +    
> +    /* optimal transfer length in sectors */
> +    int opt_transfer_length;
>  } BlockLimits;
>  
>  /*
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCHv2 1.8 5/9] block/iscsi: set bs->bl.opt_transfer_length

[Paolo Bonzini]

Il 26/11/2013 09:56, Peter Lieven ha scritto:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/iscsi.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 75d6b87..1109d8c 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1457,6 +1457,9 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>          }
>          bs->bl.write_zeroes_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran,
>                                                          iscsilun);
> +                                                        
> +        bs->bl.opt_transfer_length = sector_lun2qemu(iscsilun->bl.opt_xfer_len,
> +                                                     iscsilun);
>      }
>  
>  #if defined(LIBISCSI_FEATURE_NOP_COUNTER)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCHv2 1.8 6/9] qemu-img: dynamically adjust iobuffer size during convert

[Paolo Bonzini]

Il 26/11/2013 09:56, Peter Lieven ha scritto:
> since the convert process is basically a sync operation it might
> be benificial in some case to change the hardcoded I/O buffer
> size to a greater value.
> 
> This patch increases the I/O buffer size if the output
> driver advertises an optimal transfer length or discard alignment
> that is greater than the default buffer size of 2M.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qemu-img.c |   18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index e2d1a0a..b076d44 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1135,6 +1135,7 @@ static int img_convert(int argc, char **argv)
>              sector_num_next_status = 0;
>      uint64_t bs_sectors;
>      uint8_t * buf = NULL;
> +    size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
>      const uint8_t *buf1;
>      BlockDriverInfo bdi;
>      QEMUOptionParameter *param = NULL, *create_options = NULL;
> @@ -1371,7 +1372,16 @@ static int img_convert(int argc, char **argv)
>      bs_i = 0;
>      bs_offset = 0;
>      bdrv_get_geometry(bs[0], &bs_sectors);
> -    buf = qemu_blockalign(out_bs, IO_BUF_SIZE);
> +
> +    /* increase bufsectors from the default 4096 (2M) if opt_transfer_length
> +     * 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_length,
> +                                         out_bs->bl.discard_alignment))
> +                    );
> +
> +    buf = qemu_blockalign(out_bs, bufsectors * BDRV_SECTOR_SIZE);
>  
>      if (skip_create) {
>          int64_t output_length = bdrv_getlength(out_bs);
> @@ -1394,7 +1404,7 @@ static int img_convert(int argc, char **argv)
>              goto out;
>          }
>          cluster_size = bdi.cluster_size;
> -        if (cluster_size <= 0 || cluster_size > IO_BUF_SIZE) {
> +        if (cluster_size <= 0 || cluster_size > bufsectors * BDRV_SECTOR_SIZE) {
>              error_report("invalid cluster size");
>              ret = -1;
>              goto out;
> @@ -1531,8 +1541,8 @@ static int img_convert(int argc, char **argv)
>                  sector_num_next_status = sector_num + n1;
>              }
>  
> -            if (nb_sectors >= (IO_BUF_SIZE / 512)) {
> -                n = (IO_BUF_SIZE / 512);
> +            if (nb_sectors >= bufsectors) {
> +                n = bufsectors;
>              } else {
>                  n = nb_sectors;
>              }
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCHv2 1.8 7/9] qemu-img: round down request length to an aligned sector

[Paolo Bonzini]

Il 26/11/2013 09:56, Peter Lieven ha scritto:
> this patch shortens requests to end at an aligned sector so that
> the next request starts aligned.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qemu-img.c |   30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index b076d44..1421f0f 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1124,8 +1124,7 @@ out3:
>  
>  static int img_convert(int argc, char **argv)
>  {
> -    int c, n, n1, bs_n, bs_i, compress, cluster_size,
> -        cluster_sectors, skip_create;
> +    int c, n, n1, bs_n, bs_i, compress, cluster_sectors, skip_create;
>      int64_t ret = 0;
>      int progress = 0, flags;
>      const char *fmt, *out_fmt, *cache, *out_baseimg, *out_filename;
> @@ -1397,19 +1396,21 @@ static int img_convert(int argc, char **argv)
>          }
>      }
>  
> +    cluster_sectors = 0;
> +    ret = bdrv_get_info(out_bs, &bdi);
> +    if (ret < 0 && compress) {
> +        error_report("could not get block driver info");
> +        goto out;
> +    } else {
> +        cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE;
> +    }
> +
>      if (compress) {
> -        ret = bdrv_get_info(out_bs, &bdi);
> -        if (ret < 0) {
> -            error_report("could not get block driver info");
> -            goto out;
> -        }
> -        cluster_size = bdi.cluster_size;
> -        if (cluster_size <= 0 || cluster_size > bufsectors * BDRV_SECTOR_SIZE) {
> +        if (cluster_sectors <= 0 || cluster_sectors > bufsectors) {
>              error_report("invalid cluster size");
>              ret = -1;
>              goto out;
>          }
> -        cluster_sectors = cluster_size >> 9;
>          sector_num = 0;
>  
>          nb_sectors = total_sectors;
> @@ -1547,6 +1548,15 @@ static int img_convert(int argc, char **argv)
>                  n = nb_sectors;
>              }
>  
> +            /* round down request length to an aligned sector */

If you have to respin, /* do not bother doing this on short requests.
They happen when we found an all-zero area, and the next sector to write
will not be sector_num + n.  */

> +            if (cluster_sectors > 0 && n >= cluster_sectors) {
> +                int64_t next_aligned_sector = (sector_num + n);
> +                next_aligned_sector -= next_aligned_sector % cluster_sectors;
> +                if (sector_num + n > next_aligned_sector) {
> +                    n = next_aligned_sector - sector_num;
> +                }
> +            }
> +
>              if (n > bs_offset + bs_sectors - sector_num) {
>                  n = bs_offset + bs_sectors - sector_num;
>              }
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCHv2 1.8 9/9] qemu-img: increase min_sparse to 128 sectors (64kb)

[Paolo Bonzini]

Il 26/11/2013 09:56, Peter Lieven ha scritto:
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qemu-img.c    |    4 ++--
>  qemu-img.texi |    2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index cc7540f..6f1179b 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -101,7 +101,7 @@ static void help(void)
>             "  '-p' show progress of command (only certain commands)\n"
>             "  '-pp' show progress of command in sectors (only convert command)\n"
>             "  '-q' use Quiet mode - do not print any output (except errors)\n"
> -           "  '-S' indicates the consecutive number of bytes (defaults to 4k) that must\n"
> +           "  '-S' indicates the consecutive number of bytes (defaults to 64k) that must\n"
>             "       contain only zeros for qemu-img to create a sparse image during\n"
>             "       conversion. If the number of bytes is 0, the source will not be scanned for\n"
>             "       unallocated or zero sectors, and the destination image will always be\n"
> @@ -1158,7 +1158,7 @@ static int img_convert(int argc, char **argv)
>      QEMUOptionParameter *out_baseimg_param;
>      char *options = NULL;
>      const char *snapshot_name = NULL;
> -    int min_sparse = 8; /* Need at least 4k of zeros for sparse detection */
> +    int min_sparse = 128; /* Need at least 64k of zeros for sparse detection */
>      bool quiet = false;
>      Error *local_err = NULL;
>  
> diff --git a/qemu-img.texi b/qemu-img.texi
> index cb4a3eb..c0e86ab 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -195,7 +195,7 @@ Image conversion is also useful to get smaller image when using a
>  growable format such as @code{qcow} or @code{cow}: the empty sectors
>  are detected and suppressed from the destination image.
>  
> -@var{sparse_size} indicates the consecutive number of bytes (defaults to 4k)
> +@var{sparse_size} indicates the consecutive number of bytes (defaults to 64k)
>  that must contain only zeros for qemu-img to create a sparse image during
>  conversion. If @var{sparse_size} is 0, the source will not be scanned for
>  unallocated or zero sectors, and the destination image will always be
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCHv2 1.8 1/9] qemu-img: add support for skipping zeroes in input during convert

[Paolo Bonzini]

Il 26/11/2013 09:56, Peter Lieven ha scritto:
> we currently do not check if a sector is allocated during convert.
> This means if a sector is unallocated that we allocate a bounce
> buffer of zeroes, find out its zero later and do not write it
> in the best case. In the worst case this can lead to reading
> blocks from a raw device (like iSCSI) altough we could easily
> know via get_block_status that they are zero and simply skip them.
> 
> This patch also fixes the progress output not being at 100% after
> a successful conversion.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qemu-img.c |   85 ++++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 48 insertions(+), 37 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index dc0c2f0..efb744c 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1125,13 +1125,15 @@ out3:
>  
>  static int img_convert(int argc, char **argv)
>  {
> -    int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size,
> +    int c, n, n1, bs_n, bs_i, compress, cluster_size,
>          cluster_sectors, skip_create;
> +    int64_t ret = 0;
>      int progress = 0, flags;
>      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,
> +            sector_num_next_status = 0;
>      uint64_t bs_sectors;
>      uint8_t * buf = NULL;
>      const uint8_t *buf1;
> @@ -1140,7 +1142,6 @@ static int img_convert(int argc, char **argv)
>      QEMUOptionParameter *out_baseimg_param;
>      char *options = NULL;
>      const char *snapshot_name = NULL;
> -    float local_progress = 0;
>      int min_sparse = 8; /* Need at least 4k of zeros for sparse detection */
>      bool quiet = false;
>      Error *local_err = NULL;
> @@ -1403,10 +1404,6 @@ static int img_convert(int argc, char **argv)
>          sector_num = 0;
>  
>          nb_sectors = total_sectors;
> -        if (nb_sectors != 0) {
> -            local_progress = (float)100 /
> -                (nb_sectors / MIN(nb_sectors, cluster_sectors));
> -        }
>  
>          for(;;) {
>              int64_t bs_num;
> @@ -1464,7 +1461,7 @@ static int img_convert(int argc, char **argv)
>                  }
>              }
>              sector_num += n;
> -            qemu_progress_print(local_progress, 100);
> +            qemu_progress_print(100.0 * sector_num / total_sectors, 0);
>          }
>          /* signal EOF to align */
>          bdrv_write_compressed(out_bs, 0, NULL, 0);
> @@ -1481,21 +1478,13 @@ static int img_convert(int argc, char **argv)
>  
>          sector_num = 0; // total number of sectors converted so far
>          nb_sectors = total_sectors - sector_num;
> -        if (nb_sectors != 0) {
> -            local_progress = (float)100 /
> -                (nb_sectors / MIN(nb_sectors, IO_BUF_SIZE / 512));
> -        }
>  
>          for(;;) {
>              nb_sectors = total_sectors - sector_num;
>              if (nb_sectors <= 0) {
> +                ret = 0;
>                  break;
>              }
> -            if (nb_sectors >= (IO_BUF_SIZE / 512)) {
> -                n = (IO_BUF_SIZE / 512);
> -            } else {
> -                n = nb_sectors;
> -            }
>  
>              while (sector_num - bs_offset >= bs_sectors) {
>                  bs_i ++;
> @@ -1507,34 +1496,53 @@ static int img_convert(int argc, char **argv)
>                     sector_num, bs_i, bs_offset, bs_sectors); */
>              }
>  
> -            if (n > bs_offset + bs_sectors - sector_num) {
> -                n = bs_offset + bs_sectors - sector_num;
> -            }
> -
> -            /* If the output image is being created as a copy on write image,
> -               assume that sectors which are unallocated in the input image
> -               are present in both the output's and input's base images (no
> -               need to copy them). */
> -            if (out_baseimg) {
> -                ret = bdrv_is_allocated(bs[bs_i], sector_num - bs_offset,
> -                                        n, &n1);
> +            if ((out_baseimg || has_zero_init) &&
> +                sector_num >= sector_num_next_status) {
> +                n = nb_sectors > INT_MAX ? INT_MAX : nb_sectors;
> +                ret = bdrv_get_block_status(bs[bs_i], sector_num - bs_offset,
> +                                            n, &n1);
>                  if (ret < 0) {
> -                    error_report("error while reading metadata for sector "
> -                                 "%" PRId64 ": %s",
> -                                 sector_num - bs_offset, strerror(-ret));
> +                    error_report("error while reading block status of sector %"
> +                                 PRId64 ": %s", sector_num - bs_offset,
> +                                 strerror(-ret));
>                      goto out;
>                  }
> -                if (!ret) {
> +                /* If the output image is zero initialized, we are not working
> +                 * on a shared base and the input is zero we can skip the next
> +                 * n1 sectors */
> +                if (has_zero_init && !out_baseimg && ret & BDRV_BLOCK_ZERO) {

has_zero_init will imply !out_baseimg if skip_create == false.  Should
we care and check out_baseimg separately if skip_create == true?  If
not, you can remove "&& !out_baseimg".

Also, please add parentheses around "ret & BDRV_BLOCK_ZERO".

>                      sector_num += n1;
>                      continue;
>                  }
> -                /* The next 'n1' sectors are allocated in the input image. Copy
> -                   only those as they may be followed by unallocated sectors. */
> -                n = n1;
> +                /* If the output image is being created as a copy on write
> +                 * image, assume that sectors which are unallocated in the
> +                 * input image are present in both the output's and input's
> +                 * base images (no need to copy them). */
> +                if (out_baseimg) {
> +                    if (!(ret & BDRV_BLOCK_DATA)) {
> +                        sector_num += n1;
> +                        continue;
> +                    }
> +                    /* The next 'n1' sectors are allocated in the input image.
> +                     * Copy only those as they may be followed by unallocated
> +                     * sectors. */
> +                    nb_sectors = n1;
> +                }
> +                /* avoid redundant callouts to get_block_status */
> +                sector_num_next_status = sector_num + n1;
> +            }
> +
> +            if (nb_sectors >= (IO_BUF_SIZE / 512)) {
> +                n = (IO_BUF_SIZE / 512);
>              } else {
> -                n1 = n;
> +                n = nb_sectors;
>              }
>  
> +            if (n > bs_offset + bs_sectors - sector_num) {
> +                n = bs_offset + bs_sectors - sector_num;
> +            }
> +            n1 = n;

Please change this to:

n = MIN(nb_sectors, IO_BUF_SIZE / 512);
n = MIN(n, bs_sectors - (sector_num - bs_offset));
n1 = n;

Otherwise looks good.

Thanks,

Paolo

>              ret = bdrv_read(bs[bs_i], sector_num - bs_offset, buf, n);
>              if (ret < 0) {
>                  error_report("error while reading sector %" PRId64 ": %s",
> @@ -1559,10 +1567,13 @@ static int img_convert(int argc, char **argv)
>                  n -= n1;
>                  buf1 += n1 * 512;
>              }
> -            qemu_progress_print(local_progress, 100);
> +            qemu_progress_print(100.0 * sector_num / total_sectors, 0);
>          }
>      }
>  out:
> +    if (!ret) {
> +        qemu_progress_print(100, 0);
> +    }
>      qemu_progress_end();
>      free_option_parameters(create_options);
>      free_option_parameters(param);
> 

Re: [Qemu-devel] [PATCHv2 1.8 8/9] qemu-img: add option to show progress in sectors

[Paolo Bonzini]

I think the right way to do this would be to add the functionality to
qemu-progress.c (i.e. pass a number of sectors and let it choose between
printing % or sectors).

I don't like the qemu-progress.c API anyway, so a rewrite would be
welcome. :)

Paolo

Il 26/11/2013 09:56, Peter Lieven ha scritto:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qemu-img-cmds.hx |    4 ++--
>  qemu-img.c       |   31 ++++++++++++++++++++++++++++---
>  qemu-img.texi    |    4 +++-
>  3 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index da1d965..6c8183b 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -34,9 +34,9 @@ STEXI
>  ETEXI
>  
>  DEF("convert", img_convert,
> -    "convert [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename")
> +    "convert [-c] [-p|-pp] [-q] [-n] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename")
>  STEXI
> -@item convert [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
> +@item convert [-c] [-p|-pp] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
>  ETEXI
>  
>  DEF("info", img_info,
> diff --git a/qemu-img.c b/qemu-img.c
> index 1421f0f..cc7540f 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -99,6 +99,7 @@ static void help(void)
>             "       rebasing in this case (useful for renaming the backing file)\n"
>             "  '-h' with or without a command shows this help and lists the supported formats\n"
>             "  '-p' show progress of command (only certain commands)\n"
> +           "  '-pp' show progress of command in sectors (only convert command)\n"
>             "  '-q' use Quiet mode - do not print any output (except errors)\n"
>             "  '-S' indicates the consecutive number of bytes (defaults to 4k) that must\n"
>             "       contain only zeros for qemu-img to create a sparse image during\n"
> @@ -1122,6 +1123,22 @@ out3:
>      return ret;
>  }
>  
> +static void print_sector_progress(int progress, int64_t sector_num,
> +                                  int64_t total_sectors)
> +{
> +    static int64_t last_sector = -1;
> +    if (progress == 2) {
> +        if (sector_num == 0 ||
> +            sector_num > last_sector + 0.02 * total_sectors ||
> +            sector_num == total_sectors) {
> +            printf("%ld of %ld sectors converted.\r", sector_num,
> +                   total_sectors);
> +            fflush(stdout);
> +            last_sector = sector_num;
> +        }
> +    }
> +}
> +
>  static int img_convert(int argc, char **argv)
>  {
>      int c, n, n1, bs_n, bs_i, compress, cluster_sectors, skip_create;
> @@ -1130,7 +1147,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 = 0, nb_sectors, sector_num, bs_offset,
>              sector_num_next_status = 0;
>      uint64_t bs_sectors;
>      uint8_t * buf = NULL;
> @@ -1201,7 +1218,7 @@ static int img_convert(int argc, char **argv)
>              break;
>          }
>          case 'p':
> -            progress = 1;
> +            progress++;
>              break;
>          case 't':
>              cache = optarg;
> @@ -1227,7 +1244,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 == 1, 2.0);
>  
>      if (options && is_help_option(options)) {
>          ret = print_block_option_help(out_filename, out_fmt);
> @@ -1258,6 +1275,8 @@ static int img_convert(int argc, char **argv)
>          total_sectors += bs_sectors;
>      }
>  
> +    print_sector_progress(progress, 0, total_sectors);
> +
>      if (snapshot_name != NULL) {
>          if (bs_n > 1) {
>              error_report("No support for concatenating multiple snapshot");
> @@ -1472,6 +1491,7 @@ static int img_convert(int argc, char **argv)
>              }
>              sector_num += n;
>              qemu_progress_print(100.0 * sector_num / total_sectors, 0);
> +            print_sector_progress(progress, sector_num, total_sectors);
>          }
>          /* signal EOF to align */
>          bdrv_write_compressed(out_bs, 0, NULL, 0);
> @@ -1587,11 +1607,16 @@ static int img_convert(int argc, char **argv)
>                  buf1 += n1 * 512;
>              }
>              qemu_progress_print(100.0 * sector_num / total_sectors, 0);
> +            print_sector_progress(progress, sector_num, total_sectors);
>          }
>      }
>  out:
>      if (!ret) {
>          qemu_progress_print(100, 0);
> +        print_sector_progress(progress, total_sectors, total_sectors);
> +    }
> +    if (progress == 2) {
> +        printf("\n");
>      }
>      qemu_progress_end();
>      free_option_parameters(create_options);
> diff --git a/qemu-img.texi b/qemu-img.texi
> index da36975..cb4a3eb 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -54,6 +54,8 @@ indicates that target image must be compressed (qcow format only)
>  with or without a command shows help and lists the supported formats
>  @item -p
>  display progress bar (convert and rebase commands only)
> +@item -pp
> +display progress in sectors (convert command only)
>  @item -q
>  Quiet mode - do not print any output (except errors). There's no progress bar
>  in case both @var{-q} and @var{-p} options are used.
> @@ -179,7 +181,7 @@ Error on reading data
>  
>  @end table
>  
> -@item convert [-c] [-p] [-n] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
> +@item convert [-c] [-p|-pp] [-n] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
>  
>  Convert the disk image @var{filename} or a snapshot @var{snapshot_name} to disk image @var{output_filename}
>  using format @var{output_fmt}. It can be optionally compressed (@code{-c}
> 

Re: [Qemu-devel] [PATCHv2 1.8 8/9] qemu-img: add option to show progress in sectors

[Peter Lieven]

On 26.11.2013 11:04, Paolo Bonzini wrote:
> I think the right way to do this would be to add the functionality to
> qemu-progress.c (i.e. pass a number of sectors and let it choose between
> printing % or sectors).
I was thinking about the same, but is this not beyond the scope of this patch? ;-)
I don't mind leaving this patch out if you or the maintainers are strongly against it.
I just need it internally to show the progress of a convert operation. I could
theoretically calculate this the information I need also from the percentage output.
Its a little bit more complicated if more than one hard drive is converted.

Peter

Re: [Qemu-devel] [PATCHv2 1.8 8/9] qemu-img: add option to show progress in sectors

[Paolo Bonzini]

Il 26/11/2013 13:23, Peter Lieven ha scritto:
>> I think the right way to do this would be to add the functionality to
>> qemu-progress.c (i.e. pass a number of sectors and let it choose between
>> printing % or sectors).
> I was thinking about the same, but is this not beyond the scope of this
> patch? ;-)

I think the functionality is not important enough to warrant more code
in qemu-img.c (even 20 lines is already too much).  We should improve
the utility libraries instead.

> I don't mind leaving this patch out if you or the maintainers are
> strongly against it.

Yes, please leave it out.

Paolo

Re: [Qemu-devel] [PATCHv2 1.8 1/9] qemu-img: add support for skipping zeroes in input during convert

[Peter Lieven]

On 26.11.2013 11:02, Paolo Bonzini wrote:
> Il 26/11/2013 09:56, Peter Lieven ha scritto:
>> we currently do not check if a sector is allocated during convert.
>> This means if a sector is unallocated that we allocate a bounce
>> buffer of zeroes, find out its zero later and do not write it
>> in the best case. In the worst case this can lead to reading
>> blocks from a raw device (like iSCSI) altough we could easily
>> know via get_block_status that they are zero and simply skip them.
>>
>> This patch also fixes the progress output not being at 100% after
>> a successful conversion.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   qemu-img.c |   85 ++++++++++++++++++++++++++++++++++--------------------------
>>   1 file changed, 48 insertions(+), 37 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index dc0c2f0..efb744c 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1125,13 +1125,15 @@ out3:
>>   
>>   static int img_convert(int argc, char **argv)
>>   {
>> -    int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size,
>> +    int c, n, n1, bs_n, bs_i, compress, cluster_size,
>>           cluster_sectors, skip_create;
>> +    int64_t ret = 0;
>>       int progress = 0, flags;
>>       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,
>> +            sector_num_next_status = 0;
>>       uint64_t bs_sectors;
>>       uint8_t * buf = NULL;
>>       const uint8_t *buf1;
>> @@ -1140,7 +1142,6 @@ static int img_convert(int argc, char **argv)
>>       QEMUOptionParameter *out_baseimg_param;
>>       char *options = NULL;
>>       const char *snapshot_name = NULL;
>> -    float local_progress = 0;
>>       int min_sparse = 8; /* Need at least 4k of zeros for sparse detection */
>>       bool quiet = false;
>>       Error *local_err = NULL;
>> @@ -1403,10 +1404,6 @@ static int img_convert(int argc, char **argv)
>>           sector_num = 0;
>>   
>>           nb_sectors = total_sectors;
>> -        if (nb_sectors != 0) {
>> -            local_progress = (float)100 /
>> -                (nb_sectors / MIN(nb_sectors, cluster_sectors));
>> -        }
>>   
>>           for(;;) {
>>               int64_t bs_num;
>> @@ -1464,7 +1461,7 @@ static int img_convert(int argc, char **argv)
>>                   }
>>               }
>>               sector_num += n;
>> -            qemu_progress_print(local_progress, 100);
>> +            qemu_progress_print(100.0 * sector_num / total_sectors, 0);
>>           }
>>           /* signal EOF to align */
>>           bdrv_write_compressed(out_bs, 0, NULL, 0);
>> @@ -1481,21 +1478,13 @@ static int img_convert(int argc, char **argv)
>>   
>>           sector_num = 0; // total number of sectors converted so far
>>           nb_sectors = total_sectors - sector_num;
>> -        if (nb_sectors != 0) {
>> -            local_progress = (float)100 /
>> -                (nb_sectors / MIN(nb_sectors, IO_BUF_SIZE / 512));
>> -        }
>>   
>>           for(;;) {
>>               nb_sectors = total_sectors - sector_num;
>>               if (nb_sectors <= 0) {
>> +                ret = 0;
>>                   break;
>>               }
>> -            if (nb_sectors >= (IO_BUF_SIZE / 512)) {
>> -                n = (IO_BUF_SIZE / 512);
>> -            } else {
>> -                n = nb_sectors;
>> -            }
>>   
>>               while (sector_num - bs_offset >= bs_sectors) {
>>                   bs_i ++;
>> @@ -1507,34 +1496,53 @@ static int img_convert(int argc, char **argv)
>>                      sector_num, bs_i, bs_offset, bs_sectors); */
>>               }
>>   
>> -            if (n > bs_offset + bs_sectors - sector_num) {
>> -                n = bs_offset + bs_sectors - sector_num;
>> -            }
>> -
>> -            /* If the output image is being created as a copy on write image,
>> -               assume that sectors which are unallocated in the input image
>> -               are present in both the output's and input's base images (no
>> -               need to copy them). */
>> -            if (out_baseimg) {
>> -                ret = bdrv_is_allocated(bs[bs_i], sector_num - bs_offset,
>> -                                        n, &n1);
>> +            if ((out_baseimg || has_zero_init) &&
>> +                sector_num >= sector_num_next_status) {
>> +                n = nb_sectors > INT_MAX ? INT_MAX : nb_sectors;
>> +                ret = bdrv_get_block_status(bs[bs_i], sector_num - bs_offset,
>> +                                            n, &n1);
>>                   if (ret < 0) {
>> -                    error_report("error while reading metadata for sector "
>> -                                 "%" PRId64 ": %s",
>> -                                 sector_num - bs_offset, strerror(-ret));
>> +                    error_report("error while reading block status of sector %"
>> +                                 PRId64 ": %s", sector_num - bs_offset,
>> +                                 strerror(-ret));
>>                       goto out;
>>                   }
>> -                if (!ret) {
>> +                /* If the output image is zero initialized, we are not working
>> +                 * on a shared base and the input is zero we can skip the next
>> +                 * n1 sectors */
>> +                if (has_zero_init && !out_baseimg && ret & BDRV_BLOCK_ZERO) {
> has_zero_init will imply !out_baseimg if skip_create == false.  Should
> we care and check out_baseimg separately if skip_create == true?  If
> not, you can remove "&& !out_baseimg".
I would leave it in it doesn't hurt.
>
> Also, please add parentheses around "ret & BDRV_BLOCK_ZERO".
Ok.
>
>>                       sector_num += n1;
>>                       continue;
>>                   }
>> -                /* The next 'n1' sectors are allocated in the input image. Copy
>> -                   only those as they may be followed by unallocated sectors. */
>> -                n = n1;
>> +                /* If the output image is being created as a copy on write
>> +                 * image, assume that sectors which are unallocated in the
>> +                 * input image are present in both the output's and input's
>> +                 * base images (no need to copy them). */
>> +                if (out_baseimg) {
>> +                    if (!(ret & BDRV_BLOCK_DATA)) {
>> +                        sector_num += n1;
>> +                        continue;
>> +                    }
>> +                    /* The next 'n1' sectors are allocated in the input image.
>> +                     * Copy only those as they may be followed by unallocated
>> +                     * sectors. */
>> +                    nb_sectors = n1;
>> +                }
>> +                /* avoid redundant callouts to get_block_status */
>> +                sector_num_next_status = sector_num + n1;
>> +            }
>> +
>> +            if (nb_sectors >= (IO_BUF_SIZE / 512)) {
>> +                n = (IO_BUF_SIZE / 512);
>>               } else {
>> -                n1 = n;
>> +                n = nb_sectors;
>>               }
>>   
>> +            if (n > bs_offset + bs_sectors - sector_num) {
>> +                n = bs_offset + bs_sectors - sector_num;
>> +            }
>> +            n1 = n;
> Please change this to:
>
> n = MIN(nb_sectors, IO_BUF_SIZE / 512);
> n = MIN(n, bs_sectors - (sector_num - bs_offset));
> n1 = n;

Thats just copy and paste, I will change it.

I was thinking if it would be a good improvement (separate patch) to
scan the whole source image for holes and account only allocated sectors
in the progress display. It would be much more accurate then.

Peter
>
> Otherwise looks good.
>
> Thanks,
>
> Paolo
>
>>               ret = bdrv_read(bs[bs_i], sector_num - bs_offset, buf, n);
>>               if (ret < 0) {
>>                   error_report("error while reading sector %" PRId64 ": %s",
>> @@ -1559,10 +1567,13 @@ static int img_convert(int argc, char **argv)
>>                   n -= n1;
>>                   buf1 += n1 * 512;
>>               }
>> -            qemu_progress_print(local_progress, 100);
>> +            qemu_progress_print(100.0 * sector_num / total_sectors, 0);
>>           }
>>       }
>>   out:
>> +    if (!ret) {
>> +        qemu_progress_print(100, 0);
>> +    }
>>       qemu_progress_end();
>>       free_option_parameters(create_options);
>>       free_option_parameters(param);
>>


-- 

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

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