From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53870) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tb66U-00032y-CA for qemu-devel@nongnu.org; Wed, 21 Nov 2012 03:56:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tb66M-0007Hg-9F for qemu-devel@nongnu.org; Wed, 21 Nov 2012 03:56:18 -0500 Received: from mail-pa0-f45.google.com ([209.85.220.45]:36611) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tb66M-0007HS-38 for qemu-devel@nongnu.org; Wed, 21 Nov 2012 03:56:10 -0500 Received: by mail-pa0-f45.google.com with SMTP id bg2so2341311pad.4 for ; Wed, 21 Nov 2012 00:56:08 -0800 (PST) Message-ID: <50AC9724.4000005@inktank.com> Date: Wed, 21 Nov 2012 00:56:04 -0800 From: Josh Durgin MIME-Version: 1.0 References: <1353357585-31746-1-git-send-email-s.priebe@profihost.ag> <1353357585-31746-2-git-send-email-s.priebe@profihost.ag> In-Reply-To: <1353357585-31746-2-git-send-email-s.priebe@profihost.ag> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] rbd block driver fix race between aio completition and aio cancel List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Priebe Cc: qemu-devel@nongnu.org On 11/19/2012 12:39 PM, Stefan Priebe wrote: > From: Stefan Priebe > > This one fixes a race qemu also had in iscsi block driver between > cancellation and io completition. > > qemu_rbd_aio_cancel was not synchronously waiting for the end of > the command. > > It also removes the useless cancelled flag and introduces instead > a status flag with EINPROGRESS like iscsi block driver. > > Signed-off-by: Stefan Priebe > --- > block/rbd.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/block/rbd.c b/block/rbd.c > index 5a0f79f..7b3bcbb 100644 > --- a/block/rbd.c > +++ b/block/rbd.c > @@ -76,7 +76,7 @@ typedef struct RBDAIOCB { > int64_t sector_num; > int error; > struct BDRVRBDState *s; > - int cancelled; > + int status; > } RBDAIOCB; > > typedef struct RADOSCB { > @@ -376,9 +376,7 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb) > RBDAIOCB *acb = rcb->acb; > int64_t r; > > - if (acb->cancelled) { > - qemu_vfree(acb->bounce); > - qemu_aio_release(acb); > + if (acb->bh) { > goto done; > } I don't think this is necessary at all anymore, since this callback will never be called more than once, and it's the only thing that will allocate acb->bh. Removing this block (and the done label) altogether should have the intended effect, and the bh scheduled will free/release things as usual. > > @@ -406,9 +404,12 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb) > acb->ret = r; > } > } > + acb->status = acb->ret; > + > /* Note that acb->bh can be NULL in case where the aio was cancelled */ > acb->bh = qemu_bh_new(rbd_aio_bh_cb, acb); > qemu_bh_schedule(acb->bh); > + > done: > g_free(rcb); > } > @@ -573,7 +574,10 @@ static void qemu_rbd_close(BlockDriverState *bs) > static void qemu_rbd_aio_cancel(BlockDriverAIOCB *blockacb) > { > RBDAIOCB *acb = (RBDAIOCB *) blockacb; > - acb->cancelled = 1; > + > + while (acb->status == -EINPROGRESS) { > + qemu_aio_wait(); > + } > } > > static AIOPool rbd_aio_pool = { > @@ -642,10 +646,11 @@ static void rbd_aio_bh_cb(void *opaque) > qemu_iovec_from_buf(acb->qiov, 0, acb->bounce, acb->qiov->size); > } > qemu_vfree(acb->bounce); > - acb->common.cb(acb->common.opaque, (acb->ret > 0 ? 0 : acb->ret)); > qemu_bh_delete(acb->bh); > acb->bh = NULL; > > + acb->common.cb(acb->common.opaque, (acb->ret > 0 ? 0 : acb->ret)); > + I'm not sure changing the order matters here, but maybe I'm missing something. > qemu_aio_release(acb); > } > > @@ -689,8 +694,8 @@ static BlockDriverAIOCB *rbd_start_aio(BlockDriverState *bs, > acb->ret = 0; > acb->error = 0; > acb->s = s; > - acb->cancelled = 0; > acb->bh = NULL; > + acb->status = -EINPROGRESS; > > if (cmd == RBD_AIO_WRITE) { > qemu_iovec_to_buf(acb->qiov, 0, acb->bounce, qiov->size); >