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

[Eric Blake]

On 10/24/2014 07:57 AM, Max Reitz wrote:
> 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 emptying the L1 and
> refcount table (while having the dirty flag set, which only works for
> compat=1.1) and creating a trivial refcount structure.
> 
> If there are snapshots or for compat=0.10, fall back to the simple
> implementation (discard all clusters).
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block/blkdebug.c      |   2 +
>  block/qcow2.c         | 165 
> +++++++++++++++++++++++++++++++++++++++++++++++++-
>  include/block/block.h |   2 +
>  3 files changed, 168 insertions(+), 1 deletion(-)
> 

Looks like you resolved Kevin's review points correctly (I'm trusting
his judgment here).


>  static int qcow2_make_empty(BlockDriverState *bs)
>  {
> -    int ret = 0;
> +    BDRVQcowState *s = bs->opaque;
>      uint64_t start_sector;
>      int sector_step = INT_MAX / BDRV_SECTOR_SIZE;
> +    int l1_clusters, ret = 0;
> +
> +    l1_clusters = DIV_ROUND_UP(s->l1_size, s->cluster_size / 
> sizeof(uint64_t));
> +
> +    if (s->qcow_version >= 3 && !s->snapshots &&
> +        3 + l1_clusters <= s->refcount_block_size) {
> +        /* The following function only works for qcow2 v3 images (it requires
> +         * the dirty flag) and only as long as there are no snapshots 
> (because
> +         * it completely empties the image). Furthermore, the L1 table and 
> three
> +         * additional clusters (image header, refcount table, one refcount
> +         * block) have to fit inside one refcount block. */
> +        return make_completely_empty(bs);
> +    }

Is there ever a situation where if make_completely_empty() fails, you
can still safely fall back to the slower loop code?  For example,
failure to do qcow2_cache_empty() at the very beginning might still be
recoverable (basically, anywhere that did 'goto fail' instead of 'goto
fail_broken_refcounts' might have a chance at the fallback working).
But I'm okay with declaring all errors as fatal to the attempt, rather
than trying to figure out which errors are worth still trying the
fallback (especially since most errors are going to be caused by OOM or
file I/O error, and those are likely to still be triggered again during
fallback).

>  
> +    /* This fallback code simply discards every active clusters; this is 
> slow,

s/clusters/cluster/

> +     * but works in all cases */
>      for (start_sector = 0; start_sector < bs->total_sectors;
>           start_sector += sector_step)
>      {

With the comment typo fixed (the maintainer could do that without
needing a v15):
Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature