Re: [Qemu-devel] [PATCH v5 16/42] block: Use child access functions when flushing

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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.

-- 
Best regards,
Vladimir