[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v2 03/15] block: Add flag to avoid
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v2 03/15] block: Add flag to avoid wasted work in bdrv_is_allocated() |
Date: |
Wed, 5 Jul 2017 21:35:36 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 |
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 <address@hidden>
>
> @@ -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
Re: [Qemu-block] [Qemu-devel] [PATCH v2 03/15] block: Add flag to avoid wasted work in bdrv_is_allocated(),
Eric Blake <=
[Qemu-block] [PATCH v2 02/15] block: Allow NULL file for bdrv_get_block_status(), Eric Blake, 2017/07/03
[Qemu-block] [PATCH v2 04/15] block: Make bdrv_round_to_clusters() signature more useful, Eric Blake, 2017/07/03
[Qemu-block] [PATCH v2 05/15] qcow2: Switch is_zero_sectors() to byte-based, Eric Blake, 2017/07/03
[Qemu-block] [PATCH v2 06/15] block: Switch bdrv_make_zero() to byte-based, Eric Blake, 2017/07/03
[Qemu-block] [PATCH v2 07/15] qemu-img: Switch get_block_status() to byte-based, Eric Blake, 2017/07/03