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

[Laurent Vivier <Laurent.Vivier@bull.net>]

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.

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

Yes

Laurent
-- 
----------------- Laurent.Vivier@bull.net  ------------------
  "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry