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

[Peter Lieven <pl@kamp.de>]

Am 20.07.2013 09:00, schrieb Paolo Bonzini:
> 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?".
Ok, this makes it clearer. Thanks for pointing that out.
the bdrv_get_block_status() will add the possibility to
check for the allocation status which is a good thing.

Peter