* [Qemu-devel] [PATCH 0/2] qemu-img: Allow source cache mode specification @ 2014-07-19 20:35 Max Reitz 2014-07-19 20:35 ` [Qemu-devel] [PATCH 1/2] " Max Reitz 2014-07-19 20:35 ` [Qemu-devel] [PATCH 2/2] qemu-img: Allow cache mode specification for amend Max Reitz 0 siblings, 2 replies; 9+ messages in thread From: Max Reitz @ 2014-07-19 20:35 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Peter Lieven, Stefan Hajnoczi, 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 1/2] qemu-img: Allow source cache mode specification 2014-07-19 20:35 [Qemu-devel] [PATCH 0/2] qemu-img: Allow source cache mode specification Max Reitz @ 2014-07-19 20:35 ` Max Reitz 2014-07-21 15:52 ` Eric Blake 2014-07-19 20:35 ` [Qemu-devel] [PATCH 2/2] qemu-img: Allow cache mode specification for amend Max Reitz 1 sibling, 1 reply; 9+ messages in thread From: Max Reitz @ 2014-07-19 20:35 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Peter Lieven, Stefan Hajnoczi, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] qemu-img: Allow source cache mode specification 2014-07-19 20:35 ` [Qemu-devel] [PATCH 1/2] " Max Reitz @ 2014-07-21 15:52 ` Eric Blake 2014-07-22 20:06 ` Max Reitz 0 siblings, 1 reply; 9+ messages in thread From: Eric Blake @ 2014-07-21 15:52 UTC (permalink / raw) To: Max Reitz, qemu-devel; +Cc: Kevin Wolf, Peter Lieven, Stefan Hajnoczi [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] qemu-img: Allow source cache mode specification 2014-07-21 15:52 ` Eric Blake @ 2014-07-22 20:06 ` Max Reitz 2014-07-23 8:10 ` Kevin Wolf 0 siblings, 1 reply; 9+ messages in thread From: Max Reitz @ 2014-07-22 20:06 UTC (permalink / raw) To: Eric Blake, qemu-devel; +Cc: Kevin Wolf, Peter Lieven, Stefan Hajnoczi 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] qemu-img: Allow source cache mode specification 2014-07-22 20:06 ` Max Reitz @ 2014-07-23 8:10 ` Kevin Wolf 2014-07-24 23:44 ` Max Reitz 0 siblings, 1 reply; 9+ messages in thread From: Kevin Wolf @ 2014-07-23 8:10 UTC (permalink / raw) To: Max Reitz; +Cc: Peter Lieven, qemu-devel, Stefan Hajnoczi 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] qemu-img: Allow source cache mode specification 2014-07-23 8:10 ` Kevin Wolf @ 2014-07-24 23:44 ` Max Reitz 0 siblings, 0 replies; 9+ messages in thread From: Max Reitz @ 2014-07-24 23:44 UTC (permalink / raw) To: Kevin Wolf; +Cc: Peter Lieven, qemu-devel, Stefan Hajnoczi 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 2/2] qemu-img: Allow cache mode specification for amend 2014-07-19 20:35 [Qemu-devel] [PATCH 0/2] qemu-img: Allow source cache mode specification Max Reitz 2014-07-19 20:35 ` [Qemu-devel] [PATCH 1/2] " Max Reitz @ 2014-07-19 20:35 ` Max Reitz 2014-07-21 15:57 ` Eric Blake 1 sibling, 1 reply; 9+ messages in thread From: Max Reitz @ 2014-07-19 20:35 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Peter Lieven, Stefan Hajnoczi, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu-img: Allow cache mode specification for amend 2014-07-19 20:35 ` [Qemu-devel] [PATCH 2/2] qemu-img: Allow cache mode specification for amend Max Reitz @ 2014-07-21 15:57 ` Eric Blake 2014-07-22 20:09 ` Max Reitz 0 siblings, 1 reply; 9+ messages in thread From: Eric Blake @ 2014-07-21 15:57 UTC (permalink / raw) To: Max Reitz, qemu-devel; +Cc: Kevin Wolf, Peter Lieven, Stefan Hajnoczi [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu-img: Allow cache mode specification for amend 2014-07-21 15:57 ` Eric Blake @ 2014-07-22 20:09 ` Max Reitz 0 siblings, 0 replies; 9+ messages in thread From: Max Reitz @ 2014-07-22 20:09 UTC (permalink / raw) To: Eric Blake, qemu-devel; +Cc: Kevin Wolf, Peter Lieven, Stefan Hajnoczi 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-07-24 23:44 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-07-19 20:35 [Qemu-devel] [PATCH 0/2] qemu-img: Allow source cache mode specification Max Reitz 2014-07-19 20:35 ` [Qemu-devel] [PATCH 1/2] " Max Reitz 2014-07-21 15:52 ` Eric Blake 2014-07-22 20:06 ` Max Reitz 2014-07-23 8:10 ` Kevin Wolf 2014-07-24 23:44 ` Max Reitz 2014-07-19 20:35 ` [Qemu-devel] [PATCH 2/2] qemu-img: Allow cache mode specification for amend Max Reitz 2014-07-21 15:57 ` Eric Blake 2014-07-22 20:09 ` Max Reitz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).