From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:44471)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1Tb6Ho-0000fm-A2
	for qemu-devel@nongnu.org; Wed, 21 Nov 2012 04:08:05 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1Tb6Hi-0002Ld-I0
	for qemu-devel@nongnu.org; Wed, 21 Nov 2012 04:08:00 -0500
Received: from mail-ee0-f45.google.com ([74.125.83.45]:44065)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1Tb6Hi-0002LO-C2
	for qemu-devel@nongnu.org; Wed, 21 Nov 2012 04:07:54 -0500
Received: by mail-ee0-f45.google.com with SMTP id d49so4076705eek.4
	for <qemu-devel@nongnu.org>; Wed, 21 Nov 2012 01:07:53 -0800 (PST)
Date: Wed, 21 Nov 2012 10:07:51 +0100
From: Stefan Hajnoczi <stefanha@gmail.com>
Message-ID: <20121121090751.GC17466@stefanha-thinkpad.redhat.com>
References: <1353357585-31746-1-git-send-email-s.priebe@profihost.ag>
	<1353357585-31746-2-git-send-email-s.priebe@profihost.ag>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1353357585-31746-2-git-send-email-s.priebe@profihost.ag>
Subject: Re: [Qemu-devel] [PATCH] rbd block driver fix race between aio
 completition and aio cancel
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Stefan Priebe <s.priebe@profihost.ag>
Cc: qemu-devel@nongnu.org, Stefan Priebe <s.priebe@profhost.ag>

On Mon, Nov 19, 2012 at 09:39:45PM +0100, Stefan Priebe wrote:
> @@ -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;
>      }

When is acb->bh != NULL here?

>  
> @@ -406,9 +404,12 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
>              acb->ret = r;
>          }
>      }
> +    acb->status = acb->ret;

How about initializing acb->ret with -EINPROGRESS and using it instead
of adding a new status field?

> @@ -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();
> +    }
>  }

Need to take care that acb is still valid (not yet released!) when the
while loop iterates.

One way of doing this is to mark the acb as cancelled so the completion
handler won't release it.  Then the cancellation function can release
the acb - it's the last piece of code that needs a reference to acb.  In
this case the acb->cancelled field is useful.

> @@ -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));
> +
>      qemu_aio_release(acb);
>  }
>  

What does this hunk do?