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

[Max Reitz <mreitz@redhat.com>]

Am 29.08.2013 15:04, schrieb Kevin Wolf:
> Am 29.08.2013 um 14:56 hat Max Reitz geschrieben:
>> Am 29.08.2013 14:53, schrieb Kevin Wolf:
>>> Am 29.08.2013 um 14:38 hat Max Reitz geschrieben:
>>>> 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?
>>> Doing only this change in the original code might happen to work, but
>>> it's not really clean as the L2 entry format is different for compressed
>>> clusters (L2E_OFFSET_MASK isn't valid for it). So having the switch for
>>> the cluster type is a good thing, but the implementation for normal and
>>> zero clusters should be shared indeed.
>> Yes, of course I meant to include the switch as well.
>>
>>> The offset != 0 check ends up redundant when you introduce the switch,
>>> so in theory it could as well be removed.
>>>
>> Well, I don't know; for zero clusters, the if statement is required
>> anyway (since qcow2_get_cluster_type does not distinguish between
>> allocated and non-allocated zero clusters), so I though we might as
>> well just leave it where it is (just slightly adjusted in the way I
>> proposed).
> You do need a zero check, but it's only for normal/zero clusters, not
> for compressed ones; and it must check offset & L2E_OFFSET_MASK (or
> cluster_index) rather than offset itself.
>
> So I think you end up removing the outer if (offset != 0) and adding a
> new inner check in case QCOW2_CLUSTER_NORMAL/ZERO.
>
> Kevin
Ah, yes, you're right.

Max