Re: [Qemu-devel] [PATCH v2 05/15] qcow2: do not COW the empty areas

[Eric Blake]

On 06/01/2017 10:14 AM, Anton Nefedov wrote:
> If COW area of the newly allocated cluster is zeroes, there is no reason
> to write zero sectors in perform_cow() again now as whole clusters are
> zeroed out in single chunks by handle_alloc_space().
> 
> Introduce QCowL2Meta field "reduced", since the existing fields
> (offset and nb_bytes) still has to keep other write requests from
> simultaneous writing in the area
> 
> iotest 060:
> write to the discarded cluster does not trigger COW anymore.
> so, break on write_aio event instead, will work for the test
> (but write won't fail anymore, so update reference output)
> 
> iotest 066:
> cluster-alignment areas that were not really COWed are now detected
> as zeroes, hence the initial write has to be exactly the same size for
> the maps to match

> +++ b/block/qcow2.c
> @@ -64,6 +64,9 @@ typedef struct {
>  #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>  #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>  
> +static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
> +                            uint32_t count);
> +

I've already posted patches that redo this function to be byte-based;
I'm not sure which of our series will land first, but it is something to
keep in mind.

I'm also not a big fan of forward declarations of leaf static functions
(they are essential for mutually recursive functions, but when there is
no recursion, the better solution is to just hoist the implementation of
the function to occur where it is first needed, rather than using a
forward declaration).

> @@ -1588,8 +1611,13 @@ static void handle_alloc_space(BlockDriverState *bs, 
> QCowL2Meta *l2meta)
>          }
>  
>          /* try to alloc host space in one chunk for better locality */
> -        bdrv_co_pwrite_zeroes(bs->file, m->alloc_offset, bytes,
> -                              BDRV_REQ_ALLOCATE);
> +        ret = bdrv_co_pwrite_zeroes(bs->file, m->alloc_offset, bytes,
> +                                    BDRV_REQ_ALLOCATE);
> +        if (ret != 0) {
> +            continue;
> +        }
> +
> +        handle_cow_reduce(bs, m);

Does this look any better as:

if (bdrv_co_pwrite_zeroes(...) == 0) {
    handle_cow_reduce(bs, m);
}

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature