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

[Hu Tao]

On Mon, Jan 20, 2014 at 10:16:23AM +0800, Hu Tao wrote:
> 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.

Hi,

Any comments?

-- 
Regards,
Hu Tao