On 09/26/2017 01:31 PM, John Snow wrote: > > > On 09/13/2017 12:03 PM, Eric Blake wrote: >> Not all callers care about which BDS owns the mapping for a given >> range of the file. In particular, bdrv_is_allocated() cares more >> about finding the largest run of allocated data from the guest >> perspective, whether or not that data is consecutive from the >> host perspective. Therefore, doing subsequent refinements such >> as checking how much of the format-layer allocation also satisfies >> BDRV_BLOCK_ZERO at the protocol layer is wasted work - in the best >> case, it just costs extra CPU cycles during a single >> bdrv_is_allocated(), but in the worst case, it results in a smaller >> *pnum, and forces callers to iterate through more status probes when >> visiting the entire file for even more extra CPU cycles. >> >> This patch only optimizes the block layer. But subsequent patches >> will tweak the driver callback to be byte-based, and in the process, >> can also pass this hint through to the driver. >> >> Signed-off-by: Eric Blake <ebl...@redhat.com> >>
>> * >> + * If 'mapping' is true, the caller is querying for mapping purposes, >> + * and the result should include BDRV_BLOCK_OFFSET_VALID where >> + * possible; otherwise, the result may omit that bit particularly if >> + * it allows for a larger value in 'pnum'. I decided one more tweak to the comment will help: + * If 'mapping' is true, the caller is querying for mapping purposes, + * and the result should include BDRV_BLOCK_OFFSET_VALID and + * BDRV_BLOCK_ZERO where possible; otherwise, the result may omit those + * bits particularly if it allows for a larger value in 'pnum'. >> @@ -1836,12 +1844,13 @@ static int64_t coroutine_fn >> bdrv_co_get_block_status(BlockDriverState *bs, >> } >> } >> >> - if (local_file && local_file != bs && >> + if (mapping && local_file && local_file != bs && > > Tentatively this looks OK to me, but I have to admit I'm a little shaky > on this portion because I've not really investigated this function too > much. I am at the very least convinced that when mapping is true that > the function is equivalent and that existing callers don't have their > behavior changed too much. > > Benefit of the doubt: > > Reviewed-by: John Snow <js...@redhat.com> Then I'll tentatively keep your R-b even with the comment tweak, unless you say otherwise :) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
