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

[Eric Blake]

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 <address@hidden>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

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