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? Peter
