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

[Kevin Wolf <kwolf@redhat.com>]

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 <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

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).


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.

Kevin