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

[Zhi Yong Wu]

On Mon, May 14, 2012 at 10:39 PM, Kevin Wolf <address@hidden> wrote:
> Am 14.05.2012 16:27, schrieb Zhi Yong Wu:
>> On Mon, May 14, 2012 at 10:04 PM, Kevin Wolf <address@hidden> wrote:
>>> Am 14.05.2012 15:51, schrieb address@hidden:
>>>> From: Zhi Yong Wu <address@hidden>
>>>>
>>>> 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 <address@hidden>
>>>
>>> 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.
OK, it is the intention.
>
> 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.
Yeah, maybe.
>
> 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.
Got it. thanks
>
>>> 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.
OK.

Anyway, thanks for your explain, please ignore this patch.
>
> Kevin



-- 
Regards,

Zhi Yong Wu