Re: [Qemu-devel] callout to *file in bdrv_co_get_block_status

[Peter Lieven]

Am 20.03.2017 um 14:23 schrieb Paolo Bonzini:
>
> On 20/03/2017 14:13, Peter Lieven wrote:
>> Am 20.03.2017 um 13:47 schrieb Peter Lieven:
>>> Am 20.03.2017 um 12:49 schrieb Fam Zheng:
>>>> On Mon, 03/20 12:21, Paolo Bonzini wrote:
>>>>> On 20/03/2017 03:46, Fam Zheng wrote:
>>>>>> On Fri, 03/17 12:20, Peter Lieven wrote:
>>>>>>> Am 17.03.2017 um 12:16 schrieb Paolo Bonzini:
>>>>>>>> On 17/03/2017 12:11, Peter Lieven wrote:
>>>>>>>>>>> like VMDK or QCOW2 shouldn't we trust the information from the l2 
>>>>>>>>>>> tables in the VMDK or QCOW2?
>>>>>>>>>> It provides additional information, for example it works better with
>>>>>>>>>> prealloc=metadata.
>>>>>>>>> Okay, understood. Can you imagine of a away to conditionally avoid 
>>>>>>>>> this second callout? In my case we have an additional
>>>>>>>>> lseek for each cluster. For a 20GB file this are approx. 327k calls 
>>>>>>>>> to lseek. And if the file has no preallocated metadata
>>>>>>>>> it will likely not improve anything. And even if the metadata is 
>>>>>>>>> prealloced what is the allocation status of the clusters?
>>>>>>>> If the metadata is preallocated, cluster will (or should) show up as
>>>>>>>> zero, speeding up the copy.
>>>>>>> Okay, in this case the second call out to *file will not happen. It 
>>>>>>> only happens if the metadata says it contains data.
>>>>>>> So where does it actually help?
>>>>>>>
>>>>>>> The condition is: (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) 
>>>>>>> && (ret & BDRV_BLOCK_OFFSET_VALID))
>>>>>>>
>>>>>>> So from my view it can only have any effect if the metadata returns 
>>>>>>> BDRV_BLOCK_DATA, but the protocol driver returns
>>>>>>> BDRV_BLOCK_ZERO.
>>>>>>>
>>>>>>> This can only happen if I partially write to a cluster, or am I wrong 
>>>>>>> here?
>>>>>> I think you have a point. The metadata should have said BDRV_BLOCK_ZERO 
>>>>>> if
>>>>>> protocol would say BDRV_BLOCK_ZERO - there is no reason the format 
>>>>>> driver cannot
>>>>>> know.
>>>>> That's true of qcow2, but many formats (including raw :)) don't know
>>>>> about BDRV_BLOCK_ZERO.
>>>> Raw is a little special, it could have forwarded the call to *file in its
>>>> BlockDriver callback. Most formats with metadata stores zero/nonzero 
>>>> information
>>>> in L1/L2 tables. For qcow2 and VMDK I think it's okay to just trust meta 
>>>> data on
>>>> zero/nonzero.
>>>>
>>>> Fam
>>> BTW, the extra check was added in
>>>
>>>
>>> commit 5daa74a6ebce7543aaad178c4061dc087bb4c705
>>> Author: Paolo Bonzini <address@hidden>
>>> Date:   Wed Sep 4 19:00:38 2013 +0200
>>>
>>>     block: look for zero blocks in bs->file
>>>    
>>>     Reviewed-by: Eric Blake <address@hidden>
>>>     Signed-off-by: Paolo Bonzini <address@hidden>
>>>     Signed-off-by: Stefan Hajnoczi <address@hidden>
>>>
>>>
>>> It was introduced while introducing bdv_get_block_status. I don't know what 
>>> the real
>>>
>>> issue was that was addressed with this patch?
>> Is it possible that this optimization was added especially for RAW? I was 
>> believing that
>> raw would forward the get_block_status call to bs->file, but it looks it 
>> doesn't.
>> If this one here was for RAW would it be an option to move this callout to 
>> the raw-format driver
>> and remove it from the generic code?
> It was meant for both raw and qcow2.

Okay, but as Fam mentioned qcow2 Metadata should know that a cluster is zero. 
Do you remember
what the issue was?

Peter