* [Qemu-devel] [PATCH] qemu-img: Add cache command line option [not found] <4DF8A4FD.6050807@redhat.com> @ 2011-06-15 13:46 ` Federico Simoncelli 2011-06-15 13:51 ` [Qemu-devel] [PATCHv2] " Federico Simoncelli 2011-06-16 14:28 ` [Qemu-devel] [PATCH] " Christoph Hellwig 0 siblings, 2 replies; 9+ messages in thread From: Federico Simoncelli @ 2011-06-15 13:46 UTC (permalink / raw) To: kvm; +Cc: kwolf, Federico Simoncelli, qemu-devel, avi qemu-img currently writes disk images using writeback and filling up the cache buffers which are then flushed by the kernel preventing other processes from accessing the storage. This is particularly bad in cluster environments where time-based algorithms might be in place and accessing the storage within certain timeouts is critical. This patch adds the option to choose a cache method when writing disk images. Signed-off-by: Federico Simoncelli <fsimonce@redhat.com> --- qemu-img.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++--------- 1 files changed, 68 insertions(+), 13 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index 4f162d1..7609db5 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -40,6 +40,7 @@ typedef struct img_cmd_t { /* Default to cache=writeback as data integrity is not important for qemu-tcg. */ #define BDRV_O_FLAGS BDRV_O_CACHE_WB +#define BDRV_DEFAULT_CACHE "writeback" static void format_print(void *opaque, const char *name) { @@ -64,6 +65,8 @@ static void help(void) "Command parameters:\n" " 'filename' is a disk image filename\n" " 'fmt' is the disk image format. It is guessed automatically in most cases\n" + " 'cache' is the cache mode used to write the output disk image, the valid\n" + " options are: 'none', 'writeback' (default), 'writethrough' and 'unsafe'\n" " 'size' is the disk image size in bytes. Optional suffixes\n" " 'k' or 'K' (kilobyte, 1024), 'M' (megabyte, 1024k), 'G' (gigabyte, 1024M)\n" " and T (terabyte, 1024G) are supported. 'b' is ignored.\n" @@ -180,6 +183,27 @@ static int read_password(char *buf, int buf_size) } #endif +static int set_cache_flag(const char *mode, int *flags) +{ + *flags &= ~BDRV_O_CACHE_MASK; + + if (!strcmp(mode, "none") || !strcmp(mode, "off")) { + *flags |= BDRV_O_CACHE_WB; + *flags |= BDRV_O_NOCACHE; + } else if (!strcmp(mode, "writeback")) { + *flags |= BDRV_O_CACHE_WB; + } else if (!strcmp(mode, "unsafe")) { + *flags |= BDRV_O_CACHE_WB; + *flags |= BDRV_O_NO_FLUSH; + } else if (!strcmp(mode, "writethrough")) { + /* this is the default */ + } else { + return -1; + } + + return 0; +} + static int print_block_option_help(const char *filename, const char *fmt) { BlockDriver *drv, *proto_drv; @@ -441,13 +465,14 @@ static int img_check(int argc, char **argv) static int img_commit(int argc, char **argv) { - int c, ret; - const char *filename, *fmt; + int c, ret, flags; + const char *filename, *fmt, *cache; BlockDriverState *bs; fmt = NULL; + cache = BDRV_DEFAULT_CACHE; for(;;) { - c = getopt(argc, argv, "f:h"); + c = getopt(argc, argv, "f:ht:"); if (c == -1) { break; } @@ -459,6 +484,9 @@ static int img_commit(int argc, char **argv) case 'f': fmt = optarg; break; + case 't': + cache = optarg; + break; } } if (optind >= argc) { @@ -466,7 +494,14 @@ static int img_commit(int argc, char **argv) } filename = argv[optind++]; - bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR); + flags = BDRV_O_RDWR; + ret = set_cache_flag(cache, &flags); + if (ret < 0) { + error_report("Invalid cache option: %s\n", cache); + return -1; + } + + bs = bdrv_new_open(filename, fmt, flags); if (!bs) { return 1; } @@ -591,8 +626,8 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, static int img_convert(int argc, char **argv) { int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors; - int progress = 0; - const char *fmt, *out_fmt, *out_baseimg, *out_filename; + int progress = 0, flags; + const char *fmt, *out_fmt, *cache, *out_baseimg, *out_filename; BlockDriver *drv, *proto_drv; BlockDriverState **bs = NULL, *out_bs = NULL; int64_t total_sectors, nb_sectors, sector_num, bs_offset; @@ -608,10 +643,11 @@ static int img_convert(int argc, char **argv) fmt = NULL; out_fmt = "raw"; + cache = BDRV_DEFAULT_CACHE; out_baseimg = NULL; compress = 0; for(;;) { - c = getopt(argc, argv, "f:O:B:s:hce6o:p"); + c = getopt(argc, argv, "f:O:B:s:hce6o:pt:"); if (c == -1) { break; } @@ -649,6 +685,9 @@ static int img_convert(int argc, char **argv) case 'p': progress = 1; break; + case 't': + cache = optarg; + break; } } @@ -779,8 +818,14 @@ static int img_convert(int argc, char **argv) goto out; } - out_bs = bdrv_new_open(out_filename, out_fmt, - BDRV_O_FLAGS | BDRV_O_RDWR | BDRV_O_NO_FLUSH); + flags = BDRV_O_RDWR; + ret = set_cache_flag(cache, &flags); + if (ret < 0) { + error_report("Invalid cache option: %s\n", cache); + return -1; + } + + out_bs = bdrv_new_open(out_filename, out_fmt, flags); if (!out_bs) { ret = -1; goto out; @@ -1225,18 +1270,18 @@ 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, *out_basefmt, *out_baseimg; + const char *fmt, *cache, *out_basefmt, *out_baseimg; int c, flags, ret; int unsafe = 0; int progress = 0; /* Parse commandline parameters */ fmt = NULL; + cache = BDRV_DEFAULT_CACHE; out_baseimg = NULL; out_basefmt = NULL; - for(;;) { - c = getopt(argc, argv, "uhf:F:b:p"); + c = getopt(argc, argv, "uhf:F:b:pt:"); if (c == -1) { break; } @@ -1256,10 +1301,14 @@ static int img_rebase(int argc, char **argv) break; case 'u': unsafe = 1; + flags |= BDRV_O_NO_BACKING; break; case 'p': progress = 1; break; + case 't': + cache = optarg; + break; } } @@ -1271,13 +1320,19 @@ static int img_rebase(int argc, char **argv) qemu_progress_init(progress, 2.0); qemu_progress_print(0, 100); + flags = BDRV_O_RDWR; + ret = set_cache_flag(cache, &flags); + if (ret < 0) { + error_report("Invalid cache option: %s\n", cache); + return -1; + } + /* * Open the images. * * Ignore the old backing file for unsafe rebase in case we want to correct * the reference to a renamed or moved backing file. */ - flags = BDRV_O_FLAGS | BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0); bs = bdrv_new_open(filename, fmt, flags); if (!bs) { return 1; -- 1.7.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCHv2] qemu-img: Add cache command line option 2011-06-15 13:46 ` [Qemu-devel] [PATCH] qemu-img: Add cache command line option Federico Simoncelli @ 2011-06-15 13:51 ` Federico Simoncelli 2011-06-16 11:58 ` [Qemu-devel] [PATCHv3] " Federico Simoncelli 2011-06-16 14:28 ` [Qemu-devel] [PATCH] " Christoph Hellwig 1 sibling, 1 reply; 9+ messages in thread From: Federico Simoncelli @ 2011-06-15 13:51 UTC (permalink / raw) To: kvm; +Cc: kwolf, Federico Simoncelli, qemu-devel, avi qemu-img currently writes disk images using writeback and filling up the cache buffers which are then flushed by the kernel preventing other processes from accessing the storage. This is particularly bad in cluster environments where time-based algorithms might be in place and accessing the storage within certain timeouts is critical. This patch adds the option to choose a cache method when writing disk images. Signed-off-by: Federico Simoncelli <fsimonce@redhat.com> --- qemu-img.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++--------- 1 files changed, 67 insertions(+), 13 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index 4f162d1..c2a106e 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -40,6 +40,7 @@ typedef struct img_cmd_t { /* Default to cache=writeback as data integrity is not important for qemu-tcg. */ #define BDRV_O_FLAGS BDRV_O_CACHE_WB +#define BDRV_DEFAULT_CACHE "writeback" static void format_print(void *opaque, const char *name) { @@ -64,6 +65,8 @@ static void help(void) "Command parameters:\n" " 'filename' is a disk image filename\n" " 'fmt' is the disk image format. It is guessed automatically in most cases\n" + " 'cache' is the cache mode used to write the output disk image, the valid\n" + " options are: 'none', 'writeback' (default), 'writethrough' and 'unsafe'\n" " 'size' is the disk image size in bytes. Optional suffixes\n" " 'k' or 'K' (kilobyte, 1024), 'M' (megabyte, 1024k), 'G' (gigabyte, 1024M)\n" " and T (terabyte, 1024G) are supported. 'b' is ignored.\n" @@ -180,6 +183,27 @@ static int read_password(char *buf, int buf_size) } #endif +static int set_cache_flag(const char *mode, int *flags) +{ + *flags &= ~BDRV_O_CACHE_MASK; + + if (!strcmp(mode, "none") || !strcmp(mode, "off")) { + *flags |= BDRV_O_CACHE_WB; + *flags |= BDRV_O_NOCACHE; + } else if (!strcmp(mode, "writeback")) { + *flags |= BDRV_O_CACHE_WB; + } else if (!strcmp(mode, "unsafe")) { + *flags |= BDRV_O_CACHE_WB; + *flags |= BDRV_O_NO_FLUSH; + } else if (!strcmp(mode, "writethrough")) { + /* this is the default */ + } else { + return -1; + } + + return 0; +} + static int print_block_option_help(const char *filename, const char *fmt) { BlockDriver *drv, *proto_drv; @@ -441,13 +465,14 @@ static int img_check(int argc, char **argv) static int img_commit(int argc, char **argv) { - int c, ret; - const char *filename, *fmt; + int c, ret, flags; + const char *filename, *fmt, *cache; BlockDriverState *bs; fmt = NULL; + cache = BDRV_DEFAULT_CACHE; for(;;) { - c = getopt(argc, argv, "f:h"); + c = getopt(argc, argv, "f:ht:"); if (c == -1) { break; } @@ -459,6 +484,9 @@ static int img_commit(int argc, char **argv) case 'f': fmt = optarg; break; + case 't': + cache = optarg; + break; } } if (optind >= argc) { @@ -466,7 +494,14 @@ static int img_commit(int argc, char **argv) } filename = argv[optind++]; - bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR); + flags = BDRV_O_RDWR; + ret = set_cache_flag(cache, &flags); + if (ret < 0) { + error_report("Invalid cache option: %s\n", cache); + return -1; + } + + bs = bdrv_new_open(filename, fmt, flags); if (!bs) { return 1; } @@ -591,8 +626,8 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, static int img_convert(int argc, char **argv) { int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors; - int progress = 0; - const char *fmt, *out_fmt, *out_baseimg, *out_filename; + int progress = 0, flags; + const char *fmt, *out_fmt, *cache, *out_baseimg, *out_filename; BlockDriver *drv, *proto_drv; BlockDriverState **bs = NULL, *out_bs = NULL; int64_t total_sectors, nb_sectors, sector_num, bs_offset; @@ -608,10 +643,11 @@ static int img_convert(int argc, char **argv) fmt = NULL; out_fmt = "raw"; + cache = BDRV_DEFAULT_CACHE; out_baseimg = NULL; compress = 0; for(;;) { - c = getopt(argc, argv, "f:O:B:s:hce6o:p"); + c = getopt(argc, argv, "f:O:B:s:hce6o:pt:"); if (c == -1) { break; } @@ -649,6 +685,9 @@ static int img_convert(int argc, char **argv) case 'p': progress = 1; break; + case 't': + cache = optarg; + break; } } @@ -779,8 +818,14 @@ static int img_convert(int argc, char **argv) goto out; } - out_bs = bdrv_new_open(out_filename, out_fmt, - BDRV_O_FLAGS | BDRV_O_RDWR | BDRV_O_NO_FLUSH); + flags = BDRV_O_RDWR; + ret = set_cache_flag(cache, &flags); + if (ret < 0) { + error_report("Invalid cache option: %s\n", cache); + return -1; + } + + out_bs = bdrv_new_open(out_filename, out_fmt, flags); if (!out_bs) { ret = -1; goto out; @@ -1225,18 +1270,18 @@ 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, *out_basefmt, *out_baseimg; + const char *fmt, *cache, *out_basefmt, *out_baseimg; int c, flags, ret; int unsafe = 0; int progress = 0; /* Parse commandline parameters */ fmt = NULL; + cache = BDRV_DEFAULT_CACHE; out_baseimg = NULL; out_basefmt = NULL; - for(;;) { - c = getopt(argc, argv, "uhf:F:b:p"); + c = getopt(argc, argv, "uhf:F:b:pt:"); if (c == -1) { break; } @@ -1260,6 +1305,9 @@ static int img_rebase(int argc, char **argv) case 'p': progress = 1; break; + case 't': + cache = optarg; + break; } } @@ -1271,13 +1319,19 @@ static int img_rebase(int argc, char **argv) qemu_progress_init(progress, 2.0); qemu_progress_print(0, 100); + flags = BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0); + ret = set_cache_flag(cache, &flags); + if (ret < 0) { + error_report("Invalid cache option: %s\n", cache); + return -1; + } + /* * Open the images. * * Ignore the old backing file for unsafe rebase in case we want to correct * the reference to a renamed or moved backing file. */ - flags = BDRV_O_FLAGS | BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0); bs = bdrv_new_open(filename, fmt, flags); if (!bs) { return 1; -- 1.7.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCHv3] qemu-img: Add cache command line option 2011-06-15 13:51 ` [Qemu-devel] [PATCHv2] " Federico Simoncelli @ 2011-06-16 11:58 ` Federico Simoncelli 0 siblings, 0 replies; 9+ messages in thread From: Federico Simoncelli @ 2011-06-16 11:58 UTC (permalink / raw) To: kvm; +Cc: kwolf, Federico Simoncelli, qemu-devel qemu-img currently writes disk images using writeback and filling up the cache buffers which are then flushed by the kernel preventing other processes from accessing the storage. This is particularly bad in cluster environments where time-based algorithms might be in place and accessing the storage within certain timeouts is critical. This patch adds the option to choose a cache method when writing disk images. Signed-off-by: Federico Simoncelli <fsimonce@redhat.com> --- qemu-img-cmds.hx | 6 ++-- qemu-img.c | 80 +++++++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 70 insertions(+), 16 deletions(-) diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index 3072d38..2b70618 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -22,13 +22,13 @@ STEXI ETEXI DEF("commit", img_commit, - "commit [-f fmt] filename") + "commit [-f fmt] [-t cache] filename") STEXI @item commit [-f @var{fmt}] @var{filename} ETEXI DEF("convert", img_convert, - "convert [-c] [-p] [-f fmt] [-O output_fmt] [-o options] [-s snapshot_name] filename [filename2 [...]] output_filename") + "convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] filename [filename2 [...]] output_filename") STEXI @item convert [-c] [-f @var{fmt}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] @var{filename} [@var{filename2} [...]] @var{output_filename} ETEXI @@ -46,7 +46,7 @@ STEXI ETEXI DEF("rebase", img_rebase, - "rebase [-f fmt] [-p] [-u] -b backing_file [-F backing_fmt] filename") + "rebase [-f fmt] [-t cache] [-p] [-u] -b backing_file [-F backing_fmt] filename") STEXI @item rebase [-f @var{fmt}] [-u] -b @var{backing_file} [-F @var{backing_fmt}] @var{filename} ETEXI diff --git a/qemu-img.c b/qemu-img.c index 4f162d1..c2a106e 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -40,6 +40,7 @@ typedef struct img_cmd_t { /* Default to cache=writeback as data integrity is not important for qemu-tcg. */ #define BDRV_O_FLAGS BDRV_O_CACHE_WB +#define BDRV_DEFAULT_CACHE "writeback" static void format_print(void *opaque, const char *name) { @@ -64,6 +65,8 @@ static void help(void) "Command parameters:\n" " 'filename' is a disk image filename\n" " 'fmt' is the disk image format. It is guessed automatically in most cases\n" + " 'cache' is the cache mode used to write the output disk image, the valid\n" + " options are: 'none', 'writeback' (default), 'writethrough' and 'unsafe'\n" " 'size' is the disk image size in bytes. Optional suffixes\n" " 'k' or 'K' (kilobyte, 1024), 'M' (megabyte, 1024k), 'G' (gigabyte, 1024M)\n" " and T (terabyte, 1024G) are supported. 'b' is ignored.\n" @@ -180,6 +183,27 @@ static int read_password(char *buf, int buf_size) } #endif +static int set_cache_flag(const char *mode, int *flags) +{ + *flags &= ~BDRV_O_CACHE_MASK; + + if (!strcmp(mode, "none") || !strcmp(mode, "off")) { + *flags |= BDRV_O_CACHE_WB; + *flags |= BDRV_O_NOCACHE; + } else if (!strcmp(mode, "writeback")) { + *flags |= BDRV_O_CACHE_WB; + } else if (!strcmp(mode, "unsafe")) { + *flags |= BDRV_O_CACHE_WB; + *flags |= BDRV_O_NO_FLUSH; + } else if (!strcmp(mode, "writethrough")) { + /* this is the default */ + } else { + return -1; + } + + return 0; +} + static int print_block_option_help(const char *filename, const char *fmt) { BlockDriver *drv, *proto_drv; @@ -441,13 +465,14 @@ static int img_check(int argc, char **argv) static int img_commit(int argc, char **argv) { - int c, ret; - const char *filename, *fmt; + int c, ret, flags; + const char *filename, *fmt, *cache; BlockDriverState *bs; fmt = NULL; + cache = BDRV_DEFAULT_CACHE; for(;;) { - c = getopt(argc, argv, "f:h"); + c = getopt(argc, argv, "f:ht:"); if (c == -1) { break; } @@ -459,6 +484,9 @@ static int img_commit(int argc, char **argv) case 'f': fmt = optarg; break; + case 't': + cache = optarg; + break; } } if (optind >= argc) { @@ -466,7 +494,14 @@ static int img_commit(int argc, char **argv) } filename = argv[optind++]; - bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR); + flags = BDRV_O_RDWR; + ret = set_cache_flag(cache, &flags); + if (ret < 0) { + error_report("Invalid cache option: %s\n", cache); + return -1; + } + + bs = bdrv_new_open(filename, fmt, flags); if (!bs) { return 1; } @@ -591,8 +626,8 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, static int img_convert(int argc, char **argv) { int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors; - int progress = 0; - const char *fmt, *out_fmt, *out_baseimg, *out_filename; + int progress = 0, flags; + const char *fmt, *out_fmt, *cache, *out_baseimg, *out_filename; BlockDriver *drv, *proto_drv; BlockDriverState **bs = NULL, *out_bs = NULL; int64_t total_sectors, nb_sectors, sector_num, bs_offset; @@ -608,10 +643,11 @@ static int img_convert(int argc, char **argv) fmt = NULL; out_fmt = "raw"; + cache = BDRV_DEFAULT_CACHE; out_baseimg = NULL; compress = 0; for(;;) { - c = getopt(argc, argv, "f:O:B:s:hce6o:p"); + c = getopt(argc, argv, "f:O:B:s:hce6o:pt:"); if (c == -1) { break; } @@ -649,6 +685,9 @@ static int img_convert(int argc, char **argv) case 'p': progress = 1; break; + case 't': + cache = optarg; + break; } } @@ -779,8 +818,14 @@ static int img_convert(int argc, char **argv) goto out; } - out_bs = bdrv_new_open(out_filename, out_fmt, - BDRV_O_FLAGS | BDRV_O_RDWR | BDRV_O_NO_FLUSH); + flags = BDRV_O_RDWR; + ret = set_cache_flag(cache, &flags); + if (ret < 0) { + error_report("Invalid cache option: %s\n", cache); + return -1; + } + + out_bs = bdrv_new_open(out_filename, out_fmt, flags); if (!out_bs) { ret = -1; goto out; @@ -1225,18 +1270,18 @@ 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, *out_basefmt, *out_baseimg; + const char *fmt, *cache, *out_basefmt, *out_baseimg; int c, flags, ret; int unsafe = 0; int progress = 0; /* Parse commandline parameters */ fmt = NULL; + cache = BDRV_DEFAULT_CACHE; out_baseimg = NULL; out_basefmt = NULL; - for(;;) { - c = getopt(argc, argv, "uhf:F:b:p"); + c = getopt(argc, argv, "uhf:F:b:pt:"); if (c == -1) { break; } @@ -1260,6 +1305,9 @@ static int img_rebase(int argc, char **argv) case 'p': progress = 1; break; + case 't': + cache = optarg; + break; } } @@ -1271,13 +1319,19 @@ static int img_rebase(int argc, char **argv) qemu_progress_init(progress, 2.0); qemu_progress_print(0, 100); + flags = BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0); + ret = set_cache_flag(cache, &flags); + if (ret < 0) { + error_report("Invalid cache option: %s\n", cache); + return -1; + } + /* * Open the images. * * Ignore the old backing file for unsafe rebase in case we want to correct * the reference to a renamed or moved backing file. */ - flags = BDRV_O_FLAGS | BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0); bs = bdrv_new_open(filename, fmt, flags); if (!bs) { return 1; -- 1.7.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-img: Add cache command line option 2011-06-15 13:46 ` [Qemu-devel] [PATCH] qemu-img: Add cache command line option Federico Simoncelli 2011-06-15 13:51 ` [Qemu-devel] [PATCHv2] " Federico Simoncelli @ 2011-06-16 14:28 ` Christoph Hellwig 2011-06-16 14:43 ` Kevin Wolf 2011-06-16 14:44 ` [Qemu-devel] [PATCH] " Federico Simoncelli 1 sibling, 2 replies; 9+ messages in thread From: Christoph Hellwig @ 2011-06-16 14:28 UTC (permalink / raw) To: Federico Simoncelli; +Cc: kwolf, qemu-devel, kvm, avi On Wed, Jun 15, 2011 at 09:46:10AM -0400, Federico Simoncelli wrote: > qemu-img currently writes disk images using writeback and filling > up the cache buffers which are then flushed by the kernel preventing > other processes from accessing the storage. > This is particularly bad in cluster environments where time-based > algorithms might be in place and accessing the storage within > certain timeouts is critical. > This patch adds the option to choose a cache method when writing > disk images. Allowing to chose the mode is of course fine, but what about also choosing a good default? writethrough doesn't really make any sense for qemu-img, given that we can trivially flush the cache at the end of the operations. I'd also say that using the buffer cache doesn't make sense either, as there is little point in caching these operations. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-img: Add cache command line option 2011-06-16 14:28 ` [Qemu-devel] [PATCH] " Christoph Hellwig @ 2011-06-16 14:43 ` Kevin Wolf 2011-06-20 14:47 ` Kevin Wolf 2011-06-16 14:44 ` [Qemu-devel] [PATCH] " Federico Simoncelli 1 sibling, 1 reply; 9+ messages in thread From: Kevin Wolf @ 2011-06-16 14:43 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Federico Simoncelli, qemu-devel, kvm, avi Am 16.06.2011 16:28, schrieb Christoph Hellwig: > On Wed, Jun 15, 2011 at 09:46:10AM -0400, Federico Simoncelli wrote: >> qemu-img currently writes disk images using writeback and filling >> up the cache buffers which are then flushed by the kernel preventing >> other processes from accessing the storage. >> This is particularly bad in cluster environments where time-based >> algorithms might be in place and accessing the storage within >> certain timeouts is critical. >> This patch adds the option to choose a cache method when writing >> disk images. > > Allowing to chose the mode is of course fine, but what about also > choosing a good default? writethrough doesn't really make any sense > for qemu-img, given that we can trivially flush the cache at the end > of the operations. I'd also say that using the buffer cache doesn't > make sense either, as there is little point in caching these operations. Right, we need to keep the defaults as they are. That is, for convert unsafe and for everything else writeback. The patch seems to make writeback the default for everything. Kevin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-img: Add cache command line option 2011-06-16 14:43 ` Kevin Wolf @ 2011-06-20 14:47 ` Kevin Wolf 2011-06-20 16:48 ` [Qemu-devel] [PATCHv4] " Federico Simoncelli 0 siblings, 1 reply; 9+ messages in thread From: Kevin Wolf @ 2011-06-20 14:47 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Federico Simoncelli, qemu-devel, kvm, avi Am 16.06.2011 16:43, schrieb Kevin Wolf: > Am 16.06.2011 16:28, schrieb Christoph Hellwig: >> On Wed, Jun 15, 2011 at 09:46:10AM -0400, Federico Simoncelli wrote: >>> qemu-img currently writes disk images using writeback and filling >>> up the cache buffers which are then flushed by the kernel preventing >>> other processes from accessing the storage. >>> This is particularly bad in cluster environments where time-based >>> algorithms might be in place and accessing the storage within >>> certain timeouts is critical. >>> This patch adds the option to choose a cache method when writing >>> disk images. >> >> Allowing to chose the mode is of course fine, but what about also >> choosing a good default? writethrough doesn't really make any sense >> for qemu-img, given that we can trivially flush the cache at the end >> of the operations. I'd also say that using the buffer cache doesn't >> make sense either, as there is little point in caching these operations. > > Right, we need to keep the defaults as they are. That is, for convert > unsafe and for everything else writeback. The patch seems to make > writeback the default for everything. Federico, are you going to fix this in a v4? Kevin ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCHv4] qemu-img: Add cache command line option 2011-06-20 14:47 ` Kevin Wolf @ 2011-06-20 16:48 ` Federico Simoncelli 2011-06-29 9:48 ` Kevin Wolf 0 siblings, 1 reply; 9+ messages in thread From: Federico Simoncelli @ 2011-06-20 16:48 UTC (permalink / raw) To: kwolf; +Cc: Federico Simoncelli, avi, qemu-devel, kvm, hch qemu-img currently writes disk images using writeback and filling up the cache buffers which are then flushed by the kernel preventing other processes from accessing the storage. This is particularly bad in cluster environments where time-based algorithms might be in place and accessing the storage within certain timeouts is critical. This patch adds the option to choose a cache method when writing disk images. Signed-off-by: Federico Simoncelli <fsimonce@redhat.com> --- qemu-img-cmds.hx | 6 ++-- qemu-img.c | 80 +++++++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 70 insertions(+), 16 deletions(-) diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index 3072d38..2b70618 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -22,13 +22,13 @@ STEXI ETEXI DEF("commit", img_commit, - "commit [-f fmt] filename") + "commit [-f fmt] [-t cache] filename") STEXI @item commit [-f @var{fmt}] @var{filename} ETEXI DEF("convert", img_convert, - "convert [-c] [-p] [-f fmt] [-O output_fmt] [-o options] [-s snapshot_name] filename [filename2 [...]] output_filename") + "convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] filename [filename2 [...]] output_filename") STEXI @item convert [-c] [-f @var{fmt}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] @var{filename} [@var{filename2} [...]] @var{output_filename} ETEXI @@ -46,7 +46,7 @@ STEXI ETEXI DEF("rebase", img_rebase, - "rebase [-f fmt] [-p] [-u] -b backing_file [-F backing_fmt] filename") + "rebase [-f fmt] [-t cache] [-p] [-u] -b backing_file [-F backing_fmt] filename") STEXI @item rebase [-f @var{fmt}] [-u] -b @var{backing_file} [-F @var{backing_fmt}] @var{filename} ETEXI diff --git a/qemu-img.c b/qemu-img.c index 4f162d1..f904e32 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -40,6 +40,7 @@ typedef struct img_cmd_t { /* Default to cache=writeback as data integrity is not important for qemu-tcg. */ #define BDRV_O_FLAGS BDRV_O_CACHE_WB +#define BDRV_DEFAULT_CACHE "writeback" static void format_print(void *opaque, const char *name) { @@ -64,6 +65,8 @@ static void help(void) "Command parameters:\n" " 'filename' is a disk image filename\n" " 'fmt' is the disk image format. It is guessed automatically in most cases\n" + " 'cache' is the cache mode used to write the output disk image, the valid\n" + " options are: 'none', 'writeback' (default), 'writethrough' and 'unsafe'\n" " 'size' is the disk image size in bytes. Optional suffixes\n" " 'k' or 'K' (kilobyte, 1024), 'M' (megabyte, 1024k), 'G' (gigabyte, 1024M)\n" " and T (terabyte, 1024G) are supported. 'b' is ignored.\n" @@ -180,6 +183,27 @@ static int read_password(char *buf, int buf_size) } #endif +static int set_cache_flag(const char *mode, int *flags) +{ + *flags &= ~BDRV_O_CACHE_MASK; + + if (!strcmp(mode, "none") || !strcmp(mode, "off")) { + *flags |= BDRV_O_CACHE_WB; + *flags |= BDRV_O_NOCACHE; + } else if (!strcmp(mode, "writeback")) { + *flags |= BDRV_O_CACHE_WB; + } else if (!strcmp(mode, "unsafe")) { + *flags |= BDRV_O_CACHE_WB; + *flags |= BDRV_O_NO_FLUSH; + } else if (!strcmp(mode, "writethrough")) { + /* this is the default */ + } else { + return -1; + } + + return 0; +} + static int print_block_option_help(const char *filename, const char *fmt) { BlockDriver *drv, *proto_drv; @@ -441,13 +465,14 @@ static int img_check(int argc, char **argv) static int img_commit(int argc, char **argv) { - int c, ret; - const char *filename, *fmt; + int c, ret, flags; + const char *filename, *fmt, *cache; BlockDriverState *bs; fmt = NULL; + cache = BDRV_DEFAULT_CACHE; for(;;) { - c = getopt(argc, argv, "f:h"); + c = getopt(argc, argv, "f:ht:"); if (c == -1) { break; } @@ -459,6 +484,9 @@ static int img_commit(int argc, char **argv) case 'f': fmt = optarg; break; + case 't': + cache = optarg; + break; } } if (optind >= argc) { @@ -466,7 +494,14 @@ static int img_commit(int argc, char **argv) } filename = argv[optind++]; - bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR); + flags = BDRV_O_RDWR; + ret = set_cache_flag(cache, &flags); + if (ret < 0) { + error_report("Invalid cache option: %s\n", cache); + return -1; + } + + bs = bdrv_new_open(filename, fmt, flags); if (!bs) { return 1; } @@ -591,8 +626,8 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, static int img_convert(int argc, char **argv) { int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors; - int progress = 0; - const char *fmt, *out_fmt, *out_baseimg, *out_filename; + int progress = 0, flags; + const char *fmt, *out_fmt, *cache, *out_baseimg, *out_filename; BlockDriver *drv, *proto_drv; BlockDriverState **bs = NULL, *out_bs = NULL; int64_t total_sectors, nb_sectors, sector_num, bs_offset; @@ -608,10 +643,11 @@ static int img_convert(int argc, char **argv) fmt = NULL; out_fmt = "raw"; + cache = "unsafe"; out_baseimg = NULL; compress = 0; for(;;) { - c = getopt(argc, argv, "f:O:B:s:hce6o:p"); + c = getopt(argc, argv, "f:O:B:s:hce6o:pt:"); if (c == -1) { break; } @@ -649,6 +685,9 @@ static int img_convert(int argc, char **argv) case 'p': progress = 1; break; + case 't': + cache = optarg; + break; } } @@ -779,8 +818,14 @@ static int img_convert(int argc, char **argv) goto out; } - out_bs = bdrv_new_open(out_filename, out_fmt, - BDRV_O_FLAGS | BDRV_O_RDWR | BDRV_O_NO_FLUSH); + flags = BDRV_O_RDWR; + ret = set_cache_flag(cache, &flags); + if (ret < 0) { + error_report("Invalid cache option: %s\n", cache); + return -1; + } + + out_bs = bdrv_new_open(out_filename, out_fmt, flags); if (!out_bs) { ret = -1; goto out; @@ -1225,18 +1270,18 @@ 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, *out_basefmt, *out_baseimg; + const char *fmt, *cache, *out_basefmt, *out_baseimg; int c, flags, ret; int unsafe = 0; int progress = 0; /* Parse commandline parameters */ fmt = NULL; + cache = BDRV_DEFAULT_CACHE; out_baseimg = NULL; out_basefmt = NULL; - for(;;) { - c = getopt(argc, argv, "uhf:F:b:p"); + c = getopt(argc, argv, "uhf:F:b:pt:"); if (c == -1) { break; } @@ -1260,6 +1305,9 @@ static int img_rebase(int argc, char **argv) case 'p': progress = 1; break; + case 't': + cache = optarg; + break; } } @@ -1271,13 +1319,19 @@ static int img_rebase(int argc, char **argv) qemu_progress_init(progress, 2.0); qemu_progress_print(0, 100); + flags = BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0); + ret = set_cache_flag(cache, &flags); + if (ret < 0) { + error_report("Invalid cache option: %s\n", cache); + return -1; + } + /* * Open the images. * * Ignore the old backing file for unsafe rebase in case we want to correct * the reference to a renamed or moved backing file. */ - flags = BDRV_O_FLAGS | BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0); bs = bdrv_new_open(filename, fmt, flags); if (!bs) { return 1; -- 1.7.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCHv4] qemu-img: Add cache command line option 2011-06-20 16:48 ` [Qemu-devel] [PATCHv4] " Federico Simoncelli @ 2011-06-29 9:48 ` Kevin Wolf 0 siblings, 0 replies; 9+ messages in thread From: Kevin Wolf @ 2011-06-29 9:48 UTC (permalink / raw) To: Federico Simoncelli; +Cc: avi, qemu-devel, kvm, hch Am 20.06.2011 18:48, schrieb Federico Simoncelli: > qemu-img currently writes disk images using writeback and filling > up the cache buffers which are then flushed by the kernel preventing > other processes from accessing the storage. > This is particularly bad in cluster environments where time-based > algorithms might be in place and accessing the storage within > certain timeouts is critical. > This patch adds the option to choose a cache method when writing > disk images. > > Signed-off-by: Federico Simoncelli <fsimonce@redhat.com> Thanks, applied to the block branch. Kevin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-img: Add cache command line option 2011-06-16 14:28 ` [Qemu-devel] [PATCH] " Christoph Hellwig 2011-06-16 14:43 ` Kevin Wolf @ 2011-06-16 14:44 ` Federico Simoncelli 1 sibling, 0 replies; 9+ messages in thread From: Federico Simoncelli @ 2011-06-16 14:44 UTC (permalink / raw) To: Christoph Hellwig; +Cc: kwolf, qemu-devel, kvm, avi ----- Original Message ----- > From: "Christoph Hellwig" <hch@lst.de> > To: "Federico Simoncelli" <fsimonce@redhat.com> > Cc: kvm@vger.kernel.org, kwolf@redhat.com, qemu-devel@nongnu.org, avi@redhat.com > Sent: Thursday, June 16, 2011 4:28:09 PM > Subject: Re: [Qemu-devel] [PATCH] qemu-img: Add cache command line option > On Wed, Jun 15, 2011 at 09:46:10AM -0400, Federico Simoncelli wrote: > > qemu-img currently writes disk images using writeback and filling > > up the cache buffers which are then flushed by the kernel preventing > > other processes from accessing the storage. > > This is particularly bad in cluster environments where time-based > > algorithms might be in place and accessing the storage within > > certain timeouts is critical. > > This patch adds the option to choose a cache method when writing > > disk images. > > Allowing to chose the mode is of course fine, but what about also > choosing a good default? writethrough doesn't really make any sense > for qemu-img, given that we can trivially flush the cache at the end > of the operations. I'd also say that using the buffer cache doesn't > make sense either, as there is little point in caching these > operations. I totally agree with you, I didn't change the default to increase the chances for my patch to be accepted. If we want cache=none as default we can easily change: -#define BDRV_DEFAULT_CACHE "writeback" +#define BDRV_DEFAULT_CACHE "none" -- Federico ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-06-29 9:45 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <4DF8A4FD.6050807@redhat.com>
2011-06-15 13:46 ` [Qemu-devel] [PATCH] qemu-img: Add cache command line option Federico Simoncelli
2011-06-15 13:51 ` [Qemu-devel] [PATCHv2] " Federico Simoncelli
2011-06-16 11:58 ` [Qemu-devel] [PATCHv3] " Federico Simoncelli
2011-06-16 14:28 ` [Qemu-devel] [PATCH] " Christoph Hellwig
2011-06-16 14:43 ` Kevin Wolf
2011-06-20 14:47 ` Kevin Wolf
2011-06-20 16:48 ` [Qemu-devel] [PATCHv4] " Federico Simoncelli
2011-06-29 9:48 ` Kevin Wolf
2011-06-16 14:44 ` [Qemu-devel] [PATCH] " Federico Simoncelli
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).