Re: [Qemu-devel] [patch 5/5][v2] Try to aggregate free clusters and freed clusters

[Kevin Wolf <kwolf@suse.de>]

Laurent Vivier schrieb:
> Le lundi 11 août 2008 à 15:13 +0200, Kevin Wolf a écrit :
>> Hi Laurent,
>>
>> Laurent Vivier schrieb:
>>> In free_used_clusters(), try to aggregate free clusters and freed clusters.
>>>
>>> Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>
>>> ---
>>>  block-qcow2.c |   61 ++++++++++++++++++++++++++++++++++------------------------
>>>  1 file changed, 36 insertions(+), 25 deletions(-)
>> I know that this patch will be rewritten as well, but I'll comment on it
>> nevertheless. Maybe the comments apply to your new version, too.
>>
>>
>>> Index: qemu/block-qcow2.c
>>> ===================================================================
>>> --- qemu.orig/block-qcow2.c	2008-07-29 15:22:28.000000000 +0200
>>> +++ qemu/block-qcow2.c	2008-07-29 15:22:30.000000000 +0200
>>> @@ -714,22 +703,44 @@ static uint64_t free_used_clusters(Block
>>>          return 0;
>>>      }
>>>  
>>> -    if (nb_clusters) {
>>> -        int i = 1;
>>> -        uint64_t current;
>>> -        while (i < *nb_clusters) {
>>> -            current = be64_to_cpu((*l2_table)[(*l2_index) + i]);
>>> -            if (cluster_offset + (i << s->cluster_bits) != current)
>>> -                break;
>>> -            i++;
>>> -        }
>>> -        *nb_clusters = i;
>>> -        free_clusters(bs, cluster_offset, i << s->cluster_bits);
>>> +    if (!nb_clusters) {
>>> +        if (cluster_offset)
>>> +            free_clusters(bs, cluster_offset, s->cluster_size);
>>>          return 0;
>>>      }
>>>  
>>> -    free_clusters(bs, cluster_offset, s->cluster_size);
>>> +    i = 0;
>>> +    do_loop = 1;
>>> +    while (i < *nb_clusters && do_loop) {
>>> +        i++;
>>> +        if (!cluster_offset) {
>>> +            while (i < *nb_clusters) {
>>> +                cluster_offset = (*l2_table)[(*l2_index) + i];
>>> +                if (cluster_offset)
>>> +                    break;
>>> +                i++;
>>> +            }
>>> +            if ((cluster_offset & QCOW_OFLAG_COPIED) ||
>>> +                (cluster_offset & QCOW_OFLAG_COMPRESSED))
>>> +                do_loop = 0;
>>> +        } else {
>>> +            j = 1;
>>> +            current = 0;
>>> +            while (i < *nb_clusters) {
>>> +                current = be64_to_cpu((*l2_table)[(*l2_index) + i]);
>>> +                if (cluster_offset + (j << s->cluster_bits) != current)
>>> +                    break;
>>> +                i++;
>>> +                j++;
>>> +            }
>>> +            free_clusters(bs, cluster_offset, j << s->cluster_bits);
>>> +            cluster_offset = current;
>>> +            if (current)
>>> +                do_loop = 0;
>> The else block changes its behaviour with this patch: I think you should
>>  take QCOW_OFLAG_COPIED into consideration. In this implementation, when
>> a cluster with QCOW_OFLAG_COPIED is found, the inner loop aborts, but
>> the outer one will just call the else block once again. So you're also
>> accumulating clusters with QCOW_OFLAG_COPIED set.
> 
> No,
> before the first loop QCOW_OFLAG_COPIED is not set.
> if cluster_offset is 0 and flag appears, we set do_loop to 0 and exit
> from the main loop.
> if cluster_offset is != 0 we go into the else until current becomes 0 or
> a flag is set and then if current is not 0 (i.e. a flag or a non
> contiguous cluster) we set do_loop to 0 and exit from the loop.

You're right. I really should learn reading. Sorry for the noise.

Kevin