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

[Kevin Wolf <kwolf@suse.de>]

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.

BTW, why do you use that do_loop variable instead of a break statement?

Kevin