Re: [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS

[Stefan Hajnoczi]

On Thu, Nov 09, 2017 at 09:43:15PM +0100, Max Reitz wrote:
> Draining a BDS may lead to graph modifications, which in turn may result
> in it and other BDS being stripped of their current references.  If
> bdrv_drain_all_begin() and bdrv_drain_all_end() do not keep strong
> references themselves, the BDS they are trying to drain (or undrain) may
> disappear right under their feet -- or, more specifically, under the
> feet of BDRV_POLL_WHILE() in bdrv_drain_recurse().
> 
> This fixes an occasional hang of iotest 194.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block/io.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 3d5ef2cabe..a0a2833e8e 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -340,7 +340,10 @@ void bdrv_drain_all_begin(void)
>      bool waited = true;
>      BlockDriverState *bs;
>      BdrvNextIterator it;
> -    GSList *aio_ctxs = NULL, *ctx;
> +    GSList *aio_ctxs = NULL, *ctx, *bs_list = NULL, *bs_list_entry;
> +
> +    /* Must be called from the main loop */
> +    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
>  
>      block_job_pause_all();
>  
> @@ -355,6 +358,12 @@ void bdrv_drain_all_begin(void)
>          if (!g_slist_find(aio_ctxs, aio_context)) {
>              aio_ctxs = g_slist_prepend(aio_ctxs, aio_context);
>          }
> +
> +        /* Keep a strong reference to all root BDS and copy them into
> +         * an own list because draining them may lead to graph
> +         * modifications. */
> +        bdrv_ref(bs);
> +        bs_list = g_slist_prepend(bs_list, bs);
>      }
>  
>      /* Note that completion of an asynchronous I/O operation can trigger any
> @@ -370,7 +379,11 @@ void bdrv_drain_all_begin(void)
>              AioContext *aio_context = ctx->data;
>  
>              aio_context_acquire(aio_context);
> -            for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
> +            for (bs_list_entry = bs_list; bs_list_entry;
> +                 bs_list_entry = bs_list_entry->next)
> +            {
> +                bs = bs_list_entry->data;
> +
>                  if (aio_context == bdrv_get_aio_context(bs)) {
>                      waited |= bdrv_drain_recurse(bs, true);
>                  }
> @@ -379,24 +392,52 @@ void bdrv_drain_all_begin(void)
>          }
>      }
>  
> +    for (bs_list_entry = bs_list; bs_list_entry;
> +         bs_list_entry = bs_list_entry->next)
> +    {
> +        bdrv_unref(bs_list_entry->data);
> +    }
> +
>      g_slist_free(aio_ctxs);
> +    g_slist_free(bs_list);
>  }

Which specific parts of this function access bs without a reference?

I see bdrv_next() may do QTAILQ_NEXT(bs, monitor_list) after
bdrv_drain_recurse() has returned.

Anything else?

If bdrv_next() is the only issue then I agree with Fam that it makes
sense to build the ref/unref into bdrv_next().

signature.asc
Description: PGP signature