On 07/03/2017 05:14 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> >
> @@ -1810,12 +1817,13 @@ static int64_t coroutine_fn
> bdrv_co_get_block_status(BlockDriverState *bs,
> }
> }
>
> - if (local_file && local_file != bs &&
> + if (!allocation && local_file && local_file != bs &&
> (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
> (ret & BDRV_BLOCK_OFFSET_VALID)) {
> int file_pnum;
>
> - ret2 = bdrv_co_get_block_status(local_file, ret >> BDRV_SECTOR_BITS,
> + ret2 = bdrv_co_get_block_status(local_file, true,
> + ret >> BDRV_SECTOR_BITS,
> *pnum, &file_pnum, NULL);
> if (ret2 >= 0) {
> /* Ignore errors. This is just providing extra information, it
Hmm. My initial thinking here was that if we already have a good primary
status, we want our secondary status (where we are probing bs->file for
whether we can add BDRV_BLOCK_ZERO) to be as fast as possible, so I
hard-coded the answer that favors is_allocated (I have to be careful
describing this, since v3 will switch from 'bool allocated=true' to
'bool mapping=false' to express that same request). But it turns out
that, at least for file-posix.c (for that matter, for several protocol
drivers), it's a LOT faster to just blindly state that the entire file
is allocated and data than it is to lseek(SEEK_HOLE). So favoring
allocation status instead of mapping status defeats the purpose, and
this should be s/true/allocation/ (which is always false at this point)
[or conversely s/false/mapping/, which is always true, in v3].
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
