[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: |
Laurent Vivier |
Subject: |
Re: [Qemu-devel] [patch 5/5][v2] Try to aggregate free clusters and freed clusters |
Date: |
Mon, 11 Aug 2008 16:04:01 +0200 |
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.
> BTW, why do you use that do_loop variable instead of a break statement?
Yes
Laurent
--
----------------- address@hidden ------------------
"La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry