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

[Avi Kivity <avi@redhat.com>]

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.

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

-- 
error compiling committee.c: too many arguments to function