Re: [Qemu-devel] [RFC PATCH v4 4/4] qcow2: Add full image preallocation option

[Hu Tao]

Stefan, 

On Fri, Jan 17, 2014 at 04:48:16PM +0800, Stefan Hajnoczi wrote:
> On Fri, Dec 27, 2013 at 11:05:54AM +0800, Hu Tao wrote:
> 
> This approach seems okay but the calculation isn't quite right yet.
> 
> On Windows an error would be raised since we don't have preallocate=full
> support.  That's okay.
> 
> > @@ -1477,16 +1478,53 @@ static int qcow2_create2(const char *filename, 
> > int64_t total_size,
> >      Error *local_err = NULL;
> >      int ret;
> >  
> > +    if (prealloc == PREALLOC_MODE_FULL) {
> > +        int64_t meta_size = 0;
> > +        unsigned nrefe, nl2e;
> > +        BlockDriver *drv;
> > +
> > +        drv = bdrv_find_protocol(filename, true);
> > +        if (drv == NULL) {
> > +            error_setg(errp, "Could not find protocol for file '%s'", 
> > filename);
> > +            return -ENOENT;
> > +        }
> > +
> > +        alloc_options = append_option_parameters(alloc_options,
> > +                                                 drv->create_options);
> > +        alloc_options = append_option_parameters(alloc_options, options);
> > +
> > +        /* header: 1 cluster */
> > +        meta_size += cluster_size;
> > +        /* total size of refblocks */
> > +        nrefe = (total_size / cluster_size);
> > +        nrefe = align_offset(nrefe, cluster_size / sizeof(uint16_t));
> > +        meta_size += nrefe * sizeof(uint16_t);
> 
> Every host cluster is reference-counted, including metadata (even
> refcount blocks are recursively included).  This calculation is wrong
> because it only considers data clusters.

Right. I'll fix this.

> 
> > +        /* total size of reftables */
> > +        meta_size += nrefe * sizeof(uint16_t) * sizeof(uint16_t) / 
> > cluster_size;
> 
> I don't understand this calculation.  The refcount table consists of
> contiguous clusters where each element is a 64-bit offset to a refcount
> block.
> 
> > +        /* total size of L2 tables */
> > +        nl2e = total_size / cluster_size;
> > +        nl2e = align_offset(nl2e, cluster_size / sizeof(uint64_t));
> > +        meta_size += nl2e * sizeof(uint64_t);
> > +        /* total size of L1 tables */
> > +        meta_size += nl2e * sizeof(uint64_t) * sizeof(uint64_t) / 
> > cluster_size;
> 
> Another strange calculation.  The L1 table consists of contiguous
> clusters where each element is a 64-bit offset to an L1 table.

I guest you mean "...offset to an L2 table" here.

Given the number of total L2 table entries nl2e, the number of L2 tables
is:

  nl2table = nl2e * sizeof(l2e) / cluster_size

because each L1 table entry points to a L2 table, so this is also the
number of L1 table entries. So the total size of L1 tables is:

  l1table_size = nl2table * sizeof(l1e)
               = nl2e * sizeof(l2e) * sizeof(l1e) / cluster_size
               = nl2e * sizeof(uint64_t) * sizeof(uint64_t) / cluster_size

I still can't see what's wrong here. But you're welcome to fix me.


The caculation of reftable size above is wrong because of the two problem
you've pointed out. Thanks!