Re: [Qemu-block] [PATCH] blockdev: acquire aio_context for bitmap add/remove

[John Snow]

On 2/12/19 2:36 PM, Eric Blake wrote:
> On 2/6/19 11:02 AM, John Snow wrote:
>> When bitmaps are persistent, they may incur a disk read or write when bitmaps
>> are added or removed. For configurations like virtio-dataplane, failing to
>> acquire this lock will abort QEMU when disk IO occurs.
>>
>> We used to acquire aio_context as part of the bitmap lookup, so re-introduce
>> the lock for just the cases that have an IO penalty.
>>
>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1672010
>> Reported-By: Aihua Liang <address@hidden>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>  blockdev.c | 24 +++++++++++++++++++-----
>>  1 file changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index fb18e9c975..ce458de037 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2820,6 +2820,7 @@ void qmp_block_dirty_bitmap_add(const char *node, 
>> const char *name,
>>  {
>>      BlockDriverState *bs;
>>      BdrvDirtyBitmap *bitmap;
>> +    AioContext *aio_context = NULL;
>>  
>>      if (!name || name[0] == '\0') {
>>          error_setg(errp, "Bitmap name cannot be empty");
>> @@ -2854,10 +2855,12 @@ void qmp_block_dirty_bitmap_add(const char *node, 
>> const char *name,
>>          disabled = false;
>>      }
>>  
>> -    if (persistent &&
>> -        !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
>> -    {
>> -        return;
>> +    if (persistent) {
>> +        aio_context = bdrv_get_aio_context(bs);
>> +        aio_context_acquire(aio_context);
>> +        if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) {
>> +            goto out;
>> +        }
>>      }
>>  
>>      bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
>     if (bitmap == NULL) {
>         return;
>     }
> 
> Oops - this early return fails to release the aio context.

Good spot, thanks! This would be awful to diagnose later.