Re: [Qemu-devel] [Qemu-block] [PATCH v3 02/16] block: BDS deletion in bdrv_do_drained_begin()

[Jeff Cody <jcody@redhat.com>]

On Wed, Feb 28, 2018 at 07:04:53PM +0100, Max Reitz wrote:
> Draining a BDS (in the main loop) may cause it to go be deleted.  That
> is rather suboptimal if we still plan to access it afterwards, so let us
> enclose the main body of the function with a bdrv_ref()/bdrv_unref()
> pair.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/io.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/block/io.c b/block/io.c
> index ead12c4136..3b61e26114 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -294,12 +294,27 @@ void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
>                             BdrvChild *parent)
>  {
>      BdrvChild *child, *next;
> +    bool in_main_loop =
> +        qemu_get_current_aio_context() == qemu_get_aio_context();
> +    /* bdrv_close() invokes bdrv_drain() with bs->refcnt == 0; then,
> +     * we may not invoke bdrv_ref()/bdrv_unref() because the latter
> +     * would result in the refcount going back to 0, creating an
> +     * infinite loop.
> +     * Also, we have to be in the main loop because we may not call
> +     * bdrv_unref() elsewhere.  But because of that, the BDS is not in
> +     * danger of going away without the bdrv_ref()/bdrv_unref() pair
> +     * elsewhere, so we are fine then. */
> +    bool add_ref = in_main_loop && bs->refcnt > 0;
>  
>      if (qemu_in_coroutine()) {
>          bdrv_co_yield_to_drain(bs, true, recursive, parent);
>          return;
>      }
>  
> +    if (add_ref) {
> +        bdrv_ref(bs);
> +    }
> +
>      /* Stop things in parent-to-child order */
>      if (atomic_fetch_inc(&bs->quiesce_counter) == 0) {
>          aio_disable_external(bdrv_get_aio_context(bs));
> @@ -315,6 +330,10 @@ void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
>              bdrv_do_drained_begin(child->bs, true, child);
>          }
>      }
> +
> +    if (add_ref) {
> +        bdrv_unref(bs);
> +    }
>  }
>  
>  void bdrv_drained_begin(BlockDriverState *bs)
> -- 
> 2.14.3
> 
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>