[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
"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
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v4 0/8] block: Mirror discarded sectors, Fam Zheng, 2015/05/21
- [Qemu-devel] [PATCH v4 1/8] block: Add "base" option to bdrv_get_block_status, Fam Zheng, 2015/05/21
- [Qemu-devel] [PATCH v4 2/8] qmp: Add optional bool "unmap" to drive-mirror, Fam Zheng, 2015/05/21
- [Qemu-devel] [PATCH v4 3/8] mirror: Do zero write on target if sectors not allocated, Fam Zheng, 2015/05/21
- [Qemu-devel] [PATCH v4 4/8] block: Fix dirty bitmap in bdrv_co_discard, Fam Zheng, 2015/05/21
- [Qemu-devel] [PATCH v4 5/8] block: Remove bdrv_reset_dirty, Fam Zheng, 2015/05/21
- [Qemu-devel] [PATCH v4 6/8] qemu-iotests: Make block job methods common, Fam Zheng, 2015/05/21
- [Qemu-devel] [PATCH v4 7/8] qemu-iotests: Add test case for mirror with unmap, Fam Zheng, 2015/05/21
- [Qemu-devel] [PATCH v4 8/8] iotests: Use event_wait in wait_ready, Fam Zheng, 2015/05/21