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

[Max Reitz]

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 <address@hidden>
>> ---
>>   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

signature.asc
Description: OpenPGP digital signature