Re: [Qemu-block] [Qemu-devel] [PATCH v4 02/23] block: Add flag to avoid wasted work in bdrv_is_allocated()

[Eric Blake]

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

>>   *
>> + * 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 

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

Re: [Qemu-block] [Qemu-devel] [PATCH v4 02/23] block: Add flag to avoid wasted work in bdrv_is_allocated()

[John Snow]

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 
> 
> ---
> v4: only context changes
> v3: s/allocation/mapping/ and flip sense of bool
> v2: new patch
> ---
>  block/io.c | 52 ++--
>  1 file changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index f250029395..6509c804d4 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1709,6 +1709,7 @@ typedef struct BdrvCoGetBlockStatusData {
>  int nb_sectors;
>  int *pnum;
>  int64_t ret;
> +bool mapping;
>  bool done;
>  } BdrvCoGetBlockStatusData;
> 
> @@ -1743,6 +1744,11 @@ int64_t coroutine_fn 
> bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
>   * Drivers not implementing the functionality are assumed to not support
>   * backing files, hence all their sectors are reported as allocated.
>   *
> + * 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'.
> + *
>   * If 'sector_num' is beyond the end of the disk image the return value is
>   * BDRV_BLOCK_EOF and 'pnum' is set to 0.
>   *
> @@ -1759,6 +1765,7 @@ int64_t coroutine_fn 
> bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
>   * is allocated in.
>   */
>  static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
> + bool mapping,
>   int64_t sector_num,
>   int nb_sectors, int 
> *pnum,
>   BlockDriverState **file)
> @@ -1817,14 +1824,15 @@ static int64_t coroutine_fn 
> bdrv_co_get_block_status(BlockDriverState *bs,
> 
>  if (ret & BDRV_BLOCK_RAW) {
>  assert(ret & BDRV_BLOCK_OFFSET_VALID && local_file);
> -ret = bdrv_co_get_block_status(local_file, ret >> BDRV_SECTOR_BITS,
> +ret = bdrv_co_get_block_status(local_file, mapping,
> +   ret >> BDRV_SECTOR_BITS,
> *pnum, pnum, _file);
>  goto out;
>  }
> 
>  if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
>  ret |= BDRV_BLOCK_ALLOCATED;
> -} else {
> +} else if (mapping) {
>  if (bdrv_unallocated_blocks_are_zero(bs)) {
>  ret |= BDRV_BLOCK_ZERO;
>  } else if (bs->backing) {
> @@ -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 

>  (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, mapping,
> +ret >> BDRV_SECTOR_BITS,
>  *pnum, _pnum, NULL);
>  if (ret2 >= 0) {
>  /* Ignore errors.  This is just providing extra information, it
> @@ -1876,6 +1885,7 @@ out:
> 
>  static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState 
> *bs,
>  BlockDriverState *base,
> +bool mapping,
>  int64_t sector_num,
>  int nb_sectors,
>  int *pnum,
> @@ -1887,7 +1897,8 @@ static int64_t coroutine_fn 
>