From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48755) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFLsz-0007rj-U9 for qemu-devel@nongnu.org; Mon, 17 Feb 2014 05:57:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WFLsu-0008Kk-SI for qemu-devel@nongnu.org; Mon, 17 Feb 2014 05:57:17 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57737) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFLsu-0008Kg-Ko for qemu-devel@nongnu.org; Mon, 17 Feb 2014 05:57:12 -0500 Date: Mon, 17 Feb 2014 11:57:09 +0100 From: Kevin Wolf Message-ID: <20140217105709.GF3502@dhcp-200-207.str.redhat.com> References: <1392242799-16364-1-git-send-email-benoit.canet@irqsave.net> <1392242799-16364-6-git-send-email-benoit.canet@irqsave.net> <52FEC576.9040205@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <52FEC576.9040205@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH V17 05/12] quorum: Add quorum_aio_readv. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , famz@redhat.com, =?iso-8859-1?Q?Beno=EEt?= Canet , qemu-devel@nongnu.org, stefanha@redhat.com Am 15.02.2014 um 02:40 hat Max Reitz geschrieben: > On 12.02.2014 23:06, Beno=EEt Canet wrote: > >From: Beno=EEt Canet > > > >Add code to do num_children reads in parallel and cleanup the structur= es > >afterward. >=20 > "afterwards" >=20 > >Signed-off-by: Benoit Canet > >Reviewed-by: Max Reitz > >--- > > block/quorum.c | 39 ++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > >diff --git a/block/quorum.c b/block/quorum.c > >index 197cdca..c7a5d79 100644 > >--- a/block/quorum.c > >+++ b/block/quorum.c > >@@ -91,10 +91,18 @@ static AIOCBInfo quorum_aiocb_info =3D { > > static void quorum_aio_finalize(QuorumAIOCB *acb) > > { > >- int ret =3D 0; > >+ BDRVQuorumState *s =3D acb->common.bs->opaque; > >+ int i, ret =3D 0; > > acb->common.cb(acb->common.opaque, ret); > >+ if (acb->is_read) { > >+ for (i =3D 0; i < s->num_children; i++) { > >+ qemu_vfree(acb->qcrs[i].buf); > >+ qemu_iovec_destroy(&acb->qcrs[i].qiov); > >+ } > >+ } > >+ > > g_free(acb->qcrs); > > qemu_aio_release(acb); > > } > >@@ -149,6 +157,34 @@ static void quorum_aio_cb(void *opaque, int ret) > > quorum_aio_finalize(acb); > > } > >+static BlockDriverAIOCB *quorum_aio_readv(BlockDriverState *bs, > >+ int64_t sector_num, > >+ QEMUIOVector *qiov, > >+ int nb_sectors, > >+ BlockDriverCompletionFunc *c= b, > >+ void *opaque) > >+{ > >+ BDRVQuorumState *s =3D bs->opaque; > >+ QuorumAIOCB *acb =3D quorum_aio_get(s, bs, qiov, sector_num, > >+ nb_sectors, cb, opaque); > >+ int i; > >+ > >+ acb->is_read =3D true; > >+ > >+ for (i =3D 0; i < s->num_children; i++) { > >+ acb->qcrs[i].buf =3D qemu_blockalign(s->bs[i], qiov->size); > >+ qemu_iovec_init(&acb->qcrs[i].qiov, qiov->niov); > >+ qemu_iovec_clone(&acb->qcrs[i].qiov, qiov, acb->qcrs[i].buf); > >+ } > >+ > >+ for (i =3D 0; i < s->num_children; i++) { > >+ bdrv_aio_readv(s->bs[i], sector_num, &acb->qcrs[i].qiov, nb_s= ectors, > >+ quorum_aio_cb, &acb->qcrs[i]); >=20 > I know Kevin told you to, but using the child's own QIOV here means > quorum_aio_readv() won't work after this patch but only after patch > 6. Just pointing it out, I don't like it, but Kevin told you to, so: Sorry, but this is the entirely wrong attitude. Just because I am a maintainer that doesn't mean that I don't get things wrong. When I talk bullshit (and probably I do more often than others), just point it out. In this case, I didn't even tell him to do the change, but I asked in patch 6: > Why don't you do this from the beginning? The answer appears to be that the read data isn't copied back to the original qiov yet, but this happens only in the quorum_copy_qiov() calls in quorum_vote(). So this is a bug in this patch, which can be fixed either by reverting to the old state of using the original qiov here, or (perhaps more nicely) by introducing quorum_copy_qiov() already in this patch. Kevin