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

[Vladimir Sementsov-Ogievskiy]

31.05.2019 21:16, John Snow wrote:
> 
> 
> 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.
> 

Ok thanks. I'm now working (ok worked, I'd better go home now) on transaction 
action for bitmap-remove.
It occurs, that we need it to have an ability to _move_, not _copy_ bitmaps in 
transaction (we of course
can copy in transaction and then remove after transaction, but it doesn't work 
if bitmap is persistent and
after transaction node is RO). So, I have to refactor this code anyway, and 
therefore I've asked for this
thing which I want to refactor too.

-- 
Best regards,
Vladimir