Re: [Qemu-devel] [PATCH] rbd block driver fix race between aio completition and aio cancel

[Stefan Priebe - Profihost AG <s.priebe@profihost.ag>]

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?
>