From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52542) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJ2XX-0002Uu-St for qemu-devel@nongnu.org; Wed, 04 Feb 2015 11:10:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJ2XS-00062F-TZ for qemu-devel@nongnu.org; Wed, 04 Feb 2015 11:10:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60705) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJ2XS-00061z-Gc for qemu-devel@nongnu.org; Wed, 04 Feb 2015 11:10:50 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t14GAo0N004298 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 4 Feb 2015 11:10:50 -0500 Date: Wed, 4 Feb 2015 17:10:47 +0100 From: Kevin Wolf Message-ID: <20150204161047.GG5641@noname.redhat.com> References: <1418647857-3589-1-git-send-email-mreitz@redhat.com> <1418647857-3589-9-git-send-email-mreitz@redhat.com> <20150204115543.GB5641@noname.redhat.com> <54D23BB7.1040303@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <54D23BB7.1040303@redhat.com> Subject: Re: [Qemu-devel] [PATCH v5 08/26] qcow2: Refcount overflow and qcow2_alloc_bytes() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-devel@nongnu.org, Stefan Hajnoczi Am 04.02.2015 um 16:33 hat Max Reitz geschrieben: > On 2015-02-04 at 06:55, Kevin Wolf wrote: > >Am 15.12.2014 um 13:50 hat Max Reitz geschrieben: > >>qcow2_alloc_bytes() may reuse a cluster multiple times, in which case > >>the refcount is increased accordingly. However, if this would lead to an > >>overflow the function should instead just not reuse this cluster and > >>allocate a new one. > >> > >>Signed-off-by: Max Reitz > >>Reviewed-by: Eric Blake > >>Reviewed-by: Stefan Hajnoczi > >>--- > >> block/qcow2-refcount.c | 31 ++++++++++++++++++++++++++++++- > >> 1 file changed, 30 insertions(+), 1 deletion(-) > >> > >>diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > >>index db81647..fd28a13 100644 > >>--- a/block/qcow2-refcount.c > >>+++ b/block/qcow2-refcount.c > >>@@ -780,9 +780,11 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size) > >> BDRVQcowState *s = bs->opaque; > >> int64_t offset, cluster_offset, new_cluster; > >> int free_in_cluster, ret; > >>+ uint64_t refcount; > >> BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC_BYTES); > >> assert(size > 0 && size <= s->cluster_size); > >>+ redo: > >> if (s->free_byte_offset == 0) { > >> offset = qcow2_alloc_clusters(bs, s->cluster_size); > >> if (offset < 0) { > >>@@ -790,12 +792,25 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size) > >> } > >> s->free_byte_offset = offset; > >> } > >>- redo: > >>+ > >> free_in_cluster = s->cluster_size - > >> offset_into_cluster(s, s->free_byte_offset); > >> if (size <= free_in_cluster) { > >> /* enough space in current cluster */ > >> offset = s->free_byte_offset; > >>+ > >>+ if (offset_into_cluster(s, offset) != 0) { > >>+ /* We will have to increase the refcount of this cluster; if the > >>+ * maximum has been reached already, this cluster cannot be used */ > >>+ ret = qcow2_get_refcount(bs, offset >> s->cluster_bits, &refcount); > >>+ if (ret < 0) { > >>+ return ret; > >>+ } else if (refcount == s->refcount_max) { > >>+ s->free_byte_offset = 0; > >>+ goto redo; > >>+ } > >>+ } > >>+ > >> s->free_byte_offset += size; > >> free_in_cluster -= size; > >> if (free_in_cluster == 0) > >>@@ -816,6 +831,20 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size) > >> if ((cluster_offset + s->cluster_size) == new_cluster) { > >> /* we are lucky: contiguous data */ > >> offset = s->free_byte_offset; > >>+ > >>+ /* Same as above: In order to reuse the cluster, the refcount has to > >>+ * be increased; if that will not work, we are not so lucky after > >>+ * all */ > >>+ ret = qcow2_get_refcount(bs, offset >> s->cluster_bits, &refcount); > >>+ if (ret < 0) { > >>+ qcow2_free_clusters(bs, new_cluster, s->cluster_size, > >>+ QCOW2_DISCARD_NEVER); > >>+ return ret; > >>+ } else if (refcount == s->refcount_max) { > >>+ s->free_byte_offset = offset; > >I think you mean 0. offset is already the old value. > > Oh, right. Thanks for catching! > > >>+ goto redo; > >>+ } > >>+ > >> ret = qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, > >> 1, false, QCOW2_DISCARD_NEVER); > >> if (ret < 0) { > >I wonder if the code duplication is necessary. I was already thinking > >that there was some duplication when I reviewed the previous patch, but > >now it seems to become even more obvious that the three parts of this > >function are: > > > >1. Allocate a new cluster > >2. Allocate space in the already allocated cluster > >3. Allocate a new cluster and space inside it, which is just 1. + 2. > > Well, I can try rewriting this function, but I guess the diffcount > will be even higher. > > I think I'll try to write an independent patch which rewrites this > function and then drop these two patches from this series. Anything that makes the series shorter is welcome. ;-) In fact, I've been considering merging the first few patches even if they aren't valuable in themselves, just to make v6 shorter. Kevin