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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3347 bytes --]

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 <famz@redhat.com>
> ---
>  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
"active"?

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

or

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

But a shallow block-mirror will cause:

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

or

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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]