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 14:49, 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:
>
>
> +static bool iscsi_has_discard_zeroes(BlockDriverState *bs)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +    return !!iscsilun->lbprz;
> +}
>
> That is, unallocated block reads back as zeroes
>
> +static bool iscsi_has_discard_write_zeroes(BlockDriverState *bs)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +    return iscsilun->lbprz && iscsilun->lbp.lbpws;
> +}
>
> That is, discarded blocks read back zeroes.  This is because:
>
> - UNMAP is not guaranteeing that blocks are discarded, and thus not
> really guaranteeing anything on its contents.
>
> - but WRITE SAME is guaranteeing that blocks you "write same" read with
> the payload of the command.  This means that in practice for !LBPRZ the
> WRITE SAME command will not discard (unless the firmware has bugs).
>
> - so for !LBPRZ you must use UNMAP, but for LBPRZ you can use WRITE SAME
> and guarantee that the block reads as zero
>
>
> Perhaps better names could be
>
> - bdrv_discard_zeroes for bdrv_has_discard_write_zeroes
This would conform to the linux ioctl BLKDISCARDZEROES.
However, we need the write_zeroes operation for a guarantee
that zeroes are return.

>
> - bdrv_unallocated_blocks_are_zero for bdrv_has_discard_zeroes
>
> But I'm not sure why we have different BlockDriver APIs.  I'd rather put
> the new flags in BlockDriverInfo, and make the new functions simple
> wrappers around bdrv_get_info.  I think I proposed that before, maybe I
> wasn't clear or I was misunderstood.
I think Kevin wanted to have special functions for this.

Peter