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

[Peter Lieven <pl@kamp.de>]

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 <eblake@redhat.com>
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>>   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