diff mbox series

[v6,6/8] blockdev: Acquire AioContext on dirty bitmap functions

Message ID 20200108143138.129909-7-slp@redhat.com (mailing list archive)
State New, archived
Headers show
Series blockdev: Fix AioContext handling for various blockdev actions | expand

Commit Message

Sergio Lopez Jan. 8, 2020, 2:31 p.m. UTC 
Dirty map addition and removal functions are not acquiring to BDS
AioContext, while they may call to code that expects it to be
acquired.

This may trigger a crash with a stack trace like this one:

 #0  0x00007f0ef146370f in __GI_raise (sig=sig@entry=6)
     at ../sysdeps/unix/sysv/linux/raise.c:50
 #1  0x00007f0ef144db25 in __GI_abort () at abort.c:79
 #2  0x0000565022294dce in error_exit
     (err=<optimized out>, msg=msg@entry=0x56502243a730 <__func__.16350> "qemu_mutex_unlock_impl") at util/qemu-thread-posix.c:36
 #3  0x00005650222950ba in qemu_mutex_unlock_impl
     (mutex=mutex@entry=0x5650244b0240, file=file@entry=0x565022439adf "util/async.c", line=line@entry=526) at util/qemu-thread-posix.c:108
 #4  0x0000565022290029 in aio_context_release
     (ctx=ctx@entry=0x5650244b01e0) at util/async.c:526
 #5  0x000056502221cd08 in bdrv_can_store_new_dirty_bitmap
     (bs=bs@entry=0x5650244dc820, name=name@entry=0x56502481d360 "bitmap1", granularity=granularity@entry=65536, errp=errp@entry=0x7fff22831718)
     at block/dirty-bitmap.c:542
 #6  0x000056502206ae53 in qmp_block_dirty_bitmap_add
     (errp=0x7fff22831718, disabled=false, has_disabled=<optimized out>, persistent=<optimized out>, has_persistent=true, granularity=65536, has_granularity=<optimized out>, name=0x56502481d360 "bitmap1", node=<optimized out>) at blockdev.c:2894
 #7  0x000056502206ae53 in qmp_block_dirty_bitmap_add
     (node=<optimized out>, name=0x56502481d360 "bitmap1", has_granularity=<optimized out>, granularity=<optimized out>, has_persistent=true, persistent=<optimized out>, has_disabled=false, disabled=false, errp=0x7fff22831718) at blockdev.c:2856
 #8  0x00005650221847a3 in qmp_marshal_block_dirty_bitmap_add
     (args=<optimized out>, ret=<optimized out>, errp=0x7fff22831798)
     at qapi/qapi-commands-block-core.c:651
 #9  0x0000565022247e6c in do_qmp_dispatch
     (errp=0x7fff22831790, allow_oob=<optimized out>, request=<optimized out>, cmds=0x565022b32d60 <qmp_commands>) at qapi/qmp-dispatch.c:132
 #10 0x0000565022247e6c in qmp_dispatch
     (cmds=0x565022b32d60 <qmp_commands>, request=<optimized out>, allow_oob=<optimized out>) at qapi/qmp-dispatch.c:175
 #11 0x0000565022166061 in monitor_qmp_dispatch
     (mon=0x56502450faa0, req=<optimized out>) at monitor/qmp.c:145
 #12 0x00005650221666fa in monitor_qmp_bh_dispatcher
     (data=<optimized out>) at monitor/qmp.c:234
 #13 0x000056502228f866 in aio_bh_call (bh=0x56502440eae0)
     at util/async.c:117
 #14 0x000056502228f866 in aio_bh_poll (ctx=ctx@entry=0x56502440d7a0)
     at util/async.c:117
 #15 0x0000565022292c54 in aio_dispatch (ctx=0x56502440d7a0)
     at util/aio-posix.c:459
 #16 0x000056502228f742 in aio_ctx_dispatch
     (source=<optimized out>, callback=<optimized out>, user_data=<optimized out>) at util/async.c:260
 #17 0x00007f0ef5ce667d in g_main_dispatch (context=0x56502449aa40)
     at gmain.c:3176
 #18 0x00007f0ef5ce667d in g_main_context_dispatch
     (context=context@entry=0x56502449aa40) at gmain.c:3829
 #19 0x0000565022291d08 in glib_pollfds_poll () at util/main-loop.c:219
 #20 0x0000565022291d08 in os_host_main_loop_wait
     (timeout=<optimized out>) at util/main-loop.c:242
 #21 0x0000565022291d08 in main_loop_wait (nonblocking=<optimized out>)
     at util/main-loop.c:518
 #22 0x00005650220743c1 in main_loop () at vl.c:1828
 #23 0x0000565021f20a72 in main
     (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>)
     at vl.c:4504

Fix this by acquiring the AioContext at qmp_block_dirty_bitmap_add()
and qmp_block_dirty_bitmap_add().

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1782175
Signed-off-by: Sergio Lopez <slp@redhat.com>
---
 blockdev.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

Comments

Kevin Wolf Jan. 15, 2020, 3:19 p.m. UTC | #1 
Am 08.01.2020 um 15:31 hat Sergio Lopez geschrieben:
> Dirty map addition and removal functions are not acquiring to BDS
> AioContext, while they may call to code that expects it to be
> acquired.
> 
> This may trigger a crash with a stack trace like this one:
> 
>  #0  0x00007f0ef146370f in __GI_raise (sig=sig@entry=6)
>      at ../sysdeps/unix/sysv/linux/raise.c:50
>  #1  0x00007f0ef144db25 in __GI_abort () at abort.c:79
>  #2  0x0000565022294dce in error_exit
>      (err=<optimized out>, msg=msg@entry=0x56502243a730 <__func__.16350> "qemu_mutex_unlock_impl") at util/qemu-thread-posix.c:36
>  #3  0x00005650222950ba in qemu_mutex_unlock_impl
>      (mutex=mutex@entry=0x5650244b0240, file=file@entry=0x565022439adf "util/async.c", line=line@entry=526) at util/qemu-thread-posix.c:108
>  #4  0x0000565022290029 in aio_context_release
>      (ctx=ctx@entry=0x5650244b01e0) at util/async.c:526
>  #5  0x000056502221cd08 in bdrv_can_store_new_dirty_bitmap
>      (bs=bs@entry=0x5650244dc820, name=name@entry=0x56502481d360 "bitmap1", granularity=granularity@entry=65536, errp=errp@entry=0x7fff22831718)
>      at block/dirty-bitmap.c:542
>  #6  0x000056502206ae53 in qmp_block_dirty_bitmap_add
>      (errp=0x7fff22831718, disabled=false, has_disabled=<optimized out>, persistent=<optimized out>, has_persistent=true, granularity=65536, has_granularity=<optimized out>, name=0x56502481d360 "bitmap1", node=<optimized out>) at blockdev.c:2894
>  #7  0x000056502206ae53 in qmp_block_dirty_bitmap_add
>      (node=<optimized out>, name=0x56502481d360 "bitmap1", has_granularity=<optimized out>, granularity=<optimized out>, has_persistent=true, persistent=<optimized out>, has_disabled=false, disabled=false, errp=0x7fff22831718) at blockdev.c:2856
>  #8  0x00005650221847a3 in qmp_marshal_block_dirty_bitmap_add
>      (args=<optimized out>, ret=<optimized out>, errp=0x7fff22831798)
>      at qapi/qapi-commands-block-core.c:651
>  #9  0x0000565022247e6c in do_qmp_dispatch
>      (errp=0x7fff22831790, allow_oob=<optimized out>, request=<optimized out>, cmds=0x565022b32d60 <qmp_commands>) at qapi/qmp-dispatch.c:132
>  #10 0x0000565022247e6c in qmp_dispatch
>      (cmds=0x565022b32d60 <qmp_commands>, request=<optimized out>, allow_oob=<optimized out>) at qapi/qmp-dispatch.c:175
>  #11 0x0000565022166061 in monitor_qmp_dispatch
>      (mon=0x56502450faa0, req=<optimized out>) at monitor/qmp.c:145
>  #12 0x00005650221666fa in monitor_qmp_bh_dispatcher
>      (data=<optimized out>) at monitor/qmp.c:234
>  #13 0x000056502228f866 in aio_bh_call (bh=0x56502440eae0)
>      at util/async.c:117
>  #14 0x000056502228f866 in aio_bh_poll (ctx=ctx@entry=0x56502440d7a0)
>      at util/async.c:117
>  #15 0x0000565022292c54 in aio_dispatch (ctx=0x56502440d7a0)
>      at util/aio-posix.c:459
>  #16 0x000056502228f742 in aio_ctx_dispatch
>      (source=<optimized out>, callback=<optimized out>, user_data=<optimized out>) at util/async.c:260
>  #17 0x00007f0ef5ce667d in g_main_dispatch (context=0x56502449aa40)
>      at gmain.c:3176
>  #18 0x00007f0ef5ce667d in g_main_context_dispatch
>      (context=context@entry=0x56502449aa40) at gmain.c:3829
>  #19 0x0000565022291d08 in glib_pollfds_poll () at util/main-loop.c:219
>  #20 0x0000565022291d08 in os_host_main_loop_wait
>      (timeout=<optimized out>) at util/main-loop.c:242
>  #21 0x0000565022291d08 in main_loop_wait (nonblocking=<optimized out>)
>      at util/main-loop.c:518
>  #22 0x00005650220743c1 in main_loop () at vl.c:1828
>  #23 0x0000565021f20a72 in main
>      (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>)
>      at vl.c:4504
> 
> Fix this by acquiring the AioContext at qmp_block_dirty_bitmap_add()
> and qmp_block_dirty_bitmap_add().
> 
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1782175
> Signed-off-by: Sergio Lopez <slp@redhat.com>

> @@ -3038,20 +3045,26 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
>  {
>      BlockDriverState *bs;
>      BdrvDirtyBitmap *bitmap;
> +    AioContext *aio_context;
>  
>      bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
>      if (!bitmap || !bs) {
>          return NULL;
>      }
>  
> +    aio_context = bdrv_get_aio_context(bs);
> +    aio_context_acquire(aio_context);
> +
>      if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY | BDRV_BITMAP_RO,
>                                  errp)) {
> +        aio_context_release(aio_context);
>          return NULL;
>      }
>  
>      if (bdrv_dirty_bitmap_get_persistence(bitmap) &&
>          bdrv_remove_persistent_dirty_bitmap(bs, name, errp) < 0)
>      {
> +            aio_context_release(aio_context);
>              return NULL;
>      }

Indentation is off here. It was already off before this patch, but
rather than adding another incorrectly indented line, I think we should
just fix it. I can do that while applying.

Kevin
diff mbox series

Patch

diff --git a/blockdev.c b/blockdev.c
index 1dacbc20ec..292961a88d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2984,6 +2984,7 @@  void qmp_block_dirty_bitmap_add(const char *node, const char *name,
 {
     BlockDriverState *bs;
     BdrvDirtyBitmap *bitmap;
+    AioContext *aio_context;
 
     if (!name || name[0] == '\0') {
         error_setg(errp, "Bitmap name cannot be empty");
@@ -2995,11 +2996,14 @@  void qmp_block_dirty_bitmap_add(const char *node, const char *name,
         return;
     }
 
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
+
     if (has_granularity) {
         if (granularity < 512 || !is_power_of_2(granularity)) {
             error_setg(errp, "Granularity must be power of 2 "
                              "and at least 512");
-            return;
+            goto out;
         }
     } else {
         /* Default to cluster size, if available: */
@@ -3017,12 +3021,12 @@  void qmp_block_dirty_bitmap_add(const char *node, const char *name,
     if (persistent &&
         !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
     {
-        return;
+        goto out;
     }
 
     bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
     if (bitmap == NULL) {
-        return;
+        goto out;
     }
 
     if (disabled) {
@@ -3030,6 +3034,9 @@  void qmp_block_dirty_bitmap_add(const char *node, const char *name,
     }
 
     bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
+
+out:
+    aio_context_release(aio_context);
 }
 
 static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
@@ -3038,20 +3045,26 @@  static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
 {
     BlockDriverState *bs;
     BdrvDirtyBitmap *bitmap;
+    AioContext *aio_context;
 
     bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
     if (!bitmap || !bs) {
         return NULL;
     }
 
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
+
     if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY | BDRV_BITMAP_RO,
                                 errp)) {
+        aio_context_release(aio_context);
         return NULL;
     }
 
     if (bdrv_dirty_bitmap_get_persistence(bitmap) &&
         bdrv_remove_persistent_dirty_bitmap(bs, name, errp) < 0)
     {
+            aio_context_release(aio_context);
             return NULL;
     }
 
@@ -3063,6 +3076,7 @@  static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
         *bitmap_bs = bs;
     }
 
+    aio_context_release(aio_context);
     return release ? NULL : bitmap;
 }