Re: [Qemu-devel] [PATCH v4 11/20] qcow2: Switch to .bdrv_co_block_status()

[Eric Blake <eblake@redhat.com>]

On 11/30/2017 03:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.10.2017 21:59, Eric Blake wrote:
>> We are gradually moving away from sector-based interfaces, towards
>> byte-based.  Update the qcow2 driver accordingly.
>>
>> For now, we are ignoring the 'want_zero' hint.  However, it should
>> be relatively straightforward to honor the hint as a way to return
>> larger *pnum values when we have consecutive clusters with the same
>> data/zero status but which differ only in having non-consecutive
>> mappings.
> 
> useful thing (for example, to get several compressed clusters in one chunk,
> but do not include following zero clusters). but should not the hint be 
> called want_mapping for it?
> or may be we will move to "int hint_flags", which will include status flags
> we a interested in?

Right now, there are only two public interfaces: bdrv_is_allocated 
(want_zero is false), and bdrv_block_status (want_zero is true).  You 
may have a point that there could be further optimizations possible if 
the caller has more control over what flags it is interested in, but 
that should be a patch series for a later day; I'm already cramming 
enough into this series, and it's already stretched out (first patches 
went into 2.10), so I want it done.

> 
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 

>> -static int64_t coroutine_fn 
>> qcow2_co_get_block_status(BlockDriverState *bs,
>> -        int64_t sector_num, int nb_sectors, int *pnum, 
>> BlockDriverState **file)
>> +static int coroutine_fn qcow2_co_block_status(BlockDriverState *bs,
>> +                                              bool want_zero,
>> +                                              int64_t offset, int64_t 
>> count,
> 
> 'count' is used instead of 'bytes' because of qcow2_get_cluster_offset 
> which wants int*..

Yes,...

> 
>> +                                              int64_t *pnum, int64_t 
>> *map,
>> +                                              BlockDriverState **file)
>>   {
>>       BDRVQcow2State *s = bs->opaque;
>>       uint64_t cluster_offset;
>>       int index_in_cluster, ret;
>>       unsigned int bytes;

...because of that.

>> -    int64_t status = 0;
>> +    int status = 0;
>>
>> -    bytes = MIN(INT_MAX, nb_sectors * BDRV_SECTOR_SIZE);
>> +    bytes = MIN(INT_MAX, count);
>>       qemu_co_mutex_lock(&s->lock);
>> -    ret = qcow2_get_cluster_offset(bs, sector_num << 
>> BDRV_SECTOR_BITS, &bytes,
>> -                                   &cluster_offset);
>> +    ret = qcow2_get_cluster_offset(bs, offset, &bytes, &cluster_offset);

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org