Re: [Qemu-devel] [PATCH 10/21] qcow2: refcount_order parameter for qcow2_create2

[Max Reitz]

On 2014-11-11 at 06:40, Eric Blake wrote:
> On 11/10/2014 06:45 AM, Max Reitz wrote:
> > Add a refcount_order parameter to qcow2_create2(), use that value for
> > the image header and for calculating the size required for
> > preallocation.
> >
> > For now, always pass 4.
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   block/qcow2.c | 41 ++++++++++++++++++++++++++++++-----------
> >   1 file changed, 30 insertions(+), 11 deletions(-)
> >
> > @@ -1811,6 +1811,13 @@ static int qcow2_create2(const char *filename, int64_t 
> > total_size,
> >           int64_t meta_size = 0;
> >           uint64_t nreftablee, nrefblocke, nl1e, nl2e;
> >           int64_t aligned_total_size = align_offset(total_size, cluster_size);
> > +        int refblock_bits, refblock_size;
> > +        /* refcount entry size in bytes */
> > +        double rces = (1 << refcount_order) / 8.;
> Would float be any simpler than double?

Maybe. I'm generally in favor of using float whenever possible, but I 
don't think it's necessary here. This is really not a performance issue 
here and having more precision is probably better.

> > +
> > +        /* see qcow2_open() */
> > +        refblock_bits = cluster_bits - (refcount_order - 3);
> > +        refblock_size = 1 << refblock_bits;
> >   
> >           /* header: 1 cluster */
> >           meta_size += cluster_size;
> > @@ -1835,20 +1842,20 @@ static int qcow2_create2(const char *filename, int64_t 
> > total_size,
> >            *   c = cluster size
> >            *   y1 = number of refcount blocks entries
> >            *   y2 = meta size including everything
> > +         *   rces = refcount entry size in bytes
> >            * then,
> >            *   y1 = (y2 + a)/c
> > -         *   y2 = y1 * sizeof(u16) + y1 * sizeof(u16) * sizeof(u64) / c + m
> > +         *   y2 = y1 * rces + y1 * rces * sizeof(u64) / c + m
> Hmm. This changes from integral to floating point.  Are we going to
> suffer from any rounding problems?

We are already suffering from rounding problems. See the actual code 
below this comment.

> I guess you want double to ensure
> maximum precision; but whereas earlier patches were limited with
> refcount_order of 6 to 2^63, this now limits us around 2^53 if we are
> still trying to be accurate.

We are not trying to be accurate, which is one of the reasons I rejected 
the first version of the patch which introduced this code (though that 
version had more issues which had been fixed in the final version). If 
you remember, I did write a very similar function which only used 
integral types in order not to suffer from precision problems. Some 
version of this code used that function, but then Kevin rejected my 
patch because it was too complicated, and this was taken in with the 
argument of not having to be really exact when it comes to 
preallocation: Being close enough should be fine.

So, in short, this code did not try to be exact from the beginning (it 
did use floating point arithmetic before this patch, see the "1.0 *"). 
If we want to be exact, there's still the code which I have the LaTeX 
PDF for. ;-)

Max