On Mon, May 14, 2012 at 10:39 PM, Kevin Wolf <kw...@redhat.com> wrote: > Am 14.05.2012 16:27, schrieb Zhi Yong Wu: >> On Mon, May 14, 2012 at 10:04 PM, Kevin Wolf <kw...@redhat.com> wrote: >>> Am 14.05.2012 15:51, schrieb zwu.ker...@gmail.com: >>>> From: Zhi Yong Wu <wu...@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 <wu...@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 -- Regards, Zhi Yong Wu
