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 <mre...@redhat.com>
> ---
> 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
