From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56764) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGsHO-0001zQ-4M for qemu-devel@nongnu.org; Wed, 06 Jan 2016 12:53:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGsHN-00089n-67 for qemu-devel@nongnu.org; Wed, 06 Jan 2016 12:53:50 -0500 References: <1451983580-10561-1-git-send-email-famz@redhat.com> <1451983580-10561-2-git-send-email-famz@redhat.com> From: Max Reitz Message-ID: <568D54A3.6020803@redhat.com> Date: Wed, 6 Jan 2016 18:53:39 +0100 MIME-Version: 1.0 In-Reply-To: <1451983580-10561-2-git-send-email-famz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="NDiITjBc8QXIKBKB5jFE5XpIxEvGK0fLQ" Subject: Re: [Qemu-devel] [PATCH v9 1/2] mirror: Rewrite mirror_iteration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , pbonzini@redhat.com, Jeff Cody , qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --NDiITjBc8QXIKBKB5jFE5XpIxEvGK0fLQ Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 05.01.2016 09:46, Fam Zheng wrote: > The "pnum < nb_sectors" condition in deciding whether to actually copy > data is unnecessarily strict, and the qiov initialization is > unnecessarily for bdrv_aio_write_zeroes and bdrv_aio_discard. >=20 > Rewrite mirror_iteration to fix both flaws. >=20 > Signed-off-by: Fam Zheng > --- > block/mirror.c | 347 +++++++++++++++++++++++++++++++++++--------------= -------- > trace-events | 1 - > 2 files changed, 216 insertions(+), 132 deletions(-) >=20 > diff --git a/block/mirror.c b/block/mirror.c > index f201f2b..e3e9fad 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -46,7 +46,6 @@ typedef struct MirrorBlockJob { > BlockdevOnError on_source_error, on_target_error; > bool synced; > bool should_complete; > - int64_t sector_num; > int64_t granularity; > size_t buf_size; > int64_t bdev_length; > @@ -63,6 +62,8 @@ typedef struct MirrorBlockJob { > int ret; > bool unmap; > bool waiting_for_io; > + int target_cluster_sectors; > + int max_iov; > } MirrorBlockJob; > =20 > typedef struct MirrorOp { > @@ -158,115 +159,93 @@ static void mirror_read_complete(void *opaque, i= nt ret) > mirror_write_complete, op); > } > =20 > -static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) > +/* Round sector_num and/or nb_sectors to target cluster if COW is need= ed, and > + * return the offset of the adjusted tail sector against original. */ > +static int mirror_cow_align(MirrorBlockJob *s, > + int64_t *sector_num, > + int *nb_sectors) > +{ > + bool head_need_cow, tail_need_cow; > + int diff =3D 0; > + int chunk_sectors =3D s->granularity >> BDRV_SECTOR_BITS; > + > + head_need_cow =3D !test_bit(*sector_num / chunk_sectors, s->cow_bi= tmap); > + tail_need_cow =3D !test_bit((*sector_num + *nb_sectors - 1) / chun= k_sectors, > + s->cow_bitmap); > + if (head_need_cow || tail_need_cow) { > + int64_t align_sector_num; > + int align_nb_sectors; > + bdrv_round_to_clusters(s->target, *sector_num, *nb_sectors, > + &align_sector_num, &align_nb_sectors); > + if (tail_need_cow) { > + diff =3D align_sector_num + align_nb_sectors > + - (*sector_num + *nb_sectors); > + assert(diff >=3D 0); > + *nb_sectors +=3D diff; > + } > + if (head_need_cow) { > + int d =3D *sector_num - align_sector_num; > + assert(d >=3D 0); > + *sector_num =3D align_sector_num; > + *nb_sectors +=3D d; > + } > + } > + > + /* If the resulting chunks are more than max_iov, we have to shrin= k it > + * under the alignment restriction. */ > + if (*nb_sectors > chunk_sectors * s->max_iov) { > + int shrink =3D *nb_sectors - chunk_sectors * s->max_iov; > + if (tail_need_cow) { > + /* In this case, tail must be aligned already, so we just = make sure > + * the shrink is also aligned. */ > + shrink -=3D shrink % s->target_cluster_sectors; > + } > + assert(shrink); > + diff -=3D shrink; > + *nb_sectors -=3D shrink; > + } Hm, looking at this closer... If we get here with tail_need_cow not being set, we may end up with an unaligned tail, which then may need COW (because it points to somewhere else than before). On the other hand, if we get here with tail_need_cow being set, shrink will be decreased so that it will only remove an aligned number of sectors from *nb_sectors; however, because shrink is increased, that means that *nb_sectors may then still be too large. Also, because of the shrink, the tail may in fact not need COW any more. Should we do this check before we test whether we need COW and do the correction in a way that ensures that the cluster adjustment can never increase *nb_sectors beyond chunk_sectors * s->max_iov? Max --NDiITjBc8QXIKBKB5jFE5XpIxEvGK0fLQ 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 iQEcBAEBCAAGBQJWjVSjAAoJEDuxQgLoOKytFAoIAJlX5FoLZP1hbAw/YX7cald0 35I1S3APwODMETtXrjcEvv9kyAyOqPX72ud50CneegBzLrXQPDXy+HILYN3VDlLy Jqh5sxjGwRu3zXcqs0H77ihByB2/NcZ/h/hbdfQWCshMCpdPdpTwXMPAASR5BF0Q wx0LuYThwhywk+PMEtUJ3skkVX+TfIEexoMT1At0Cqpn4itQXSnnbYBPdlb/VTEu eke0EruzT9TlA3rB1SKdy0BqydJxyWD6xNBzcF/zhaD/s7j+1/skBoppj2Fjxtod Ehc9R9lU80TTcBRIFEJzG0dCaNC03LjUAnhQmTzDqkpC4kLfSTehcHz7+Msmlqc= =lT2W -----END PGP SIGNATURE----- --NDiITjBc8QXIKBKB5jFE5XpIxEvGK0fLQ--