From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50523) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xo7i4-0003d0-Ok for qemu-devel@nongnu.org; Tue, 11 Nov 2014 04:26:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xo7hy-0002LS-Ef for qemu-devel@nongnu.org; Tue, 11 Nov 2014 04:26:00 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38730) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xo7hy-0002LO-59 for qemu-devel@nongnu.org; Tue, 11 Nov 2014 04:25:54 -0500 Message-ID: <5461D61B.5010507@redhat.com> Date: Tue, 11 Nov 2014 10:25:47 +0100 From: Max Reitz MIME-Version: 1.0 References: <1415627159-15941-1-git-send-email-mreitz@redhat.com> <1415627159-15941-4-git-send-email-mreitz@redhat.com> <54612727.6030900@redhat.com> <20141111092253.GA3674@noname.redhat.com> In-Reply-To: <20141111092253.GA3674@noname.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 03/21] qcow2: Use 64 bits for refcount values List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , Eric Blake Cc: Peter Lieven , qemu-devel@nongnu.org, Stefan Hajnoczi On 2014-11-11 at 10:22, Kevin Wolf wrote: > Am 10.11.2014 um 21:59 hat Eric Blake geschrieben: >> On 11/10/2014 06:45 AM, Max Reitz wrote: >>> Refcounts may have a width of up to 64 bit, so qemu should use the same >> s/bit/bits/ >> >>> width to represent refcount values internally. >>> >>> Signed-off-by: Max Reitz >>> --- >>> block/qcow2-cluster.c | 9 ++++++--- >>> block/qcow2-refcount.c | 37 ++++++++++++++++++++----------------- >>> block/qcow2.h | 7 ++++--- >>> 3 files changed, 30 insertions(+), 23 deletions(-) >>> >>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c >>> index df0b2c9..ab43902 100644 >>> --- a/block/qcow2-cluster.c >>> +++ b/block/qcow2-cluster.c >>> @@ -1640,7 +1640,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table, >>> for (i = 0; i < l1_size; i++) { >>> uint64_t l2_offset = l1_table[i] & L1E_OFFSET_MASK; >>> bool l2_dirty = false; >>> - int l2_refcount; >>> + int64_t l2_refcount; >> You may want to mention in the commit message that you choose a signed >> type to allow negative for errors, and therefore we really allow only up >> to 63 useful bits. Or even mention that this is okay because no one >> can feasibly generate an image with more than 2^63 refs to the same >> cluster (there isn't that much storage or time to do such a task in our >> lifetime...) > Should patch 1 then set refcount_max = 2^63 for refcount order 6? It does set refcount_max to INT64_MAX (instead of UINT64_MAX, and there is a comment above that line why it's the signed maximum). > Also note that while it might not be feasible to create a cluster with > 2^63 references, this doesn't mean that it's impossible to create a > cluster with a stored refcount of (more than) 2^63. We'll have to have > checks there. Yes, the check is done in qcow2_get_refcount() (and needs to be done in update_refcount_discard() as well, which I forgot in this version) and consists of returning -ERANGE on error. Max