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

[Paolo Bonzini <pbonzini@redhat.com>]

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

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

Paolo