Re: [Qemu-devel] [PATCH] qcow2: adjust qcow2_co_flush_to_os -> qcow2_co_flush_to_disk

[Kevin Wolf <kwolf@redhat.com>]

Am 14.05.2012 16:27, schrieb Zhi Yong Wu:
> On Mon, May 14, 2012 at 10:04 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 14.05.2012 15:51, schrieb zwu.kernel@gmail.com:
>>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>>
>>> qcow2_co_flush_to_os() actually flush all cached data to the disk. To keep its name consistent with its actual function, adjust its name accordingly.
>>>
>>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> This patch is plain wrong.
>>
>> You're aware that you're not changing a name, but functionality here?
> Sure, i know this.
> 
>> Have a look at block_int.h for the semantics of each function:
>>
>>    /*
>>     * Flushes all data that was already written to the OS all the way
>> down to
>>     * the disk (for example raw-posix calls fsync()).
>>     */
>>    int coroutine_fn (*bdrv_co_flush_to_disk)(BlockDriverState *bs);
>>
>>    /*
>>     * Flushes all internal caches to the OS. The data may still sit in a
>>     * writeback cache of the host OS, but it will survive a crash of
>> the qemu
>>     * process.
>>     */
>>    int coroutine_fn (*bdrv_co_flush_to_os)(BlockDriverState *bs);
>>
>> Apart from that, it's not even intentional that qcow2 does a
>> bdrv_flush() even if it didn't write out any cache entries. If we
> Since bdrv_flush() is invoked now, qcow2_co_flush_to_os() is not
> strictly alligned to its expected semantics, but the semantics of 'int
> coroutine_fn (*bdrv_co_flush_to_disk)(BlockDriverState *bs)', so i
> suggested adjusting its name.

Let me put it another way: When bdrv_co_flush_to_os() has been called,
the promise is that the block driver has no more dirty caches inside
qemu. You can SIGKILL qemu and the data would survive. If there is no
bdrv_co_flush_to_os(), this means that the driver doesn't have any data
that must be flushed.

Now you remove the bdrv_co_flush_to_os() callback, so qcow2 may still
have dirty caches even though we promised that there are none. The
result of this can be data loss.

In contrast, bdrv_co_flush_to_disk() doesn't promise to flush dirty
cached to the OS. It merely says that anything that is already flushed
to the OS will be written out to disk. This works without any code in
qcow2, it's only raw-posix.c that has to do something for this function
to work.

>> optimise the cache code a bit, this might disappear in the future.
> You mean that bdrv_flush() will be not invoked here in the future?

Yes, possibly. It's not really required in this case, though it is in
some other code paths. With some refactoring we can probably drop it in
this case.

Kevin