From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45156) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VQczs-0003IZ-6v for qemu-devel@nongnu.org; Mon, 30 Sep 2013 08:54:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VQczj-0004NW-Kb for qemu-devel@nongnu.org; Mon, 30 Sep 2013 08:54:43 -0400 Received: from nodalink.pck.nerim.net ([62.212.105.220]:41028 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VQczj-0004NH-4V for qemu-devel@nongnu.org; Mon, 30 Sep 2013 08:54:35 -0400 Date: Mon, 30 Sep 2013 14:54:26 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20130930125426.GA3318@irqsave.net> References: <1359392845-15905-1-git-send-email-benoit@irqsave.net> <1359392845-15905-4-git-send-email-benoit@irqsave.net> <5114D5AE.6070901@redhat.com> <20130926162930.GE3338@irqsave.net> <20130927100307.GI2440@dhcp-200-207.str.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20130927100307.GI2440@dhcp-200-207.str.redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC V8 03/13] quorum: Add quorum_aio_writev and its dependencies. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , qemu-devel@nongnu.org, stefanha@redhat.com Le Friday 27 Sep 2013 =E0 12:03:07 (+0200), Kevin Wolf a =E9crit : > Am 26.09.2013 um 18:29 hat Beno=EEt Canet geschrieben: > > Le Friday 08 Feb 2013 =E0 11:38:38 (+0100), Kevin Wolf a =E9crit : > > > Am 28.01.2013 18:07, schrieb Beno=EEt Canet: > > > > Signed-off-by: Benoit Canet > > > > --- > > > > block/quorum.c | 111 ++++++++++++++++++++++++++++++++++++++++++= ++++++++++++++ > > > > 1 file changed, 111 insertions(+) > > > >=20 > > > > diff --git a/block/quorum.c b/block/quorum.c > > > > index d8fffbe..5d8470b 100644 > > > > --- a/block/quorum.c > > > > +++ b/block/quorum.c > > > > @@ -52,11 +52,122 @@ struct QuorumAIOCB { > > > > int vote_ret; > > > > }; > > > > =20 > > > > +static void quorum_aio_cancel(BlockDriverAIOCB *blockacb) > > > > +{ > > > > + QuorumAIOCB *acb =3D container_of(blockacb, QuorumAIOCB, com= mon); > > > > + bool finished =3D false; > > > > + > > > > + /* Wait for the request to finish */ > > > > + acb->finished =3D &finished; > > > > + while (!finished) { > > > > + qemu_aio_wait(); > > > > + } > > > > +} > > > > + > > > > +static AIOCBInfo quorum_aiocb_info =3D { > > > > + .aiocb_size =3D sizeof(QuorumAIOCB), > > > > + .cancel =3D quorum_aio_cancel, > > > > +}; > > > > + > > > > +static void quorum_aio_bh(void *opaque) > > > > +{ > > > > + QuorumAIOCB *acb =3D opaque; > > > > + BDRVQuorumState *s =3D acb->bqs; > > > > + int ret; > > > > + > > > > + ret =3D s->threshold <=3D acb->success_count ? 0 : -EIO; > > >=20 > > > It would be very much preferable if you stored the actual error cod= e > > > instead of turning everything into -EIO. > > >=20 > > > > + > > > > + qemu_bh_delete(acb->bh); > > > > + acb->common.cb(acb->common.opaque, ret); > > > > + if (acb->finished) { > > > > + *acb->finished =3D true; > > > > + } > > > > + g_free(acb->aios); > > > > + qemu_aio_release(acb); > > > > +} > > >=20 > > > Move this down so that it's next to the function using the bottom h= alf. > > >=20 > > > > + > > > > +static QuorumAIOCB *quorum_aio_get(BDRVQuorumState *s, > > > > + BlockDriverState *bs, > > > > + QEMUIOVector *qiov, > > > > + uint64_t sector_num, > > > > + int nb_sectors, > > > > + BlockDriverCompletionFunc *cb= , > > > > + void *opaque) > > > > +{ > > > > + QuorumAIOCB *acb =3D qemu_aio_get(&quorum_aiocb_info, bs, cb= , opaque); > > > > + int i; > > > > + > > > > + acb->aios =3D g_new0(QuorumSingleAIOCB, s->total); > > > > + > > > > + acb->bqs =3D s; > > > > + acb->qiov =3D qiov; > > > > + acb->bh =3D NULL; > > > > + acb->count =3D 0; > > > > + acb->success_count =3D 0; > > > > + acb->sector_num =3D sector_num; > > > > + acb->nb_sectors =3D nb_sectors; > > > > + acb->vote =3D NULL; > > > > + acb->vote_ret =3D 0; > > > > + acb->finished =3D NULL; > > > > + > > > > + for (i =3D 0; i < s->total; i++) { > > > > + acb->aios[i].buf =3D NULL; > > > > + acb->aios[i].ret =3D 0; > > > > + acb->aios[i].parent =3D acb; > > > > + } > > >=20 > > > Would you mind to reorder the initialisation of the fields accordin= g to > > > the order that is used in the struct definition? > > >=20 > > > > + > > > > + return acb; > > > > +} > > > > + > > > > +static void quorum_aio_cb(void *opaque, int ret) > > > > +{ > > > > + QuorumSingleAIOCB *sacb =3D opaque; > > > > + QuorumAIOCB *acb =3D sacb->parent; > > > > + BDRVQuorumState *s =3D acb->bqs; > > > > + > > > > + sacb->ret =3D ret; > > > > + acb->count++; > > > > + if (ret =3D=3D 0) { > > > > + acb->success_count++; > > > > + } > > > > + assert(acb->count <=3D s->total); > > > > + assert(acb->success_count <=3D s->total); > > > > + if (acb->count < s->total) { > > > > + return; > > > > + } > > > > + > > > > + acb->bh =3D qemu_bh_new(quorum_aio_bh, acb); > > > > + qemu_bh_schedule(acb->bh); > > >=20 > > > What's the reason for using a bottom half here? Worth a comment? > > >=20 > > > multiwrite_cb() in block.c doesn't use one to achieve something sim= ilar. > > > Is it buggy when you need one here? > > >=20 > >=20 > > I tried the code without bh and it doesn't work. >=20 > It's long ago tbat I wrote that comment, but the remark about > multiwrite_cb() concerns me. Do you know _why_ it doesn't work without > the BH, and whether the same problem affects multiwrite_cb()? I'd prefe= r > if we understood what we're doing over just basing the code on > experiments. Tried to do the conversion again. It seems to works fine. Best regards Beno=EEt >=20 > Kevin