* [Qemu-devel] [PATCH] blockdev: acquire aio_context for bitmap add/remove
@ 2019-02-06 17:02 John Snow
2019-02-06 17:19 ` Vladimir Sementsov-Ogievskiy
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: John Snow @ 2019-02-06 17:02 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Max Reitz, Kevin Wolf, Markus Armbruster, eblake, vsementsov, John Snow
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 <aliang@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
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);
@@ -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);
+ }
}
/**
--
2.17.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] blockdev: acquire aio_context for bitmap add/remove
2019-02-06 17:02 [Qemu-devel] [PATCH] blockdev: acquire aio_context for bitmap add/remove John Snow
@ 2019-02-06 17:19 ` Vladimir Sementsov-Ogievskiy
2019-02-11 20:25 ` John Snow
2019-02-12 19:36 ` Eric Blake
2 siblings, 0 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-02-06 17:19 UTC (permalink / raw)
To: John Snow, qemu-devel, qemu-block
Cc: Max Reitz, Kevin Wolf, Markus Armbruster, eblake, Paolo Bonzini
add Paolo
06.02.2019 20:02, 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 <aliang@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] blockdev: acquire aio_context for bitmap add/remove
2019-02-06 17:02 [Qemu-devel] [PATCH] blockdev: acquire aio_context for bitmap add/remove John Snow
2019-02-06 17:19 ` Vladimir Sementsov-Ogievskiy
@ 2019-02-11 20:25 ` John Snow
2019-02-12 19:36 ` Eric Blake
2 siblings, 0 replies; 5+ messages in thread
From: John Snow @ 2019-02-11 20:25 UTC (permalink / raw)
To: qemu-devel, qemu-block, Kevin Wolf
Cc: vsementsov, Markus Armbruster, Max Reitz
On 2/6/19 12:02 PM, 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 <aliang@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> 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);
> @@ -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);
> + }
> }
>
> /**
>
If this looks noncontroversial to everyone (Paolo said he agreed with
Kevin's advice on the general approach here) I'm keen to merge it this week.
May I take it through my tree, or should I be patient and wait for you,
Kevin?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] blockdev: acquire aio_context for bitmap add/remove
2019-02-06 17:02 [Qemu-devel] [PATCH] blockdev: acquire aio_context for bitmap add/remove John Snow
2019-02-06 17:19 ` Vladimir Sementsov-Ogievskiy
2019-02-11 20:25 ` John Snow
@ 2019-02-12 19:36 ` Eric Blake
2019-02-12 19:43 ` John Snow
2 siblings, 1 reply; 5+ messages in thread
From: Eric Blake @ 2019-02-12 19:36 UTC (permalink / raw)
To: John Snow, qemu-devel, qemu-block
Cc: Max Reitz, Kevin Wolf, Markus Armbruster, vsementsov
[-- Attachment #1: Type: text/plain, Size: 3295 bytes --]
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 <aliang@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> 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.
> @@ -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);
> + }
> }
>
> /**
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] blockdev: acquire aio_context for bitmap add/remove
2019-02-12 19:36 ` Eric Blake
@ 2019-02-12 19:43 ` John Snow
0 siblings, 0 replies; 5+ messages in thread
From: John Snow @ 2019-02-12 19:43 UTC (permalink / raw)
To: Eric Blake, qemu-devel, qemu-block
Cc: Max Reitz, Kevin Wolf, Markus Armbruster, vsementsov
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 <aliang@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-02-12 19:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-06 17:02 [Qemu-devel] [PATCH] blockdev: acquire aio_context for bitmap add/remove John Snow
2019-02-06 17:19 ` Vladimir Sementsov-Ogievskiy
2019-02-11 20:25 ` John Snow
2019-02-12 19:36 ` Eric Blake
2019-02-12 19:43 ` John Snow
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.