diff mbox

qcow2: adjust qcow2_co_flush_to_os -> qcow2_co_flush_to_disk

Message ID 1337003481-30215-1-git-send-email-zwu.kernel@gmail.com
State New
Headers show

Commit Message

Zhiyong Wu May 14, 2012, 1:51 p.m. UTC 
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>
---
 block/qcow2.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Comments

Kevin Wolf May 14, 2012, 2:04 p.m. UTC | #1 
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?
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
optimise the cache code a bit, this might disappear in the future.

Kevin
Zhiyong Wu May 14, 2012, 2:27 p.m. UTC | #2 
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.

> 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?
>
> Kevin
Kevin Wolf May 14, 2012, 2:39 p.m. UTC | #3 
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
Zhiyong Wu May 14, 2012, 2:54 p.m. UTC | #4 
On Mon, May 14, 2012 at 10:39 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> 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.
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
diff mbox

Patch

diff --git a/block/qcow2.c b/block/qcow2.c
index ee4678f..a04c304 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1435,7 +1435,7 @@  fail:
     return ret;
 }
 
-static coroutine_fn int qcow2_co_flush_to_os(BlockDriverState *bs)
+static coroutine_fn int qcow2_co_flush_to_disk(BlockDriverState *bs)
 {
     BDRVQcowState *s = bs->opaque;
     int ret;
@@ -1580,7 +1580,7 @@  static BlockDriver bdrv_qcow2 = {
 
     .bdrv_co_readv          = qcow2_co_readv,
     .bdrv_co_writev         = qcow2_co_writev,
-    .bdrv_co_flush_to_os    = qcow2_co_flush_to_os,
+    .bdrv_co_flush_to_disk    = qcow2_co_flush_to_disk,
 
     .bdrv_co_write_zeroes   = qcow2_co_write_zeroes,
     .bdrv_co_discard        = qcow2_co_discard,