Re: [Qemu-devel] [PATCH 03/21] qcow2: Use 64 bits for refcount values

[Max Reitz]

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 <address@hidden>
> > > > ---
> > > >   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/