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

[Qemu-devel] [PATCH v2 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.

v2:
 - Patch 1:
   - Remove superfluous space in qemu-img-cmds.hx [Eric]
   - Reorder all getopt() optstrings which are touched by this patch
     anyway according to the subsequent switch [Eric (implicitly)]
 - Patch 2:
   - Reorder img_amend()'s optstring [Eric]


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 v2 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..48df0b8 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 c98896b..9f3909c 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"
@@ -587,7 +588,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;
@@ -596,6 +597,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[] = {
@@ -605,7 +607,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, "hf:r:T:q",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -633,6 +635,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;
@@ -652,6 +657,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;
@@ -944,7 +955,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;
@@ -952,6 +963,7 @@ 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;
@@ -959,8 +971,9 @@ static int img_compare(int argc, char **argv)
     uint64_t bs_sectors;
     uint64_t progress_base;
 
+    cache = BDRV_DEFAULT_CACHE;
     for (;;) {
-        c = getopt(argc, argv, "hpf:F:sq");
+        c = getopt(argc, argv, "hf:F:T:pqs");
         if (c == -1) {
             break;
         }
@@ -975,6 +988,9 @@ static int img_compare(int argc, char **argv)
         case 'F':
             fmt2 = optarg;
             break;
+        case 'T':
+            cache = optarg;
+            break;
         case 'p':
             progress = true;
             break;
@@ -999,17 +1015,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;
@@ -1178,8 +1202,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;
@@ -1201,11 +1225,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, "hf:O:B:ce6o:s:l:S:pt:T:qn");
         if (c == -1) {
             break;
         }
@@ -1286,6 +1311,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;
@@ -1322,6 +1350,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_malloc0(bs_n * sizeof(BlockDriverState *));
@@ -1330,7 +1365,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]) {
@@ -2272,8 +2307,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;
@@ -2282,10 +2317,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, "hf:F:b:upt:T:q");
         if (c == -1) {
             break;
         }
@@ -2312,6 +2348,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;
@@ -2340,6 +2379,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.
      *
@@ -2383,7 +2429,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",
@@ -2393,8 +2439,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 v2 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>
Reviewed-by: Eric Blake <eblake@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 48df0b8..55aec6b 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 9f3909c..ef74ae4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2738,12 +2738,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, "ho:f:t:q");
         if (c == -1) {
             break;
         }
@@ -2770,6 +2772,9 @@ static int img_amend(int argc, char **argv)
             case 'f':
                 fmt = optarg;
                 break;
+            case 't':
+                cache = optarg;
+                break;
             case 'q':
                 quiet = true;
                 break;
@@ -2792,8 +2797,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 v2 1/2] qemu-img: Allow source cache mode specification

[Eric Blake]

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

On 07/22/2014 02:58 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).
> 
> 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(-)

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

[Kevin Wolf]

Am 22.07.2014 um 22:58 hat Max Reitz geschrieben:
> 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.

I would prefer if we used -t in all places where only one image is
used, so that I don't have to remember that supposedly check -r has only
an input image, whereas amend has only an output image. But that's
probably a matter of taste...

If you don't want to change it, the implementation looks okay:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[Max Reitz]

On 14.08.2014 17:30, Kevin Wolf wrote:
> Am 22.07.2014 um 22:58 hat Max Reitz geschrieben:
>> 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.
> I would prefer if we used -t in all places where only one image is
> used, so that I don't have to remember that supposedly check -r has only
> an input image, whereas amend has only an output image. But that's
> probably a matter of taste...
>
> If you don't want to change it, the implementation looks okay:
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

In order to get this series in as fast as possible, I won't change it in 
this series anymore.

Maybe we can add support for "-t" and "-T" being usable interchangably 
for such single-image commands in the future, but for now I hope the 
error messages will suffice.

Max

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

[Kevin Wolf]

Am 22.07.2014 um 22:58 hat Max Reitz geschrieben:
> 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.
> 
> v2:
>  - Patch 1:
>    - Remove superfluous space in qemu-img-cmds.hx [Eric]
>    - Reorder all getopt() optstrings which are touched by this patch
>      anyway according to the subsequent switch [Eric (implicitly)]
>  - Patch 2:
>    - Reorder img_amend()'s optstring [Eric]

Thanks, applied all to the block branch.

Kevin