[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 02/23] block: Add flag to avoid wasted work i
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v4 02/23] block: Add flag to avoid wasted work in bdrv_is_allocated() |
Date: |
Thu, 28 Sep 2017 09:58:37 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
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 <address@hidden>
>>
>> *
>> + * 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 <address@hidden>
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