From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47932) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WB3Fg-00076p-Cx for qemu-devel@nongnu.org; Wed, 05 Feb 2014 09:15:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WB3Fa-0004RO-3d for qemu-devel@nongnu.org; Wed, 05 Feb 2014 09:14:56 -0500 Received: from paradis.irqsave.net ([62.212.105.220]:49225) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WB3FZ-0004Qa-JP for qemu-devel@nongnu.org; Wed, 05 Feb 2014 09:14:50 -0500 Date: Wed, 5 Feb 2014 15:14:47 +0100 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140205141447.GE3066@irqsave.net> References: <1391464280-25627-1-git-send-email-benoit.canet@irqsave.net> <1391464280-25627-4-git-send-email-benoit.canet@irqsave.net> <20140204135722.GJ3384@dhcp-200-207.str.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140204135722.GJ3384@dhcp-200-207.str.redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH V15 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, mreitz@redhat.com Le Tuesday 04 Feb 2014 =E0 14:57:22 (+0100), Kevin Wolf a =E9crit : > Am 03.02.2014 um 22:51 hat Beno=EEt Canet geschrieben: > > From: Beno=EEt Canet > >=20 > > Signed-off-by: Benoit Canet > > --- > > block/quorum.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++= ++++++++++ > > 1 file changed, 104 insertions(+) >=20 > Starting with writes before the driver can even open an image is a weir= d > order to do things in. It also doesn't make the review any easier when > you don't know how things are initialised. I have done it in this way for better git bisectability: if the driver ca= nnot open the quorum the commit is a preparation work that should be excluded = of git bisect commit range. For the write before read order it's this way because quorum writes are s= impler. Best regards Beno=EEt >=20 > > diff --git a/block/quorum.c b/block/quorum.c > > index 157efdf..81bffdd 100644 > > --- a/block/quorum.c > > +++ b/block/quorum.c > > @@ -64,11 +64,115 @@ struct QuorumAIOCB { > > int vote_ret; > > }; > > =20 > > +static void quorum_aio_cancel(BlockDriverAIOCB *blockacb) > > +{ > > + QuorumAIOCB *acb =3D container_of(blockacb, QuorumAIOCB, common)= ; > > + BDRVQuorumState *s =3D acb->bqs; > > + int i; > > + > > + /* cancel all callback */ >=20 > "callbacks" >=20 > > + for (i =3D 0; i < s->total; i++) { > > + bdrv_aio_cancel(acb->aios[i].aiocb); > > + } > > +} >=20 > Don't you want to free acb and similar cleanup? >=20 > > + > > +static AIOCBInfo quorum_aiocb_info =3D { > > + .aiocb_size =3D sizeof(QuorumAIOCB), > > + .cancel =3D quorum_aio_cancel, > > +}; > > + > > +static void quorum_aio_finalize(QuorumAIOCB *acb) > > +{ > > + BDRVQuorumState *s =3D acb->bqs; >=20 > block/quorum.c: In function 'quorum_aio_finalize': > block/quorum.c:86:22: error: unused variable 's' [-Werror=3Dunused-vari= able] >=20 > > + int ret =3D 0; > > + > > + acb->common.cb(acb->common.opaque, ret); > > + if (acb->finished) { > > + *acb->finished =3D true; > > + } > > + g_free(acb->aios); > > + qemu_aio_release(acb); > > +} > > + > > +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, op= aque); > > + int i; > > + > > + acb->bqs =3D s; >=20 > Noticed it only here, but it's really in patch 2 (and should be in > patch 1): >=20 > What is acb->bqs good for? Isn't it always the same as > acb->common.bs->opaque? >=20 > > + acb->sector_num =3D sector_num; > > + acb->nb_sectors =3D nb_sectors; > > + acb->qiov =3D qiov; > > + acb->aios =3D g_new0(QuorumSingleAIOCB, s->total); > > + acb->count =3D 0; > > + acb->success_count =3D 0; > > + acb->finished =3D NULL; > > + acb->is_read =3D false; > > + acb->vote_ret =3D 0; > > + > > + 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; > > + } > > > > + > > + return acb; > > +} >=20 > Kevin