diff mbox series

[PULL,v2,3/3] blockdev: acquire aio_context for bitmap add/remove

Message ID 20190220180112.28250-4-jsnow@redhat.com
State New
Headers show
Series [PULL,v2,1/3] dirty-bitmap: Expose persistent flag to 'query-block' | expand

Commit Message

John Snow Feb. 20, 2019, 6:01 p.m. UTC 
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 <aliang@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-id: 20190218233154.19303-1-jsnow@redhat.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy May 31, 2019, 5:30 p.m. UTC | #1 
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 <aliang@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Message-id: 20190218233154.19303-1-jsnow@redhat.com
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---

[..]

>   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?
John Snow May 31, 2019, 6:16 p.m. UTC | #2 
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 <aliang@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Message-id: 20190218233154.19303-1-jsnow@redhat.com
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
> 
> [..]
> 
>>   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.
Vladimir Sementsov-Ogievskiy May 31, 2019, 7:01 p.m. UTC | #3 
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 <aliang@redhat.com>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> Message-id: 20190218233154.19303-1-jsnow@redhat.com
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>
>> [..]
>>
>>>    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.
John Snow May 31, 2019, 7:03 p.m. UTC | #4 
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 <aliang@redhat.com>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>> Message-id: 20190218233154.19303-1-jsnow@redhat.com
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>
>>> [..]
>>>
>>>>    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
diff mbox series

Patch

diff --git a/blockdev.c b/blockdev.c
index fb18e9c975..8714ad2702 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,15 +2855,17 @@  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;
+        goto out;
     }
 
     if (disabled) {
@@ -2870,6 +2873,10 @@  void qmp_block_dirty_bitmap_add(const char *node, const char *name,
     }
 
     bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+ out:
+    if (aio_context) {
+        aio_context_release(aio_context);
+    }
 }
 
 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);
+    }
 }
 
 /**