Re: [Qemu-devel] [PATCH] block: Clean up after deleting BHs

[Filip Navara]

On Tue, Jun 23, 2009 at 6:57 PM, Avi Kivity<address@hidden> wrote:
> On 06/23/2009 07:50 PM, Christoph Hellwig wrote:
>>
>> On Tue, Jun 23, 2009 at 04:20:36PM +0300, Avi Kivity wrote:
>>
>>>
>>> Commit 6a7ad299 ("Call qemu_bh_delete at bdrv_aio_bh_cb") deletes
>>> emulated
>>> aio bottom halves to prevent endless accumulation.  However, it leaves a
>>> stale ->bh pointer, which is then waited on when the aio is reused.
>>>
>>> Zeroing the pointer fixes the issue, allowing vmdk format images to be
>>> used.
>>>
>>
>> What operations on vmdk images does this cause to fail?  qemu-iotests
>> seems to do fine on vmdk so it's nothing yet exercised by it.
>>
>
> Just starting qemu with a vmdk image hangs.  I think the very first read
> triggers it.

Actually I think it's the second read ;-)

>
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -1374,6 +1374,7 @@ static void bdrv_aio_cancel_em(BlockDriverAIOCB
>>> *blockacb)
>>>  {
>>>      BlockDriverAIOCBSync *acb = (BlockDriverAIOCBSync *)blockacb;
>>>      qemu_bh_delete(acb->bh);
>>> +    acb->bh = NULL;
>>>      qemu_aio_release(acb);
>>>  }
>>>
>>> @@ -1391,6 +1392,7 @@ static void bdrv_aio_bh_cb(void *opaque)
>>>      qemu_vfree(acb->bounce);
>>>      acb->common.cb(acb->common.opaque, acb->ret);
>>>      qemu_bh_delete(acb->bh);
>>> +    acb->bh = NULL;
>>>      qemu_aio_release(acb);
>>>  }
>>>
>>
>> I think not having the state of the private acb area cleared over a
>> free/realloc cycle is pretty dangerous.  Wouldn't it be better to always
>> clear that space in qemu_aio_get?
>>
>
> Maybe, but that's a bigger change.  Let's start with this (in stable- too)
> and rework aio later.
>

Agreed, let's get this in, the win32 builds are seriously affected by
the bug due to the absence of AIO on the platform.

Best regards,
Filip Navara