From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50586) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xo7iM-0003yr-87 for qemu-devel@nongnu.org; Tue, 11 Nov 2014 04:26:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xo7iG-0002Ox-2Z for qemu-devel@nongnu.org; Tue, 11 Nov 2014 04:26:18 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60986) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xo7iF-0002Oo-R0 for qemu-devel@nongnu.org; Tue, 11 Nov 2014 04:26:12 -0500 Message-ID: <5461D62D.9060403@redhat.com> Date: Tue, 11 Nov 2014 10:26:05 +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> <5461D61B.5010507@redhat.com> In-Reply-To: <5461D61B.5010507@redhat.com> Content-Type: text/plain; charset=iso-8859-15; 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:25, Max Reitz wrote: > 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. s/error/overflow/