From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KSXE0-00068P-Kx for qemu-devel@nongnu.org; Mon, 11 Aug 2008 09:14:16 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KSXDy-000672-Qe for qemu-devel@nongnu.org; Mon, 11 Aug 2008 09:14:16 -0400 Received: from [199.232.76.173] (port=49066 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KSXDy-00066s-N8 for qemu-devel@nongnu.org; Mon, 11 Aug 2008 09:14:14 -0400 Received: from cantor.suse.de ([195.135.220.2]:50300 helo=mx1.suse.de) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1KSXDx-0006ca-Uo for qemu-devel@nongnu.org; Mon, 11 Aug 2008 09:14:14 -0400 Message-ID: <48A03AE7.7050908@suse.de> Date: Mon, 11 Aug 2008 15:13:11 +0200 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [patch 5/5][v2] Try to aggregate free clusters and freed clusters References: <20080729141352.573798859@bull.net> <20080729141447.763322831@bull.net> In-Reply-To: <20080729141447.763322831@bull.net> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier Cc: qemu-devel@nongnu.org Hi Laurent, Laurent Vivier schrieb: > In free_used_clusters(), try to aggregate free clusters and freed clusters. > > Signed-off-by: Laurent Vivier > --- > 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