Re: [Qemu-devel] [PATCH v2 10/17] block: define get_block_status return value

[Paolo Bonzini <pbonzini@redhat.com>]

Il 19/07/2013 15:06, Peter Lieven ha scritto:
>>>> Il 19/07/2013 08:48, Peter Lieven ha scritto:
>>>>>> -    return bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
>>>>>> +    int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors,
>>>>>> pnum);
>>>>>> +    return
>>>>>> +        (ret & BDRV_BLOCK_DATA) ||
>>>>>> +        ((ret & BDRV_BLOCK_ZERO) && !bdrv_has_zero_init(bs));
>>>>> i do also not understand the "((ret & BDRV_BLOCK_ZERO) &&
>>>>> !bdrv_has_zero_init(bs))";
>>>>> if a block is unallocated and reads as zero, but the device lacks zero
>>>>> init, it is declared as allocated with this, isn't it?
> 
> If it is zero and allocated the API should return only BDRV_BLOCK_DATA
> and if it is zero and unallocated only BDRV_BLOCK_ZERO or not?
> 
> What I mean is the new API shouldn't change the behaviour of the old
> bdrv_is_allocated().
> It would have returned
> 
> (ret & BDRV_BLOCK_DATA) regardless if BDRV_BLOCK_ZERO or not.

bdrv_is_allocated must return true for some zero clusters, even
if BDRV_BLOCK_DATA = 0.  See

commit 381b487d54ba18c73df9db8452028a330058c505
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Wed Mar 6 18:02:01 2013 +0100

    qcow2: make is_allocated return true for zero clusters
    
    Otherwise, live migration of the top layer will miss zero clusters and
    let the backing file show through.  This also matches what is done in qed.
    
    QCOW2_CLUSTER_ZERO clusters are invalid in v2 image files.  Check this
    directly in qcow2_get_cluster_offset instead of replicating the test
    everywhere.
    
    Cc: qemu-stable@nongnu.org
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
    Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

I think the source of the confusion is that SCSI "GET LBA STATUS"
does not have to deal with backing files, bdrv_is_allocated must.
If bs->backing_hd != NULL, bdrv_is_allocated is not about allocation
of blocks in the SCSI sense; it's a query for "does the block come
from this BDS or rather from its backing file?".

So this patch must take those slightly different semantics into
account.

Paolo