[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [patch 5/5][v2] Try to aggregate free clusters and free
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [patch 5/5][v2] Try to aggregate free clusters and freed clusters |
Date: |
Mon, 11 Aug 2008 17:03:34 +0200 |
User-agent: |
Thunderbird 2.0.0.12 (X11/20071114) |
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 <address@hidden>
>>> ---
>>> 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