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 <pbonz...@redhat.com> >>> Date: Wed Sep 4 19:00:38 2013 +0200 >>> >>> block: look for zero blocks in bs->file >>> >>> Reviewed-by: Eric Blake <ebl...@redhat.com> >>> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com> >>> Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com> >>> >>> >>> 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
