Re: [Qemu-devel] [PATCH v5 07/12] qemu-img: Empty images after commit

[Max Reitz]

On 22.04.2014 18:22, Max Reitz wrote:
> On 22.04.2014 17:19, Eric Blake wrote:
> > On 04/17/2014 03:59 PM, Max Reitz wrote:
> > > After the top image has been committed into an image in its backing
> > > chain, all images above that base image should be emptied to restore 
> > > the
> > > old qemu-img commit behavior.
> > >
> > > Signed-off-by: Max Reitz <address@hidden>
> > > ---
> > >   qemu-img.c | 87 
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> > >   1 file changed, 84 insertions(+), 3 deletions(-)
> > Does emptying an image take significant time?  If so, does that need to
> > be reflected in the progress meter?
>
> For a 16 GB image I have here (should be nearly full) it took 1:22 
> min. Copying it took six minutes, so I guess committing it would take 
> even more. I think the ratio is small enough not to include it in the 
> progress meter.
>
> Furthermore, I don't see a reasonable implementation for a make_empty 
> progress output: As a general implementation for all image formats 
> implementing discard will not work (the qcow2 implementation clearly 
> states that discarded sectors should read back as zero), we would need 
> some way of returning the progress in every format-specific make_empty 
> implementation. This would probably require callbacks which I don't 
> see feasible.
>
> An in my opinion more reasonable approach would be to speed up the 
> implementation of make_empty for qcow2 - as qcow does it, we could 
> just create a new empty image (that is, rewrite the L1 and refcount 
> table and truncate the image appropriately) instead of discarding 
> every cluster in the existing file. This would be much faster, but the 
> implementation would not be as nice and easy, so I chose not to 
> implement it. I guess I'll have a second look, then. ;-)

Okay, after having begun to implement this, I noticed that this approach 
will not work for images with internal snapshots as we probably want to 
keep those during make_empty. On the other hand, I guess few people will 
use internal and external snapshots at the same time; I guess I'll 
implement both paths then. If there are internal snapshots, fall back to 
the implementation discarding every clusters, if there aren't (which is 
more likely), recreate the image.

Max