Re: [Qemu-devel] [PULL v2 3/3] blockdev: acquire aio_context for bitmap add/remove

[John Snow]

On 5/31/19 1:30 PM, Vladimir Sementsov-Ogievskiy wrote:
> Hi!
> 
> 20.02.2019 21:01, 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. Commit 2119882c removed
>> these locks, and I failed to notice this when we committed fd5ae4cc, so this
>> has been broken since persistent bitmaps were introduced.
>>
>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1672010
>> Reported-By: Aihua Liang <address@hidden>
>> Signed-off-by: John Snow <address@hidden>
>> Reviewed-by: Eric Blake <address@hidden>
>> Message-id: address@hidden
>> Signed-off-by: John Snow <address@hidden>
>> ---
> 
> [..]
> 
>>   void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>> @@ -2878,6 +2885,7 @@ void qmp_block_dirty_bitmap_remove(const char *node, 
>> const char *name,
>>       BlockDriverState *bs;
>>       BdrvDirtyBitmap *bitmap;
>>       Error *local_err = NULL;
>> +    AioContext *aio_context = NULL;
>>   
>>       bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
>>       if (!bitmap || !bs) {
>> @@ -2892,14 +2900,20 @@ void qmp_block_dirty_bitmap_remove(const char *node, 
>> const char *name,
>>       }
>>   
>>       if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
>> +        aio_context = bdrv_get_aio_context(bs);
>> +        aio_context_acquire(aio_context);
>>           bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
>>           if (local_err != NULL) {
>>               error_propagate(errp, local_err);
>> -            return;
>> +            goto out;
>>           }
>>       }
>>   
>>       bdrv_release_dirty_bitmap(bs, bitmap);
>> + out:
>> +    if (aio_context) {
>> +        aio_context_release(aio_context);
>> +    }
>>   }
>>   
>>   /**
>>
> 
> A bit late, but I have a question:
> 
> Why did you include bdrv_release_dirty_bitmap call into context-acquired 
> section? As I can
> understand from commit message, it's not actually needed?
> 

No reason beyond habit.