[PATCH] qemu-img: Add nocache command line option

[PATCH] qemu-img: Add nocache command line option

[Federico Simoncelli]

qemu-img currently writes 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 use nocache when other solutions
(eg: cgroups) are not available.

Signed-off-by: Federico Simoncelli <fsimonce@redhat.com>
---
 qemu-img.c |   33 ++++++++++++++++++++++++---------
 1 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 4f162d1..c10e4a6 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -78,6 +78,7 @@ static void help(void)
            "       rebasing in this case (useful for renaming the backing file)\n"
            "  '-h' with or without a command shows this help and lists the supported formats\n"
            "  '-p' show progress of command (only certain commands)\n"
+           "  '-t' use write-through (no cache), valid with: convert, commit and rebase\n"
            "\n"
            "Parameters to snapshot subcommand:\n"
            "  'snapshot' is the name of the snapshot to create, apply or delete\n"
@@ -441,13 +442,14 @@ static int img_check(int argc, char **argv)
 
 static int img_commit(int argc, char **argv)
 {
-    int c, ret;
+    int c, ret, flags;
     const char *filename, *fmt;
     BlockDriverState *bs;
 
     fmt = NULL;
+    flags = BDRV_O_FLAGS | BDRV_O_RDWR;
     for(;;) {
-        c = getopt(argc, argv, "f:h");
+        c = getopt(argc, argv, "f:ht");
         if (c == -1) {
             break;
         }
@@ -459,6 +461,10 @@ static int img_commit(int argc, char **argv)
         case 'f':
             fmt = optarg;
             break;
+        case 't':
+            flags &= ~BDRV_O_CACHE_WB;
+            flags |= BDRV_O_NOCACHE;
+            break;
         }
     }
     if (optind >= argc) {
@@ -466,7 +472,7 @@ static int img_commit(int argc, char **argv)
     }
     filename = argv[optind++];
 
-    bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR);
+    bs = bdrv_new_open(filename, fmt, flags);
     if (!bs) {
         return 1;
     }
@@ -591,7 +597,7 @@ 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;
+    int progress = 0, flags;
     const char *fmt, *out_fmt, *out_baseimg, *out_filename;
     BlockDriver *drv, *proto_drv;
     BlockDriverState **bs = NULL, *out_bs = NULL;
@@ -610,8 +616,9 @@ static int img_convert(int argc, char **argv)
     out_fmt = "raw";
     out_baseimg = NULL;
     compress = 0;
+    flags = BDRV_O_FLAGS | BDRV_O_RDWR | BDRV_O_NO_FLUSH;
     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 +656,10 @@ static int img_convert(int argc, char **argv)
         case 'p':
             progress = 1;
             break;
+        case 't':
+            flags &= ~BDRV_O_CACHE_WB;
+            flags |= BDRV_O_NOCACHE;
+            break;
         }
     }
 
@@ -779,8 +790,7 @@ 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);
+    out_bs = bdrv_new_open(out_filename, out_fmt, flags);
     if (!out_bs) {
         ret = -1;
         goto out;
@@ -1234,9 +1244,10 @@ static int img_rebase(int argc, char **argv)
     fmt = NULL;
     out_baseimg = NULL;
     out_basefmt = NULL;
+    flags = BDRV_O_FLAGS | BDRV_O_RDWR;
 
     for(;;) {
-        c = getopt(argc, argv, "uhf:F:b:p");
+        c = getopt(argc, argv, "uhf:F:b:pt");
         if (c == -1) {
             break;
         }
@@ -1256,10 +1267,15 @@ 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':
+            flags &= ~BDRV_O_CACHE_WB;
+            flags |= BDRV_O_NOCACHE;
+            break;
         }
     }
 
@@ -1277,7 +1293,6 @@ static int img_rebase(int argc, char **argv)
      * 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

Re: [PATCH] qemu-img: Add nocache command line option

[Avi Kivity]

On 06/14/2011 03:09 PM, Federico Simoncelli wrote:
> qemu-img currently writes 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 use nocache when other solutions
> (eg: cgroups) are not available.
>

Please copy qemu-devel@nongnu.org as this is not a kvm-specific patch.

> diff --git a/qemu-img.c b/qemu-img.c
> index 4f162d1..c10e4a6 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -78,6 +78,7 @@ static void help(void)
>              "       rebasing in this case (useful for renaming the backing file)\n"
>              "  '-h' with or without a command shows this help and lists the supported formats\n"
>              "  '-p' show progress of command (only certain commands)\n"
> +           "  '-t' use write-through (no cache), valid with: convert, commit and rebase\n"
>              "\n"
>              "Parameters to snapshot subcommand:\n"
>              "  'snapshot' is the name of the snapshot to create, apply or delete\n"

IMO better to use the existing qemu option format, say -o 
cache=writeback|writethrough|none|volatile.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] qemu-img: Add nocache command line option

[Kevin Wolf]

Am 14.06.2011 15:13, schrieb Avi Kivity:
> On 06/14/2011 03:09 PM, Federico Simoncelli wrote:
>> qemu-img currently writes 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 use nocache when other solutions
>> (eg: cgroups) are not available.

Your patch confuses a write-through cache (cache=writethrough == O_SYNC)
with bypassing the cache (cache=none == O_DIRECT), which are entirely
different.

> Please copy qemu-devel@nongnu.org as this is not a kvm-specific patch.
> 
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 4f162d1..c10e4a6 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -78,6 +78,7 @@ static void help(void)
>>              "       rebasing in this case (useful for renaming the backing file)\n"
>>              "  '-h' with or without a command shows this help and lists the supported formats\n"
>>              "  '-p' show progress of command (only certain commands)\n"
>> +           "  '-t' use write-through (no cache), valid with: convert, commit and rebase\n"
>>              "\n"
>>              "Parameters to snapshot subcommand:\n"
>>              "  'snapshot' is the name of the snapshot to create, apply or delete\n"
> 
> IMO better to use the existing qemu option format, say -o 
> cache=writeback|writethrough|none|volatile.

No, -o is used with qemu-img create for options that change what the
image will look like (e.g. setting flags in the image header). This
options is about the cache mode that is used for various operations.

Kevin

Re: [PATCH] qemu-img: Add nocache command line option

[Federico Simoncelli]

----- Original Message -----
> From: "Kevin Wolf" <kwolf@redhat.com>
> To: "Avi Kivity" <avi@redhat.com>
> Cc: "Federico Simoncelli" <fsimonce@redhat.com>, kvm@vger.kernel.org
> Sent: Wednesday, June 15, 2011 12:50:21 PM
> Subject: Re: [PATCH] qemu-img: Add nocache command line option
> Am 14.06.2011 15:13, schrieb Avi Kivity:
> > On 06/14/2011 03:09 PM, Federico Simoncelli wrote:
> >> qemu-img currently writes 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 use nocache when other solutions
> >> (eg: cgroups) are not available.
> 
> Your patch confuses a write-through cache (cache=writethrough ==
> O_SYNC)
> with bypassing the cache (cache=none == O_DIRECT), which are entirely
> different.

I know the difference, how am I confusing them in the patch?
I am using BDRV_O_NOCACHE which is translated to O_DIRECT, which is
what I wanted to use (as per commit message: nocache).
Maybe I overlooked something, can you point me to the code parts you
suppose are wrong?

> > Please copy qemu-devel@nongnu.org as this is not a kvm-specific
> > patch.
> >
> >> diff --git a/qemu-img.c b/qemu-img.c
> >> index 4f162d1..c10e4a6 100644
> >> --- a/qemu-img.c
> >> +++ b/qemu-img.c
> >> @@ -78,6 +78,7 @@ static void help(void)
> >>              " rebasing in this case (useful for renaming the
> >>              backing file)\n"
> >>              " '-h' with or without a command shows this help and
> >>              lists the supported formats\n"
> >>              " '-p' show progress of command (only certain
> >>              commands)\n"
> >> + " '-t' use write-through (no cache), valid with: convert, commit
> >> and rebase\n"
> >>              "\n"
> >>              "Parameters to snapshot subcommand:\n"
> >>              " 'snapshot' is the name of the snapshot to create,
> >>              apply or delete\n"
> >
> > IMO better to use the existing qemu option format, say -o
> > cache=writeback|writethrough|none|volatile.
> 
> No, -o is used with qemu-img create for options that change what the
> image will look like (e.g. setting flags in the image header). This
> options is about the cache mode that is used for various operations.

Actually I am just working over this. I am going to add a qemu-img
specific option, roughly something like this:

static QEMUOptionParameter qemuimg_options[] = {
    {
        .name = QEMUIMG_OPT_CACHE,
        .type = OPT_STRING,
        .help = "Write cache method"
    },
    { NULL }
};
[...]
    create_options = append_option_parameters(create_options,
                                              qemuimg_options);
[...]
    /* Setting output cache flag */
    out_flags = BDRV_O_RDWR | BDRV_O_NO_FLUSH;
    ret = qemuimg_option_cache(param, &out_flags);
    if (ret < 0) {
        goto out;
    }

-- 
Federico

Re: [PATCH] qemu-img: Add nocache command line option

[Kevin Wolf]

Am 15.06.2011 13:17, schrieb Federico Simoncelli:
> ----- Original Message -----
>> From: "Kevin Wolf" <kwolf@redhat.com>
>> To: "Avi Kivity" <avi@redhat.com>
>> Cc: "Federico Simoncelli" <fsimonce@redhat.com>, kvm@vger.kernel.org
>> Sent: Wednesday, June 15, 2011 12:50:21 PM
>> Subject: Re: [PATCH] qemu-img: Add nocache command line option
>> Am 14.06.2011 15:13, schrieb Avi Kivity:
>>> On 06/14/2011 03:09 PM, Federico Simoncelli wrote:
>>>> qemu-img currently writes 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 use nocache when other solutions
>>>> (eg: cgroups) are not available.
>>
>> Your patch confuses a write-through cache (cache=writethrough ==
>> O_SYNC)
>> with bypassing the cache (cache=none == O_DIRECT), which are entirely
>> different.
> 
> I know the difference, how am I confusing them in the patch?
> I am using BDRV_O_NOCACHE which is translated to O_DIRECT, which is
> what I wanted to use (as per commit message: nocache).
> Maybe I overlooked something, can you point me to the code parts you
> suppose are wrong?

The first thing that I noticed is that you call it write-through in the
help message. But you also unset BDRV_O_CACHE_WB, while cache=none is a
write-back cache mode, in fact.

>>> Please copy qemu-devel@nongnu.org as this is not a kvm-specific
>>> patch.
>>>
>>>> diff --git a/qemu-img.c b/qemu-img.c
>>>> index 4f162d1..c10e4a6 100644
>>>> --- a/qemu-img.c
>>>> +++ b/qemu-img.c
>>>> @@ -78,6 +78,7 @@ static void help(void)
>>>>              " rebasing in this case (useful for renaming the
>>>>              backing file)\n"
>>>>              " '-h' with or without a command shows this help and
>>>>              lists the supported formats\n"
>>>>              " '-p' show progress of command (only certain
>>>>              commands)\n"
>>>> + " '-t' use write-through (no cache), valid with: convert, commit
>>>> and rebase\n"
>>>>              "\n"
>>>>              "Parameters to snapshot subcommand:\n"
>>>>              " 'snapshot' is the name of the snapshot to create,
>>>>              apply or delete\n"
>>>
>>> IMO better to use the existing qemu option format, say -o
>>> cache=writeback|writethrough|none|volatile.
>>
>> No, -o is used with qemu-img create for options that change what the
>> image will look like (e.g. setting flags in the image header). This
>> options is about the cache mode that is used for various operations.
> 
> Actually I am just working over this. I am going to add a qemu-img
> specific option, roughly something like this:
> 
> static QEMUOptionParameter qemuimg_options[] = {
>     {
>         .name = QEMUIMG_OPT_CACHE,
>         .type = OPT_STRING,
>         .help = "Write cache method"
>     },
>     { NULL }
> };
> [...]
>     create_options = append_option_parameters(create_options,
>                                               qemuimg_options);
> [...]
>     /* Setting output cache flag */
>     out_flags = BDRV_O_RDWR | BDRV_O_NO_FLUSH;
>     ret = qemuimg_option_cache(param, &out_flags);
>     if (ret < 0) {
>         goto out;
>     }

My concerns were not about the implementation (I'm sure that you can do
that), but rather on a conceptual level. Persistent image options are
semantically different from options only used for the qemu-img
operation, so mixing the two things gives you a bad user interface.

Kevin

Re: [PATCH] qemu-img: Add nocache command line option

[Federico Simoncelli]

----- Original Message -----
> From: "Kevin Wolf" <kwolf@redhat.com>
> To: "Federico Simoncelli" <fsimonce@redhat.com>
> Cc: kvm@vger.kernel.org, "Avi Kivity" <avi@redhat.com>
> Sent: Wednesday, June 15, 2011 1:45:02 PM
> Subject: Re: [PATCH] qemu-img: Add nocache command line option
> Am 15.06.2011 13:17, schrieb Federico Simoncelli:
> > ----- Original Message -----
> >> From: "Kevin Wolf" <kwolf@redhat.com>
> >> To: "Avi Kivity" <avi@redhat.com>
> >> Cc: "Federico Simoncelli" <fsimonce@redhat.com>,
> >> kvm@vger.kernel.org
> >> Sent: Wednesday, June 15, 2011 12:50:21 PM
> >> Subject: Re: [PATCH] qemu-img: Add nocache command line option
> >>
> >> Your patch confuses a write-through cache (cache=writethrough ==
> >> O_SYNC)
> >> with bypassing the cache (cache=none == O_DIRECT), which are
> >> entirely
> >> different.
> >
> > I know the difference, how am I confusing them in the patch?
> > I am using BDRV_O_NOCACHE which is translated to O_DIRECT, which is
> > what I wanted to use (as per commit message: nocache).
> > Maybe I overlooked something, can you point me to the code parts you
> > suppose are wrong?
> 
> The first thing that I noticed is that you call it write-through in
> the
> help message. But you also unset BDRV_O_CACHE_WB, while cache=none is
> a
> write-back cache mode, in fact.

You are totally right, I overlooked the help message!
What confuses me is that cache=none shouldn't be write-back (strictly
speaking), but it probably does just to avoid the use of O_DIRECT
with O_DSYNC:

[blockdev.c:330]

  if (!strcmp(buf, "off") || !strcmp(buf, "none")) {
      bdrv_flags |= BDRV_O_NOCACHE | BDRV_O_CACHE_WB;
  } else if [...]

[block/raw-posix.c:203]

  if ((bdrv_flags & BDRV_O_NOCACHE))
      s->open_flags |= O_DIRECT;
  if (!(bdrv_flags & BDRV_O_CACHE_WB))
      s->open_flags |= O_DSYNC;

> >>> IMO better to use the existing qemu option format, say -o
> >>> cache=writeback|writethrough|none|volatile.
> >>
> >> No, -o is used with qemu-img create for options that change what
> >> the
> >> image will look like (e.g. setting flags in the image header). This
> >> options is about the cache mode that is used for various
> >> operations.
> >
> > Actually I am just working over this. I am going to add a qemu-img
> > specific option, roughly something like this:
> >
> > static QEMUOptionParameter qemuimg_options[] = {
> >     {
> >         .name = QEMUIMG_OPT_CACHE,
> >         .type = OPT_STRING,
> >         .help = "Write cache method"
> >     },
> >     { NULL }
> > };
> > [...]
> >     create_options = append_option_parameters(create_options,
> >                                               qemuimg_options);
> > [...]
> >     /* Setting output cache flag */
> >     out_flags = BDRV_O_RDWR | BDRV_O_NO_FLUSH;
> >     ret = qemuimg_option_cache(param, &out_flags);
> >     if (ret < 0) {
> >         goto out;
> >     }
> 
> My concerns were not about the implementation (I'm sure that you can
> do
> that), but rather on a conceptual level. Persistent image options are
> semantically different from options only used for the qemu-img
> operation, so mixing the two things gives you a bad user interface.

What I understood was that we were trying to conform to the qemu-kvm
-drive option which already looks very similar:

 cache=<mode>,format=<fmt>,...

So in the final analysis which one do you prefer?

 qemu-img -t none|writeback|writethrough ...
 qemu-img -o cache=none|writeback|writethrough ...

-- 
Federico

Re: [PATCH] qemu-img: Add nocache command line option

[Kevin Wolf]

Am 15.06.2011 14:15, schrieb Federico Simoncelli:
> ----- Original Message -----
>> From: "Kevin Wolf" <kwolf@redhat.com>
>> To: "Federico Simoncelli" <fsimonce@redhat.com>
>> Cc: kvm@vger.kernel.org, "Avi Kivity" <avi@redhat.com>
>> Sent: Wednesday, June 15, 2011 1:45:02 PM
>> Subject: Re: [PATCH] qemu-img: Add nocache command line option
>> Am 15.06.2011 13:17, schrieb Federico Simoncelli:
>>> ----- Original Message -----
>>>> From: "Kevin Wolf" <kwolf@redhat.com>
>>>> To: "Avi Kivity" <avi@redhat.com>
>>>> Cc: "Federico Simoncelli" <fsimonce@redhat.com>,
>>>> kvm@vger.kernel.org
>>>> Sent: Wednesday, June 15, 2011 12:50:21 PM
>>>> Subject: Re: [PATCH] qemu-img: Add nocache command line option
>>>>
>>>> Your patch confuses a write-through cache (cache=writethrough ==
>>>> O_SYNC)
>>>> with bypassing the cache (cache=none == O_DIRECT), which are
>>>> entirely
>>>> different.
>>>
>>> I know the difference, how am I confusing them in the patch?
>>> I am using BDRV_O_NOCACHE which is translated to O_DIRECT, which is
>>> what I wanted to use (as per commit message: nocache).
>>> Maybe I overlooked something, can you point me to the code parts you
>>> suppose are wrong?
>>
>> The first thing that I noticed is that you call it write-through in
>> the
>> help message. But you also unset BDRV_O_CACHE_WB, while cache=none is
>> a
>> write-back cache mode, in fact.
> 
> You are totally right, I overlooked the help message!
> What confuses me is that cache=none shouldn't be write-back (strictly
> speaking), but it probably does just to avoid the use of O_DIRECT
> with O_DSYNC:

And this is exactly what makes it a write-back mode.

I know that people often don't expect this, but even though we're
bypassing the host page cache, it's a write-back mode. The data might be
cached in a volatile write cache of the disk, for example.

> [blockdev.c:330]
> 
>   if (!strcmp(buf, "off") || !strcmp(buf, "none")) {
>       bdrv_flags |= BDRV_O_NOCACHE | BDRV_O_CACHE_WB;
>   } else if [...]
> 
> [block/raw-posix.c:203]
> 
>   if ((bdrv_flags & BDRV_O_NOCACHE))
>       s->open_flags |= O_DIRECT;
>   if (!(bdrv_flags & BDRV_O_CACHE_WB))
>       s->open_flags |= O_DSYNC;
> 
>>>>> IMO better to use the existing qemu option format, say -o
>>>>> cache=writeback|writethrough|none|volatile.
>>>>
>>>> No, -o is used with qemu-img create for options that change what
>>>> the
>>>> image will look like (e.g. setting flags in the image header). This
>>>> options is about the cache mode that is used for various
>>>> operations.
>>>
>>> Actually I am just working over this. I am going to add a qemu-img
>>> specific option, roughly something like this:
>>>
>>> static QEMUOptionParameter qemuimg_options[] = {
>>>     {
>>>         .name = QEMUIMG_OPT_CACHE,
>>>         .type = OPT_STRING,
>>>         .help = "Write cache method"
>>>     },
>>>     { NULL }
>>> };
>>> [...]
>>>     create_options = append_option_parameters(create_options,
>>>                                               qemuimg_options);
>>> [...]
>>>     /* Setting output cache flag */
>>>     out_flags = BDRV_O_RDWR | BDRV_O_NO_FLUSH;
>>>     ret = qemuimg_option_cache(param, &out_flags);
>>>     if (ret < 0) {
>>>         goto out;
>>>     }
>>
>> My concerns were not about the implementation (I'm sure that you can
>> do
>> that), but rather on a conceptual level. Persistent image options are
>> semantically different from options only used for the qemu-img
>> operation, so mixing the two things gives you a bad user interface.
> 
> What I understood was that we were trying to conform to the qemu-kvm
> -drive option which already looks very similar:
> 
>  cache=<mode>,format=<fmt>,...
> 
> So in the final analysis which one do you prefer?
> 
>  qemu-img -t none|writeback|writethrough ...
>  qemu-img -o cache=none|writeback|writethrough ...

I would pick the former.

Maybe we can even use a cache=<mode>,format=<fmt> style thing that
resembles -drive. Just don't make it -o but something separate.

Kevin

[PATCH] qemu-img: Add cache command line option

[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>
---
 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

[PATCHv2] qemu-img: Add cache command line option

[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>
---
 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

[PATCHv3] qemu-img: Add cache command line option

[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>
---
 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

Re: [Qemu-devel] [PATCH] qemu-img: Add cache command line option

[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.

Re: [Qemu-devel] [PATCH] qemu-img: Add cache command line option

[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.

Kevin

Re: [Qemu-devel] [PATCH] qemu-img: Add cache command line option

[Federico Simoncelli]

----- 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

Re: [Qemu-devel] [PATCH] qemu-img: Add cache command line option

[Kevin Wolf]

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

[PATCHv4] qemu-img: Add cache command line option

[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>
---
 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

Re: [PATCHv4] qemu-img: Add cache command line option

[Kevin Wolf]

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