Re: [Qemu-devel] [PATCH v8 03/14] qcow2: Optimize bdrv_make_empty()

[Hu Tao]

On Mon, Jun 30, 2014 at 01:33:39PM +0200, Kevin Wolf wrote:
> Am 07.06.2014 um 20:51 hat Max Reitz geschrieben:
> > bdrv_make_empty() is currently only called if the current image
> > represents an external snapshot that has been committed to its base
> > image; it is therefore unlikely to have internal snapshots. In this
> > case, bdrv_make_empty() can be greatly sped up by creating an empty L1
> > table and dropping all data clusters at once by recreating the refcount
> > structure accordingly instead of normally discarding all clusters.
> > 
> > If there are snapshots, fall back to the simple implementation (discard
> > all clusters).
> > 
> > Signed-off-by: Max Reitz <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> 
> This approach looks a bit too complicated to me, and calulating the
> required metadata size seems error-prone.
> 
> How about this:
> 
> 1. Set the dirty flag in the header so we can mess with the L1 table
>    without keeping the refcounts consistent
> 
> 2. Overwrite the L1 table with zeros
> 
> 3. Overwrite the first n clusters after the header with zeros
>    (n = 2 + l1_clusters).
> 
> 4. Update the header:
>    refcount_table_offset = cluster_size
>    refcount_table_clusters = 1
>    l1_table_offset = 3 * cluster_size
> 
> 6. bdrv_truncate to n + 1 clusters
> 
> 7. Now update the first 8 bytes at cluster_size (the first new refcount
>    table entry) to point to 2 * cluster_size (new refcount block)
> 
> 8. Reset refcount block and L2 cache
> 
> 9. Allocate n + 1 clusters (the header, too) and make sure you get
>    offset 0
> 
> 10. Remove the dirty flag
> 
> Surprisingly (or not) this is much like an ordinary image creation. The
> main difference is that we keep the full size of the L1 table so the
> image stays always valid (the spec would even allow us to temporarily
> set l1_size = 0, but qcow2_open() doesn't seem to like that) and all
> areas where the L1 table could be are zeroed (this includes the new
> refcount table/block until the header is updated).

Kevin,

It seems that this approach doesn't need calculation of metadata
size(minimal_blob_size()), which is exactly the one prealllocation=full
will depend on.

> 
> 
> I wanted to check whether this would still give the preallocation=full
> series what it needs, but a v11 doesn't seem to be on the list yet and
> v10 doesn't have the dependency on this series yet.

Although I'm now have v11 done, I'm not sure it's ready to post since
you rejected the calculation of metadata size. But for you to check how
the series depends on this patch, I uploaded it to github at
https://github.com/taohu/qemu/commits/preallocation-v11.
(specifically, the dependency exists on commit
https://github.com/taohu/qemu/commit/308720c6b10166d60045c81a4d9fab7205c85986)

If you think it's not a problem to post v11, just tell me and I can post
to list.

Regards,
Hu