On 14.06.19 16:01, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> If the driver does not support .bdrv_co_flush() so bdrv_co_flush()
>> itself has to flush the children of the given node, it should not flush
>> just bs->file->bs, but in fact both the child that stores data, and the
>> one that stores metadata (if they are separate).
>>
>> In any case, the BLKDBG_EVENT() should be emitted on the primary child,
>> because that is where a blkdebug node would be if there is any.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> block/io.c | 21 ++++++++++++++++++---
>> 1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index 53aabf86b5..64408cf19a 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -2533,6 +2533,8 @@ static void coroutine_fn bdrv_flush_co_entry(void *opaque)
>>
>> int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>> {
>> + BdrvChild *primary_child = bdrv_primary_child(bs);
>> + BlockDriverState *storage_bs, *metadata_bs;
>> int current_gen;
>> int ret = 0;
>>
>> @@ -2562,7 +2564,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>> }
>>
>> /* Write back cached data to the OS even with cache=unsafe */
>> - BLKDBG_EVENT(bs->file, BLKDBG_FLUSH_TO_OS);
>> + BLKDBG_EVENT(primary_child, BLKDBG_FLUSH_TO_OS);
>> if (bs->drv->bdrv_co_flush_to_os) {
>> ret = bs->drv->bdrv_co_flush_to_os(bs);
>> if (ret < 0) {
>> @@ -2580,7 +2582,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>> goto flush_parent;
>> }
>>
>> - BLKDBG_EVENT(bs->file, BLKDBG_FLUSH_TO_DISK);
>> + BLKDBG_EVENT(primary_child, BLKDBG_FLUSH_TO_DISK);
>> if (!bs->drv) {
>> /* bs->drv->bdrv_co_flush() might have ejected the BDS
>> * (even in case of apparent success) */
>> @@ -2625,7 +2627,20 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>> * in the case of cache=unsafe, so there are no useless flushes.
>> */
>> flush_parent:
>> - ret = bs->file ? bdrv_co_flush(bs->file->bs) : 0;
>> + storage_bs = bdrv_storage_bs(bs);
>> + metadata_bs = bdrv_metadata_bs(bs);
>> +
>> + ret = 0;
>> + if (storage_bs) {
>> + ret = bdrv_co_flush(storage_bs);
>> + }
>> + if (metadata_bs && metadata_bs != storage_bs) {
>> + int ret_metadata = bdrv_co_flush(metadata_bs);
>> + if (!ret) {
>> + ret = ret_metadata;
>> + }
>> + }
>> +
>> out:
>> /* Notify any pending flushes that we have completed */
>> if (ret == 0) {
>>
>
> Hmm, I'm not sure that if in one driver we decided to store data and metadata separately,
> we need to support flushing them both generic code.. If at some point qcow2 decides store part
> of metadata in third child, we will not flush it here too?
>
> Should not we instead loop through children and flush all? And I'd s/flush_parent/flush_children as
> it is rather weird.
That sounds good. Well, we only need to flush the ones the driver has
taken a WRITE permission on, but yes.
Max