Re: [Qemu-devel] [PATCHv4 14/17] block/get_block_status: fix BDRV_BLOCK_ZERO for unallocated blocks

[Peter Lieven]

On 18.10.2013 15:24, Stefan Hajnoczi wrote:
> On Fri, Oct 18, 2013 at 02:49:11PM +0200, Paolo Bonzini wrote:
> > Il 18/10/2013 14:38, Stefan Hajnoczi ha scritto:
> > > On Tue, Oct 08, 2013 at 01:58:08PM +0200, Peter Lieven wrote:
> > > > this patch does 2 things:
> > > > a) only do additional call outs if BDRV_BLOCK_ZERO is not already set.
> > > > b) use the newly introduced bdrv_has_discard_zeroes() to return the
> > > >     zero state of an unallocated block. the used callout to
> > > >     bdrv_has_zero_init() is only valid right after bdrv_create.
> > > >
> > > > Reviewed-by: Eric Blake <address@hidden>
> > > > Signed-off-by: Peter Lieven <address@hidden>
> > > > ---
> > > >   block.c |    4 ++--
> > > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/block.c b/block.c
> > > > index fc931e3..1be4418 100644
> > > > --- a/block.c
> > > > +++ b/block.c
> > > > @@ -3247,8 +3247,8 @@ static int64_t coroutine_fn 
> > > > bdrv_co_get_block_status(BlockDriverState *bs,
> > > >           return ret;
> > > >       }
> > > >   
> > > > -    if (!(ret & BDRV_BLOCK_DATA)) {
> > > > -        if (bdrv_has_zero_init(bs)) {
> > > > +    if (!(ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO)) {
> > > > +        if (bdrv_has_discard_zeroes(bs)) {
> > > I'm a little unclear about the semantics of bdrv_has_discard_zeroes().
> > > Originally I thought it just meant any blocks discarded will read back
> > > as zeroes.  But here it implies that any unallocated block reads
> > > back as zeroes too?
> > >
> > > In other words, this patch assumes unallocated blocks behave the same as
> > > discarded blocks wrt to zeroes.
> > Note that earlier patches introduce both bdrv_has_discard_zeroes and
> > bdrv_has_discard_write_zeroes.  There is no documentation, but the iscsi
> > implementation let us understand the meaning:
> There are doc comments but they differ from what you've posted:
>
> +    /*
> +     * Returns true if discarded blocks read back as zeroes.
> +     */
> +    bool (*bdrv_has_discard_zeroes)(BlockDriverState *bs);
>
> > +static bool iscsi_has_discard_zeroes(BlockDriverState *bs)
> > +{
> > +    IscsiLun *iscsilun = bs->opaque;
> > +    return !!iscsilun->lbprz;
> > +}
> >
> > That is, unallocated block reads back as zeroes
> Okay, your semantics make sense.  With them the later patches are correct.
>
> Peter: Please update the doc comments although and consider Paolo's comments
> about block driver info.
Ok, we have 2 features, but we need better names for them.

a) unallocated blocks read back as zeroes. I would suggest to rename
bdrv_has_discard_zeroes() to bdrv_unallocated_blocks_return_zero() and
update the comment to:

 /*
  * Returns true if unallocated blocks read back as zeroes.
  */

b) the driver has an efficient way of speeding up bdrv_write_zeroes by unmapping
blocks. for iscsi this is done through WRITESAME16 with the UNMAP flag. for 
other
drivers this could be a similar approach as long as it is guaranteed that its 
not
writing actual zeroes to disk and that zeroes are returned in any case.
I would suggest to rename bdrv_has_discard_write_zeroes to 
bdrv_can_write_zeroes_by_unmap().

    /*
     * Returns true if the driver can optimize writing zeroes by unmapping
     * without actually writing real zeroes to disk. However, it must be 
guaranteed
     * that all blocks read back as zeroes afterwards. It is additionally 
required that
     * the block device is opened with BDRV_O_UNMAP and the that the
     * bdrv_write_zeroes request carries the BDRV_REQ_MAY_UNMAP flag for
     * this to work.
     */

Regarding putting this info into the BDI I am fine with that, but I would keep 
the wrapper functions.
On the other hand, bdrv_has_zero_init is also not in the BDI... I had it in the 
BDI and got the request
to move it to separate functions. To finish this series I need a definitive 
decision where to put it.

Peter