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

[Kevin Wolf <kwolf@redhat.com>]

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