[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v4 3/8] mirror: Do zero write on target if secto

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v4 3/8] mirror: Do zero write on target if sectors not allocated
Date: Fri, 22 May 2015 14:20:53 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0

On 05/21/2015 09:40 PM, Fam Zheng wrote:
> If guest discards a source cluster, mirroring with bdrv_aio_readv is overkill.
> Some protocols do zero upon discard, where it's best to use
> bdrv_aio_write_zeroes, otherwise, bdrv_aio_discard will be enough.
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  block/mirror.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> +
> +    ret = bdrv_get_block_status(source, NULL, sector_num, nb_sectors, &pnum);

Ah, you are checking the entire chain for allocation, so if it is
unallocated through all layers, then the destination doesn't need to
allocate it either.  But is this the correct location to start with,
when the block-mirror is shallow?

> +    if (ret < 0 || pnum < nb_sectors ||
> +            (ret & BDRV_BLOCK_ALLOCATED && !(ret & BDRV_BLOCK_ZERO))) {
> +        bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,

Do we still want to call bdrv_aio_readv() if ret < 0 (where it will
likely fail), or should this 'if' be broken into two clauses?

> +                       mirror_read_complete, op);
> +    } else if (ret & BDRV_BLOCK_ZERO) {
> +        bdrv_aio_write_zeroes(s->target, sector_num, op->nb_sectors,
> +                              s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
> +                              mirror_write_complete, op);
> +    } else {
> +        assert(!(ret & BDRV_BLOCK_ALLOCATED));
> +        bdrv_aio_discard(s->target, sector_num, op->nb_sectors,
> +                         mirror_write_complete, op);
> +    }

I'm okay with what happens when you mirror to a flat image, as in
copying "base <- active" into "copy".  There, the copy can omit clusters
that are not allocated in anywhere in the source chain, and can also
omit clusters if the source has all zero in the cluster, and the
destination would read back all zero even if the cluster is unallocated.

But I'm worried about a shallow copy.  If I start with "base <- active",
where "active" has an explicit zero cluster that is overwriting an
allocated non-zero cluster in "base", and I'm creating the shallow clone
to "base <- copy", then the default of 'unmap=true' says that
bdrv_aio_write_zeroes() may attempt to unmap the cluster in "copy".  At
which point, doesn't that mean that reading from "copy" will dredge up
the non-zero data from "base", which is NOT a faithful mirroring of

Or symbolically, suppose I have this layout, with letters for non-zero
clusters, 0 for explicit zero clusters, and - for unallocated clusters:

base   AAA000---
active -0B-0B-0B   # Guest sees A0B00B00B

If I'm understanding your code correctly, a deep block-mirror will
create either:

copy   A-B--B--B   # Guest sees A0B00B00B, unmap was true, image is sparse


copy   A0B00B-0B   # Guest sees A0B00B00B, unmap was false, image is

But a shallow block-mirror will cause:

base   AAA000---
copy   -0B-0B-0B   # Guest sees A0B00B00B, unmap was false


base   AAA000---
copy   --B--B--B   # Guest sees AAB00B00B, unmap was true

Whoops - unmapping a cluster in the destination which was all zeros in
the source caused corruption in what the guest sees.

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

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]