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

[John Snow]

On 5/31/19 3:01 PM, Vladimir Sementsov-Ogievskiy wrote:
> 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.
> 

Safe travels home!

(Why transactions for remove? Just convenience for batching actions?
I'll find out on Monday.)

Have a good weekend!

--js