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

[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.

Paolo