| Message ID | 20190206170205.13061-1-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blockdev: acquire aio_context for bitmap add/remove | expand |
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>
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?
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); > + } > } > > /** >
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.
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); + } } /**
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(-)