From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:37199)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <s.priebe@profihost.ag>) id 1TbTbC-0002kP-87
	for qemu-devel@nongnu.org; Thu, 22 Nov 2012 05:01:39 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <s.priebe@profihost.ag>) id 1TbTb6-0003CE-Cc
	for qemu-devel@nongnu.org; Thu, 22 Nov 2012 05:01:34 -0500
Received: from mail.profihost.ag ([85.158.179.208]:54085)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <s.priebe@profihost.ag>) id 1TbTb6-0003Bx-12
	for qemu-devel@nongnu.org; Thu, 22 Nov 2012 05:01:28 -0500
Message-ID: <50ADF7F0.8030002@profihost.ag>
Date: Thu, 22 Nov 2012 11:01:20 +0100
From: Stefan Priebe - Profihost AG <s.priebe@profihost.ag>
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>
	<20121121090751.GC17466@stefanha-thinkpad.redhat.com>
In-Reply-To: <20121121090751.GC17466@stefanha-thinkpad.redhat.com>
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: <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 Hajnoczi <stefanha@gmail.com>
Cc: Josh Durgin <josh.durgin@inktank.com>, qemu-devel@nongnu.org, Paolo Bonzini <pbonzini@redhat.com>

Hello,

i've send a new patch which hopefully cares about all your comments.

[PATCH] rbd block driver fix race between aio completition and aio cancel

Greets
Stefan

Am 21.11.2012 10:07, schrieb Stefan Hajnoczi:
> 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?
>