Re: [Qemu-devel] [PATCH v10 02/17] qcow2: Correctly report status of preallocated zero clusters

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4169 bytes --]

On 04/28/2017 12:35 PM, Max Reitz wrote:
> On 27.04.2017 03:46, Eric Blake wrote:
>> We were throwing away the preallocation information associated with
>> zero clusters.  But we should be matching the well-defined semantics
>> in bdrv_get_block_status(), where (BDRV_BLOCK_ZERO |
>> BDRV_BLOCK_OFFSET_VALID) informs the user which offset is reserved,
>> while still reminding the user that reading from that offset is
>> likely to read garbage.
>>
>> Making this change lets us see which portions of an image are zero
>> but preallocated, when using qemu-img map --output=json.  The
>> --output=human side intentionally ignores all zero clusters, whether
>> or not they are preallocated.
>>
>> The fact that there is no change to qemu-iotests './check -qcow2'
>> merely means that we aren't yet testing this aspect of qemu-img;
>> a later patch will add a test.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v10: new patch
>> ---
>>  block/qcow2-cluster.c | 32 +++++++++++++++++++++++++++-----
>>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> I'd propose you split the qcow2 changes off of this series.

Yeah, it's sort of morphed into a well-tested later part and an earlier
part that is still undergoing heavy review.  I don't know how much churn
there would be to rebase things to do the blkdebug changes first (it may
invalidate portions of my test 177; but I could always tag such spots as
FIXME while waiting on the qcow2 part to settle and land).  I'll see
what I can do, as I've now got multiple series queued up, and should at
least get SOMETHING to land to start clearing up the logjam.


>> @@ -335,9 +339,10 @@ static int count_contiguous_clusters_by_type(int nb_clusters,
>>      int i;
>>
>>      for (i = 0; i < nb_clusters; i++) {
>> -        int type = qcow2_get_cluster_type(be64_to_cpu(l2_table[i]));
>> +        uint64_t entry = be64_to_cpu(l2_table[i]);
>> +        int type = qcow2_get_cluster_type(entry);
>>
>> -        if (type != wanted_type) {
>> +        if (type != wanted_type || entry & L2E_OFFSET_MASK) {
> 
> This works only for wanted_type \in \{ ZERO, UNALLOCATED \} -- which is
> what the comment you added describes, but this may still warrant a
> renaming of this function (count_contiguous_unallocated_clusters?) and
> probably an assertion that wanted_type is ZERO or UNALLOCATED.

Good idea.


>> @@ -559,9 +564,26 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>>              ret = -EIO;
>>              goto fail;
>>          }
>> -        c = count_contiguous_clusters_by_type(nb_clusters, &l2_table[l2_index],
>> -                                              QCOW2_CLUSTER_ZERO);
>> -        *cluster_offset = 0;
>> +        /* Distinguish between pure zero clusters and pre-allocated ones */
>> +        if (*cluster_offset & L2E_OFFSET_MASK) {
>> +            c = count_contiguous_clusters(nb_clusters, s->cluster_size,
>> +                                          &l2_table[l2_index], QCOW_OFLAG_ZERO);
> 
> You should probably switch this patch and the next one -- or I just send
> my patches myself and you base your series on top of it...

I tested with your patches in, then rebased with your patches out to see
what broke, and...

> 
> Because before the next patch, this happens:
> 

> $ ./qemu-img map foo.qcow2
> Offset          Length          Mapped to       File
> qemu-img: block/qcow2-cluster.c:319: count_contiguous_clusters:
> Assertion `qcow2_get_cluster_type(first_entry) == QCOW2_CLUSTER_NORMAL'
> failed.

got that same assert. So I realized that your patch was necessary and
rebased it back in, but obviously forgot to re-test at all points in the
series. Yes, your should go first, and I'd be more than happy if you
post yours formally and I rebase my qcow2 portions on top of yours (all
the more reason for me to split this series into two, and get the
blkdebug portions in now while we still hammer on the qcow2 stuff).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]