From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54327) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YvtRL-00052t-37 for qemu-devel@nongnu.org; Fri, 22 May 2015 16:21:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YvtRJ-0003wQ-OM for qemu-devel@nongnu.org; Fri, 22 May 2015 16:21:07 -0400 Message-ID: <555F8FA5.9050209@redhat.com> Date: Fri, 22 May 2015 14:20:53 -0600 From: Eric Blake MIME-Version: 1.0 References: <1432266060-22104-1-git-send-email-famz@redhat.com> <1432266060-22104-4-git-send-email-famz@redhat.com> In-Reply-To: <1432266060-22104-4-git-send-email-famz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="3xfNLTtFBIVPUjOEuWGpwWQgItATUIBbO" Subject: Re: [Qemu-devel] [PATCH v4 3/8] mirror: Do zero write on target if sectors not allocated List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , qemu-block@nongnu.org, Jeff Cody , qemu-stable@nongnu.org, Stefan Hajnoczi , pbonzini@redhat.com, jsnow@redhat.com, wangxiaolong@ucloud.cn This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --3xfNLTtFBIVPUjOEuWGpwWQgItATUIBbO Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 05/21/2015 09:40 PM, Fam Zheng wrote: > If guest discards a source cluster, mirroring with bdrv_aio_readv is ov= erkill. > Some protocols do zero upon discard, where it's best to use > bdrv_aio_write_zeroes, otherwise, bdrv_aio_discard will be enough. >=20 > Signed-off-by: Fam Zheng > --- > block/mirror.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) >=20 > + > + ret =3D 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=3Dtrue' 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 spars= e 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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --3xfNLTtFBIVPUjOEuWGpwWQgItATUIBbO Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJVX4+lAAoJEKeha0olJ0NqXTgIAKad6KpnGLGpVvKlrG3N9hfP TajkQHSHR+zkSTUBxvE1W5TIo45Dbo/9pPKjnTLwKUD+8h9CbKBBUonVKtnXX3sg G431zZP+OoY17Ld2x+KBtP7CIf7YHCLwgE45ag8N7p1/+y9C/gJAhQD39X+Ud1I6 ywu+X9+GMq8Wbw2Jh0bW7MgcipzzXvXKx+Dcto6tNR3CBUbu9fZI242Bdub16TqF XEH/lFK3+TK4zQMLYKqsZiX925FGTTPSQ6G39QD6ik9mvsFrHMPoYu/xGKZFgWd9 xbWV7TEnvoYeJafgZHFWSd5WgjjI+rpWA02ZF0gUkG3cRFHEodoimcUdc3fQhnM= =Idnc -----END PGP SIGNATURE----- --3xfNLTtFBIVPUjOEuWGpwWQgItATUIBbO--