Re: [Qemu-block] [Qemu-devel] [PATCH v4 02/20] block: Switch passthrough drivers to .bdrv_co_block_status()

[Eric Blake]

On 11/28/2017 02:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.10.2017 21:58, Eric Blake wrote:
> > We are gradually moving away from sector-based interfaces, towards
> > byte-based.  Update the generic helpers, and all passthrough clients
> > (blkdebug, commit, mirror, throttle) accordingly.
> >
> > Signed-off-by: Eric Blake <address@hidden>

> > BlockDriverState **file)
> > +int coroutine_fn bdrv_co_block_status_from_backing(BlockDriverState *bs,
> > +                                                   bool want_zero,
> > +                                                   int64_t offset,
> > +                                                   int64_t bytes,
> > +                                                   int64_t *pnum,
> > +                                                   int64_t *map,
> > +                                                   BlockDriverState 
> > **file)
> >   {
> >       assert(bs->backing && bs->backing->bs);
> > -    *pnum = nb_sectors;
> > +    *pnum = bytes;
> > +    *map = offset;
> >       *file = bs->backing->bs;
> > -    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
> > -           (sector_num << BDRV_SECTOR_BITS);
> > +    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;
> >   }
>
> hmm. not related question, but can you please explain, why we use such 
> stubs
> instead of really call bdrv_co_block_status on bs->file->bs or 
> bs->backing->bs ?

Returning BDRV_BLOCK_RAW is what tells io.c to call 
bdrv_co_block_status() on whatever got assigned into *file.  The two 
stubs make it so there are fewer conditionals in io.c; perhaps it could 
be reworked to avoid the stubs and have io.c have all the smarts when 
.bdrv_co_block_status is NULL, but as you say, that would be a separate 
patch (although it was Manos' work earlier this year that even created 
the common helper stubs rather than duplicating code in each callback in 
the first place).


> > +static int coroutine_fn blkdebug_co_block_status(BlockDriverState *bs,

> > +    assert(QEMU_IS_ALIGNED(offset | bytes, bs->bl.request_alignment));
>
> looks like this is obviously guaranteed by block layer, so no needs for 
> special
> function. and we can use
>
> .bdrv_co_block_status       = bdrv_co_block_status_from_backing
>
> like for other drivers.

We could, but we don't want to.  The point of the blkdebug driver is to 
explicitly test that certain preconditions are being satisfied, so that 
even if the block layer in io.c changes, our iotests make sure that 
drivers are provided with certain guarantees.  In other words, the 
assert() added here is added value that we cannot stick in the common 
helper, but something that we want to keep associated with the blkdebug 
driver.

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