Re: [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters

[Max Reitz <mreitz@redhat.com>]

Am 29.08.2013 14:33, schrieb Kevin Wolf:
> Am 29.08.2013 um 12:16 hat Max Reitz geschrieben:
>> Do not try to update the refcount for zero clusters in
>> qcow2_update_snapshot_refcount.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/qcow2-refcount.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
> Please don't forget to add a test case for v2.
Okay.

>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 1244693..7555242 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -863,9 +863,13 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>>               for(j = 0; j < s->l2_size; j++) {
>>                   offset = be64_to_cpu(l2_table[j]);
>>                   if (offset != 0) {
>> +                    uint64_t cluster_index;
>> +
>>                       old_offset = offset;
>>                       offset &= ~QCOW_OFLAG_COPIED;
>> -                    if (offset & QCOW_OFLAG_COMPRESSED) {
>> +
>> +                    switch (qcow2_get_cluster_type(offset)) {
>> +                    case QCOW2_CLUSTER_COMPRESSED:
>>                           nb_csectors = ((offset >> s->csize_shift) &
>>                                          s->csize_mask) + 1;
>>                           if (addend != 0) {
>> @@ -880,8 +884,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>>                           }
>>                           /* compressed clusters are never modified */
>>                           refcount = 2;
>> -                    } else {
>> -                        uint64_t cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits;
>> +                        break;
>> +
>> +                    case QCOW2_CLUSTER_NORMAL:
>> +                        cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits;
>>                           if (addend != 0) {
>>                               refcount = update_cluster_refcount(bs, cluster_index, addend,
>>                                                                  QCOW2_DISCARD_SNAPSHOT);
>> @@ -893,6 +899,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>>                               ret = refcount;
>>                               goto fail;
>>                           }
>> +                        break;
> I think this part isn't quite right. Even zero clusters can have an
> allocated offset, so I think what's really needed is that the same code
> runs from QCOW2_CLUSTER_ZERO. However, add an if (cluster_index == 0)
> case for the unallocated case.
So then it would be enough to change:

- if (offset != 0) {
+ if ((offset & L2E_OFFSET_MASK) != 0) {

and just execute the same code for QCOW2_CLUSTER_NORMAL and 
QCOW2_CLUSTER_ZERO?

>> +
>> +                    default:
>> +                        refcount = 0;
> Other places enumerate the enum values explicitly and have a default
> label that simply abort()s. This will help catching forgotten places
> when a new value is added.
Okay.

>>                       }
>>   
>>                       if (refcount == 1) {
> Kevin
Max