On 07/30/2014 10:14 AM, Eric Blake wrote:
On 07/25/2014 12:07 PM, Max Reitz wrote:
Actually, we do not need to allocate a new data cluster for every zero
cluster to be expanded: It is completely sufficient to rely on qcow2's
COW part and instead create a single zero cluster and reuse it as much
as possible.
Also, I have to wonder - since the all-zero cluster is the most likely
cluster to have a large refcount, even during normal runtime, should we
special case the normal qcow2 write code to track the current all-zero
cluster (if any), and merely increase its refcount rather than allocate
a new cluster any time it is detected that an all-zero cluster is
needed? [Of course, the tracking would be runtime only, since
compat=0.10 header doesn't provide any way to track the location of an
all-zero cluster across file reloads. Each new runtime would probably
settle on a new location for the all-zero cluster used during that run,
rather than trying to find an existing one. And there's really no point
to adding a header to track an all-zero cluster in compat=1.1 images,
since those images already have the ability to track zero clusters
without needing one allocated.]
+ ret = bdrv_write_zeroes(bs->file, offset /
BDRV_SECTOR_SIZE,
+ s->cluster_sectors, 0);
That is, if bdrv_write_zeroes knows how to take advantage of an already
existing all-zero cluster, it would be less special casing in this code,
but still get the same benefits of maximizing refcount during the amend
operation, if all expanded clusters go through bdrv_write_zeroes.
Now that I've looked through both variants, I'm leaning towards the
simplicity of your alternate series, rather than the complexity of this
one, if we can (independently?) optimize bdrv_write_zeroes to reuse a
known-all-zeroes cluster when possible. Of course, you may want to get
other opinions than just mine before posting your next round of these
patches.