[Qemu-devel] [PATCH 0/2] qemu-img: Allow source cache mode specification

[Qemu-devel] [PATCH 0/2] qemu-img: Allow source cache mode specification

[Max Reitz]

Currently, qemu-img does not allow setting the cache mode for source
images. However, it reads images generally only once, therefore a full
writeback cache unnecessarily clutters the host cache. In case the user
finds this undesirable, there has to be a way of disabling that cache.
This series first introduces such a way for check, compare, convert and
rebase and then (while at it) adds a cache mode switch to amend (which
was missing so far).

Peter already tried to add a BDRV_O_SEQUENTIAL mode to qemu-img convert.
If we want to add such a mode (which in my opinion would be the logical
consequence of this series), we may want to make it just another cache
mode which can be selected by the user.


Max Reitz (2):
  qemu-img: Allow source cache mode specification
  qemu-img: Allow cache mode specification for amend

 qemu-img-cmds.hx | 20 ++++++------
 qemu-img.c       | 97 ++++++++++++++++++++++++++++++++++++++++++++------------
 qemu-img.texi    | 16 +++++++---
 3 files changed, 98 insertions(+), 35 deletions(-)

-- 
2.0.1

[Qemu-devel] [PATCH 1/2] qemu-img: Allow source cache mode specification

[Max Reitz]

Many qemu-img subcommands only read the source file(s) once. For these
use cases, a full write-back cache is unnecessary and mainly clutters
host cache memory. Though this is generally no concern as cache memory
is freely available and can be scaled by the host OS, it may become a
concern with thin provisioning.

For these cases, it makes sense to allow users to freely specify the
source cache mode (e.g. use no cache at all).

This commit adds a new switch (-T) for the qemu-img subcommands check,
compare, convert and rebase to specify the cache to be used for source
images (the backing file in case of rebase).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img-cmds.hx | 16 ++++++------
 qemu-img.c       | 78 ++++++++++++++++++++++++++++++++++++++++++++------------
 qemu-img.texi    | 14 +++++++---
 3 files changed, 80 insertions(+), 28 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index d029609..5613628 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -10,9 +10,9 @@ STEXI
 ETEXI
 
 DEF("check", img_check,
-    "check [-q] [-f fmt] [--output=ofmt]  [-r [leaks | all]] filename")
+    "check [-q] [-f fmt] [--output=ofmt]  [-r [leaks | all]] [-T src_cache] filename")
 STEXI
-@item check [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] @var{filename}
+@item check [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
 ETEXI
 
 DEF("create", img_create,
@@ -28,15 +28,15 @@ STEXI
 ETEXI
 
 DEF("compare", img_compare,
-    "compare [-f fmt] [-F fmt] [-p] [-q] [-s] filename1 filename2")
+    "compare [-f fmt] [-F fmt] [-T src_cache] [-p] [-q] [-s] filename1 filename2")
 STEXI
-@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s] @var{filename1} @var{filename2}
+@item compare [-f @var{fmt}] [-F @var{fmt}] [-T @var{src_cache}] [-p] [-q] [-s] @var{filename1} @var{filename2}
 ETEXI
 
 DEF("convert", img_convert,
-    "convert [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename")
+    "convert [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-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_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
 
 DEF("info", img_info,
@@ -58,9 +58,9 @@ STEXI
 ETEXI
 
 DEF("rebase", img_rebase,
-    "rebase [-q] [-f fmt] [-t cache] [-p] [-u] -b backing_file [-F backing_fmt] filename")
+    "rebase [-q] [-f fmt] [-t cache] [-T src_cache] [-p] [-u] -b backing_file [-F backing_fmt] filename")
 STEXI
-@item rebase [-q] [-f @var{fmt}] [-t @var{cache}] [-p] [-u] -b @var{backing_file} [-F @var{backing_fmt}] @var{filename}
+@item rebase [-q] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-p] [-u] -b @var{backing_file} [-F @var{backing_fmt}] @var{filename}
 ETEXI
 
 DEF("resize", img_resize,
diff --git a/qemu-img.c b/qemu-img.c
index 9566fae..845b8c7 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -109,6 +109,7 @@ static void QEMU_NORETURN help(void)
            "  'cache' is the cache mode used to write the output disk image, the valid\n"
            "    options are: 'none', 'writeback' (default, except for convert), 'writethrough',\n"
            "    'directsync' and 'unsafe' (default for convert)\n"
+           "  'src_cache' in contrast is the cache mode used to read input disk images\n"
            "  'size' is the disk image size in bytes. Optional suffixes\n"
            "    'k' or 'K' (kilobyte, 1024), 'M' (megabyte, 1024k), 'G' (gigabyte, 1024M),\n"
            "    'T' (terabyte, 1024G), 'P' (petabyte, 1024T) and 'E' (exabyte, 1024P)  are\n"
@@ -586,7 +587,7 @@ static int img_check(int argc, char **argv)
 {
     int c, ret;
     OutputFormat output_format = OFORMAT_HUMAN;
-    const char *filename, *fmt, *output;
+    const char *filename, *fmt, *output, *cache;
     BlockDriverState *bs;
     int fix = 0;
     int flags = BDRV_O_FLAGS | BDRV_O_CHECK;
@@ -595,6 +596,7 @@ static int img_check(int argc, char **argv)
 
     fmt = NULL;
     output = NULL;
+    cache = BDRV_DEFAULT_CACHE;
     for(;;) {
         int option_index = 0;
         static const struct option long_options[] = {
@@ -604,7 +606,7 @@ static int img_check(int argc, char **argv)
             {"output", required_argument, 0, OPTION_OUTPUT},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:hr:q",
+        c = getopt_long(argc, argv, "f:hr:T:q",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -632,6 +634,9 @@ static int img_check(int argc, char **argv)
         case OPTION_OUTPUT:
             output = optarg;
             break;
+        case 'T':
+            cache = optarg;
+            break;
         case 'q':
             quiet = true;
             break;
@@ -651,6 +656,12 @@ static int img_check(int argc, char **argv)
         return 1;
     }
 
+    ret = bdrv_parse_cache_flags(cache, &flags);
+    if (ret < 0) {
+        error_report("Invalid source cache option: %s", cache);
+        return 1;
+    }
+
     bs = bdrv_new_open("image", filename, fmt, flags, true, quiet);
     if (!bs) {
         return 1;
@@ -943,7 +954,7 @@ static int check_empty_sectors(BlockDriverState *bs, int64_t sect_num,
  */
 static int img_compare(int argc, char **argv)
 {
-    const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2;
+    const char *fmt1 = NULL, *fmt2 = NULL, *cache, *filename1, *filename2;
     BlockDriverState *bs1, *bs2;
     int64_t total_sectors1, total_sectors2;
     uint8_t *buf1 = NULL, *buf2 = NULL;
@@ -951,14 +962,16 @@ static int img_compare(int argc, char **argv)
     int allocated1, allocated2;
     int ret = 0; /* return value - 0 Ident, 1 Different, >1 Error */
     bool progress = false, quiet = false, strict = false;
+    int flags;
     int64_t total_sectors;
     int64_t sector_num = 0;
     int64_t nb_sectors;
     int c, pnum;
     uint64_t progress_base;
 
+    cache = BDRV_DEFAULT_CACHE;
     for (;;) {
-        c = getopt(argc, argv, "hpf:F:sq");
+        c = getopt(argc, argv, "hpf:F:T:sq");
         if (c == -1) {
             break;
         }
@@ -973,6 +986,9 @@ static int img_compare(int argc, char **argv)
         case 'F':
             fmt2 = optarg;
             break;
+        case 'T':
+            cache = optarg;
+            break;
         case 'p':
             progress = true;
             break;
@@ -997,17 +1013,25 @@ static int img_compare(int argc, char **argv)
     filename1 = argv[optind++];
     filename2 = argv[optind++];
 
+    flags = BDRV_O_FLAGS;
+    ret = bdrv_parse_cache_flags(cache, &flags);
+    if (ret < 0) {
+        error_report("Invalid source cache option: %s", cache);
+        ret = 2;
+        goto out3;
+    }
+
     /* Initialize before goto out */
     qemu_progress_init(progress, 2.0);
 
-    bs1 = bdrv_new_open("image 1", filename1, fmt1, BDRV_O_FLAGS, true, quiet);
+    bs1 = bdrv_new_open("image 1", filename1, fmt1, flags, true, quiet);
     if (!bs1) {
         error_report("Can't open file %s", filename1);
         ret = 2;
         goto out3;
     }
 
-    bs2 = bdrv_new_open("image 2", filename2, fmt2, BDRV_O_FLAGS, true, quiet);
+    bs2 = bdrv_new_open("image 2", filename2, fmt2, flags, true, quiet);
     if (!bs2) {
         error_report("Can't open file %s", filename2);
         ret = 2;
@@ -1186,8 +1210,8 @@ static int img_convert(int argc, char **argv)
 {
     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;
+    int progress = 0, flags, src_flags;
+    const char *fmt, *out_fmt, *cache, *src_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;
@@ -1209,11 +1233,12 @@ static int img_convert(int argc, char **argv)
     fmt = NULL;
     out_fmt = "raw";
     cache = "unsafe";
+    src_cache = BDRV_DEFAULT_CACHE;
     out_baseimg = NULL;
     compress = 0;
     skip_create = 0;
     for(;;) {
-        c = getopt(argc, argv, "f:O:B:s:hce6o:pS:t:qnl:");
+        c = getopt(argc, argv, "f:O:B:s:hce6o:pS:t:T:qnl:");
         if (c == -1) {
             break;
         }
@@ -1294,6 +1319,9 @@ static int img_convert(int argc, char **argv)
         case 't':
             cache = optarg;
             break;
+        case 'T':
+            src_cache = optarg;
+            break;
         case 'q':
             quiet = true;
             break;
@@ -1330,6 +1358,13 @@ static int img_convert(int argc, char **argv)
         goto out;
     }
 
+    src_flags = BDRV_O_FLAGS;
+    ret = bdrv_parse_cache_flags(src_cache, &src_flags);
+    if (ret < 0) {
+        error_report("Invalid source cache option: %s", src_cache);
+        goto out;
+    }
+
     qemu_progress_print(0, 100);
 
     bs = g_new0(BlockDriverState *, bs_n);
@@ -1339,7 +1374,7 @@ static int img_convert(int argc, char **argv)
     for (bs_i = 0; bs_i < bs_n; bs_i++) {
         char *id = bs_n > 1 ? g_strdup_printf("source %d", bs_i)
                             : g_strdup("source");
-        bs[bs_i] = bdrv_new_open(id, argv[optind + bs_i], fmt, BDRV_O_FLAGS,
+        bs[bs_i] = bdrv_new_open(id, argv[optind + bs_i], fmt, src_flags,
                                  true, quiet);
         g_free(id);
         if (!bs[bs_i]) {
@@ -2286,8 +2321,8 @@ static int img_rebase(int argc, char **argv)
     BlockDriverState *bs, *bs_old_backing = NULL, *bs_new_backing = NULL;
     BlockDriver *old_backing_drv, *new_backing_drv;
     char *filename;
-    const char *fmt, *cache, *out_basefmt, *out_baseimg;
-    int c, flags, ret;
+    const char *fmt, *cache, *src_cache, *out_basefmt, *out_baseimg;
+    int c, flags, src_flags, ret;
     int unsafe = 0;
     int progress = 0;
     bool quiet = false;
@@ -2296,10 +2331,11 @@ static int img_rebase(int argc, char **argv)
     /* Parse commandline parameters */
     fmt = NULL;
     cache = BDRV_DEFAULT_CACHE;
+    src_cache = BDRV_DEFAULT_CACHE;
     out_baseimg = NULL;
     out_basefmt = NULL;
     for(;;) {
-        c = getopt(argc, argv, "uhf:F:b:pt:q");
+        c = getopt(argc, argv, "uhf:F:b:pt:T:q");
         if (c == -1) {
             break;
         }
@@ -2326,6 +2362,9 @@ static int img_rebase(int argc, char **argv)
         case 't':
             cache = optarg;
             break;
+        case 'T':
+            src_cache = optarg;
+            break;
         case 'q':
             quiet = true;
             break;
@@ -2354,6 +2393,13 @@ static int img_rebase(int argc, char **argv)
         return -1;
     }
 
+    src_flags = BDRV_O_FLAGS;
+    ret = bdrv_parse_cache_flags(src_cache, &src_flags);
+    if (ret < 0) {
+        error_report("Invalid source cache option: %s", src_cache);
+        return -1;
+    }
+
     /*
      * Open the images.
      *
@@ -2397,7 +2443,7 @@ static int img_rebase(int argc, char **argv)
 
         bs_old_backing = bdrv_new("old_backing", &error_abort);
         bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
-        ret = bdrv_open(&bs_old_backing, backing_name, NULL, NULL, BDRV_O_FLAGS,
+        ret = bdrv_open(&bs_old_backing, backing_name, NULL, NULL, src_flags,
                         old_backing_drv, &local_err);
         if (ret) {
             error_report("Could not open old backing file '%s': %s",
@@ -2407,8 +2453,8 @@ static int img_rebase(int argc, char **argv)
         }
         if (out_baseimg[0]) {
             bs_new_backing = bdrv_new("new_backing", &error_abort);
-            ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, NULL,
-                            BDRV_O_FLAGS, new_backing_drv, &local_err);
+            ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, NULL, src_flags,
+                            new_backing_drv, &local_err);
             if (ret) {
                 error_report("Could not open new backing file '%s': %s",
                              out_baseimg, error_get_pretty(local_err));
diff --git a/qemu-img.texi b/qemu-img.texi
index 514be90..688b28d 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -72,6 +72,9 @@ down to the nearest 512 bytes. You may use the common size suffixes like
 specifies the cache mode that should be used with the (destination) file. See
 the documentation of the emulator's @code{-drive cache=...} option for allowed
 values.
+@item -T @var{src_cache}
+in contrast specifies the cache mode that should be used with the source
+file(s).
 @end table
 
 Parameters to snapshot subcommand:
@@ -113,7 +116,7 @@ Skip the creation of the target volume
 Command description:
 
 @table @option
-@item check [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] @var{filename}
+@item check [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
 
 Perform a consistency check on the disk image @var{filename}. The command can
 output in the format @var{ofmt} which is either @code{human} or @code{json}.
@@ -172,7 +175,7 @@ the backing file, the backing file will not be truncated.  If you want the
 backing file to match the size of the smaller snapshot, you can safely truncate
 it yourself once the commit operation successfully completes.
 
-@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] @var{filename1} @var{filename2}
+@item compare [-f @var{fmt}] [-F @var{fmt}] [-T @var{src_cache}] [-p] [-s] [-q] @var{filename1} @var{filename2}
 
 Check if two images have the same content. You can compare images with
 different format or settings.
@@ -213,7 +216,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_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [-c] [-p] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 
 Convert the disk image @var{filename} or a snapshot @var{snapshot_param}(@var{snapshot_id_or_name} is deprecated)
 to disk image @var{output_filename} using format @var{output_fmt}. It can be optionally compressed (@code{-c}
@@ -325,7 +328,7 @@ source code.
 
 List, apply, create or delete snapshots in image @var{filename}.
 
-@item rebase [-f @var{fmt}] [-t @var{cache}] [-p] [-u] -b @var{backing_file} [-F @var{backing_fmt}] @var{filename}
+@item rebase [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-p] [-u] -b @var{backing_file} [-F @var{backing_fmt}] @var{filename}
 
 Changes the backing file of an image. Only the formats @code{qcow2} and
 @code{qed} support changing the backing file.
@@ -336,6 +339,9 @@ The backing file is changed to @var{backing_file} and (if the image format of
 string), then the image is rebased onto no backing file (i.e. it will exist
 independently of any backing file).
 
+@var{cache} specifies the cache mode to be used for @var{filename}, whereas
+@var{src_cache} specifies the cache mode for reading the new backing file.
+
 There are two different modes in which @code{rebase} can operate:
 @table @option
 @item Safe mode
-- 
2.0.1

[Qemu-devel] [PATCH 2/2] qemu-img: Allow cache mode specification for amend

[Max Reitz]

qemu-img amend may extensively modify the target image, depending on the
options to be amended (e.g. conversion to qcow2 compat level 0.10 from
1.1 for an image with many unallocated zero clusters). Therefore it
makes sense to allow the user to specify the cache mode to be used.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img-cmds.hx |  4 ++--
 qemu-img.c       | 19 +++++++++++++++----
 qemu-img.texi    |  2 +-
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 5613628..20a21ff 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -70,8 +70,8 @@ STEXI
 ETEXI
 
 DEF("amend", img_amend,
-    "amend [-q] [-f fmt] -o options filename")
+    "amend [-q] [-f fmt] [-t cache] -o options filename")
 STEXI
-@item amend [-q] [-f @var{fmt}] -o @var{options} @var{filename}
+@item amend [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
 @end table
 ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index 845b8c7..df6cb46 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2773,12 +2773,14 @@ static int img_amend(int argc, char **argv)
     char *options = NULL;
     QemuOptsList *create_opts = NULL;
     QemuOpts *opts = NULL;
-    const char *fmt = NULL, *filename;
+    const char *fmt = NULL, *filename, *cache;
+    int flags;
     bool quiet = false;
     BlockDriverState *bs = NULL;
 
+    cache = BDRV_DEFAULT_CACHE;
     for (;;) {
-        c = getopt(argc, argv, "hqf:o:");
+        c = getopt(argc, argv, "hqf:t:o:");
         if (c == -1) {
             break;
         }
@@ -2805,6 +2807,9 @@ static int img_amend(int argc, char **argv)
             case 'f':
                 fmt = optarg;
                 break;
+            case 't':
+                cache = optarg;
+                break;
             case 'q':
                 quiet = true;
                 break;
@@ -2827,8 +2832,14 @@ static int img_amend(int argc, char **argv)
         error_exit("Expecting one image file name");
     }
 
-    bs = bdrv_new_open("image", filename, fmt,
-                       BDRV_O_FLAGS | BDRV_O_RDWR, true, quiet);
+    flags = BDRV_O_FLAGS | BDRV_O_RDWR;
+    ret = bdrv_parse_cache_flags(cache, &flags);
+    if (ret < 0) {
+        error_report("Invalid cache option: %s", cache);
+        goto out;
+    }
+
+    bs = bdrv_new_open("image", filename, fmt, flags, true, quiet);
     if (!bs) {
         error_report("Could not open image '%s'", filename);
         ret = -1;
diff --git a/qemu-img.texi b/qemu-img.texi
index 688b28d..cb68948 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -397,7 +397,7 @@ After using this command to grow a disk image, you must use file system and
 partitioning tools inside the VM to actually begin using the new space on the
 device.
 
-@item amend [-f @var{fmt}] -o @var{options} @var{filename}
+@item amend [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
 
 Amends the image format specific @var{options} for the image file
 @var{filename}. Not all file formats support this operation.
-- 
2.0.1

Re: [Qemu-devel] [PATCH 1/2] qemu-img: Allow source cache mode specification

[Eric Blake]

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

On 07/19/2014 02:35 PM, Max Reitz wrote:
> Many qemu-img subcommands only read the source file(s) once. For these
> use cases, a full write-back cache is unnecessary and mainly clutters
> host cache memory. Though this is generally no concern as cache memory
> is freely available and can be scaled by the host OS, it may become a
> concern with thin provisioning.
> 
> For these cases, it makes sense to allow users to freely specify the
> source cache mode (e.g. use no cache at all).
> 
> This commit adds a new switch (-T) for the qemu-img subcommands check,
> compare, convert and rebase to specify the cache to be used for source
> images (the backing file in case of rebase).

What mnemonic did you have in mind when choosing -T? Or was it just a
universally available letter for the subcommands you were touching?

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img-cmds.hx | 16 ++++++------
>  qemu-img.c       | 78 ++++++++++++++++++++++++++++++++++++++++++++------------
>  qemu-img.texi    | 14 +++++++---
>  3 files changed, 80 insertions(+), 28 deletions(-)
> 
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index d029609..5613628 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -10,9 +10,9 @@ STEXI
>  ETEXI
>  
>  DEF("check", img_check,
> -    "check [-q] [-f fmt] [--output=ofmt]  [-r [leaks | all]] filename")
> +    "check [-q] [-f fmt] [--output=ofmt]  [-r [leaks | all]] [-T src_cache] filename")

Might be nice to fix the unintentional double space before -r while
touching this line.

>  DEF("convert", img_convert,
> -    "convert [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename")
> +    "convert [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename")

Oh, maybe you just picked -T for source because -t was already picked
for destination?

At any rate, seems reasonable.
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 2/2] qemu-img: Allow cache mode specification for amend

[Eric Blake]

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

On 07/19/2014 02:35 PM, Max Reitz wrote:
> qemu-img amend may extensively modify the target image, depending on the
> options to be amended (e.g. conversion to qcow2 compat level 0.10 from
> 1.1 for an image with many unallocated zero clusters). Therefore it
> makes sense to allow the user to specify the cache mode to be used.

Extensive modifications implies long-running operation - the 'amend'
subcommand is a good candidate for the -p progress meter option.  But
that would be a separate patch.

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img-cmds.hx |  4 ++--
>  qemu-img.c       | 19 +++++++++++++++----
>  qemu-img.texi    |  2 +-
>  3 files changed, 18 insertions(+), 7 deletions(-)
> 

>  
> +    cache = BDRV_DEFAULT_CACHE;
>      for (;;) {
> -        c = getopt(argc, argv, "hqf:o:");
> +        c = getopt(argc, argv, "hqf:t:o:");
>          if (c == -1) {
>              break;
>          }
> @@ -2805,6 +2807,9 @@ static int img_amend(int argc, char **argv)
>              case 'f':
>                  fmt = optarg;
>                  break;
> +            case 't':
> +                cache = optarg;
> +                break;
>              case 'q':


Pre-existing, so I won't hold up review, but I'm a big fan of having the
switch block in the same order as the getopt string (that is, we listed
'q' before 'f' in the string above, so the cases are out-of-order with
respect to that string).  The fix can go either way (reshuffle the case
statements, or reorder the optstring above).  [and for the truly OCD, I
prefer the optstring in case-insensitive alphabetical order "f:ho:qt:",
because then it's easier to scan the string to see what letters are
still available for new options - but that's asking a bit much]

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 1/2] qemu-img: Allow source cache mode specification

[Max Reitz]

On 21.07.2014 17:52, Eric Blake wrote:
> On 07/19/2014 02:35 PM, Max Reitz wrote:
>> Many qemu-img subcommands only read the source file(s) once. For these
>> use cases, a full write-back cache is unnecessary and mainly clutters
>> host cache memory. Though this is generally no concern as cache memory
>> is freely available and can be scaled by the host OS, it may become a
>> concern with thin provisioning.
>>
>> For these cases, it makes sense to allow users to freely specify the
>> source cache mode (e.g. use no cache at all).
>>
>> This commit adds a new switch (-T) for the qemu-img subcommands check,
>> compare, convert and rebase to specify the cache to be used for source
>> images (the backing file in case of rebase).
> What mnemonic did you have in mind when choosing -T? Or was it just a
> universally available letter for the subcommands you were touching?

To be honest, I just didn't know what -t stands for. Therefore I just 
thought it might be remotely logical if the lower-cased letter is used 
for destination and the upper-cased letter for source caching.

>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   qemu-img-cmds.hx | 16 ++++++------
>>   qemu-img.c       | 78 ++++++++++++++++++++++++++++++++++++++++++++------------
>>   qemu-img.texi    | 14 +++++++---
>>   3 files changed, 80 insertions(+), 28 deletions(-)
>>
>> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
>> index d029609..5613628 100644
>> --- a/qemu-img-cmds.hx
>> +++ b/qemu-img-cmds.hx
>> @@ -10,9 +10,9 @@ STEXI
>>   ETEXI
>>   
>>   DEF("check", img_check,
>> -    "check [-q] [-f fmt] [--output=ofmt]  [-r [leaks | all]] filename")
>> +    "check [-q] [-f fmt] [--output=ofmt]  [-r [leaks | all]] [-T src_cache] filename")
> Might be nice to fix the unintentional double space before -r while
> touching this line.

Okay, I'll send a v2 (taking into account your comments for patch 2).

>>   DEF("convert", img_convert,
>> -    "convert [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename")
>> +    "convert [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename")
> Oh, maybe you just picked -T for source because -t was already picked
> for destination?

Right. :-)

> At any rate, seems reasonable.
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks,

Max

Re: [Qemu-devel] [PATCH 2/2] qemu-img: Allow cache mode specification for amend

[Max Reitz]

On 21.07.2014 17:57, Eric Blake wrote:
> On 07/19/2014 02:35 PM, Max Reitz wrote:
>> qemu-img amend may extensively modify the target image, depending on the
>> options to be amended (e.g. conversion to qcow2 compat level 0.10 from
>> 1.1 for an image with many unallocated zero clusters). Therefore it
>> makes sense to allow the user to specify the cache mode to be used.
> Extensive modifications implies long-running operation - the 'amend'
> subcommand is a good candidate for the -p progress meter option.  But
> that would be a separate patch.

Yes, this is on my to do list anyway.

>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   qemu-img-cmds.hx |  4 ++--
>>   qemu-img.c       | 19 +++++++++++++++----
>>   qemu-img.texi    |  2 +-
>>   3 files changed, 18 insertions(+), 7 deletions(-)
>>
>>   
>> +    cache = BDRV_DEFAULT_CACHE;
>>       for (;;) {
>> -        c = getopt(argc, argv, "hqf:o:");
>> +        c = getopt(argc, argv, "hqf:t:o:");
>>           if (c == -1) {
>>               break;
>>           }
>> @@ -2805,6 +2807,9 @@ static int img_amend(int argc, char **argv)
>>               case 'f':
>>                   fmt = optarg;
>>                   break;
>> +            case 't':
>> +                cache = optarg;
>> +                break;
>>               case 'q':
>
> Pre-existing, so I won't hold up review, but I'm a big fan of having the
> switch block in the same order as the getopt string (that is, we listed
> 'q' before 'f' in the string above, so the cases are out-of-order with
> respect to that string).  The fix can go either way (reshuffle the case
> statements, or reorder the optstring above).  [and for the truly OCD, I
> prefer the optstring in case-insensitive alphabetical order "f:ho:qt:",
> because then it's easier to scan the string to see what letters are
> still available for new options - but that's asking a bit much]

Right, in fact I thought of you when I touched this block; since it was 
already completely out of order, I decided for the minimal change, 
however. I'll redo it in v2.

I personally somehow like to have common switches like -q at the end, I 
don't know why. For now, I'll just reshuffle the getopt() string.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks,

Max

Re: [Qemu-devel] [PATCH 1/2] qemu-img: Allow source cache mode specification

[Kevin Wolf]

Am 22.07.2014 um 22:06 hat Max Reitz geschrieben:
> On 21.07.2014 17:52, Eric Blake wrote:
> >On 07/19/2014 02:35 PM, Max Reitz wrote:
> >>Many qemu-img subcommands only read the source file(s) once. For these
> >>use cases, a full write-back cache is unnecessary and mainly clutters
> >>host cache memory. Though this is generally no concern as cache memory
> >>is freely available and can be scaled by the host OS, it may become a
> >>concern with thin provisioning.
> >>
> >>For these cases, it makes sense to allow users to freely specify the
> >>source cache mode (e.g. use no cache at all).
> >>
> >>This commit adds a new switch (-T) for the qemu-img subcommands check,
> >>compare, convert and rebase to specify the cache to be used for source
> >>images (the backing file in case of rebase).
> >What mnemonic did you have in mind when choosing -T? Or was it just a
> >universally available letter for the subcommands you were touching?
> 
> To be honest, I just didn't know what -t stands for. Therefore I
> just thought it might be remotely logical if the lower-cased letter
> is used for destination and the upper-cased letter for source
> caching.

Things might get a bit confusing there, though, because upper-case
often means the "other image", i.e. destination or backing file, in other
commands (create -F, compare -F, convert -O and -B, rebase -F).

Of course, most of them are deprecated, so I wouldn't make that a reason
to block this series, but perhaps we should consider using more long
options instead of randomly assigning the letters that are still free.

Kevin

Re: [Qemu-devel] [PATCH 1/2] qemu-img: Allow source cache mode specification

[Max Reitz]

On 23.07.2014 10:10, Kevin Wolf wrote:
> Am 22.07.2014 um 22:06 hat Max Reitz geschrieben:
>> On 21.07.2014 17:52, Eric Blake wrote:
>>> On 07/19/2014 02:35 PM, Max Reitz wrote:
>>>> Many qemu-img subcommands only read the source file(s) once. For these
>>>> use cases, a full write-back cache is unnecessary and mainly clutters
>>>> host cache memory. Though this is generally no concern as cache memory
>>>> is freely available and can be scaled by the host OS, it may become a
>>>> concern with thin provisioning.
>>>>
>>>> For these cases, it makes sense to allow users to freely specify the
>>>> source cache mode (e.g. use no cache at all).
>>>>
>>>> This commit adds a new switch (-T) for the qemu-img subcommands check,
>>>> compare, convert and rebase to specify the cache to be used for source
>>>> images (the backing file in case of rebase).
>>> What mnemonic did you have in mind when choosing -T? Or was it just a
>>> universally available letter for the subcommands you were touching?
>> To be honest, I just didn't know what -t stands for. Therefore I
>> just thought it might be remotely logical if the lower-cased letter
>> is used for destination and the upper-cased letter for source
>> caching.
> Things might get a bit confusing there, though, because upper-case
> often means the "other image", i.e. destination or backing file, in other
> commands (create -F, compare -F, convert -O and -B, rebase -F).

Right, but we don't really have the choice to suddenly make -t an 
upper-case option.

> Of course, most of them are deprecated, so I wouldn't make that a reason
> to block this series, but perhaps we should consider using more long
> options instead of randomly assigning the letters that are still free.

Well, it's not completely random; -t is a cache mode and -T will be, too 
(and I don't think -T should be used for anything but another cache 
mode). I guess I'd generally vote for introducing long options for 
everything after this series, but then some people might use -T and 
confuse others (hence not introducing -T at all might actually make 
sense). I'd be fine with -t and -T and introducing long options later 
on, so I leave it up to you. ;-)

Max