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

[Kevin Wolf]

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 <address@hidden>

> @@ -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