Am 15.06.2011 13:17, schrieb Federico Simoncelli:
> ----- Original Message -----
>> From: "Kevin Wolf" <kwolf@xxxxxxxxxx>
>> To: "Avi Kivity" <avi@xxxxxxxxxx>
>> Cc: "Federico Simoncelli" <fsimonce@xxxxxxxxxx>, kvm@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxx 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
