[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when drai
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS |
Date: |
Fri, 10 Nov 2017 10:45:57 +0800 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
On Thu, 11/09 21:43, 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);
> }
>
> void bdrv_drain_all_end(void)
> {
> BlockDriverState *bs;
> BdrvNextIterator it;
> + GSList *bs_list = NULL, *bs_list_entry;
> +
> + /* Must be called from the main loop */
> + assert(qemu_get_current_aio_context() == qemu_get_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.
> + */
> for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
> - AioContext *aio_context = bdrv_get_aio_context(bs);
> + bdrv_ref(bs);
> + bs_list = g_slist_prepend(bs_list, bs);
> + }
> +
> + for (bs_list_entry = bs_list; bs_list_entry;
> + bs_list_entry = bs_list_entry->next)
> + {
> + AioContext *aio_context;
> +
> + bs = bs_list_entry->data;
> + aio_context = bdrv_get_aio_context(bs);
>
> aio_context_acquire(aio_context);
> aio_enable_external(aio_context);
> bdrv_parent_drained_end(bs);
> bdrv_drain_recurse(bs, false);
> aio_context_release(aio_context);
> +
> + bdrv_unref(bs);
> }
>
> + g_slist_free(bs_list);
> +
> block_job_resume_all();
> }
>
> --
> 2.13.6
>
>
It is better to put the references into BdrvNextIterator and introduce
bdrv_next_iterator_destroy() to free them? You'll need to touch all callers
because it is not C++, but it secures all of rest, which seems vulnerable in the
same pattern, for example the aio_poll() in iothread_stop_all().
Fam
- [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Max Reitz, 2017/11/09
- Re: [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Eric Blake, 2017/11/09
- Re: [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS,
Fam Zheng <=
- Re: [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Kevin Wolf, 2017/11/10
- Re: [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Fam Zheng, 2017/11/10
- Re: [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Max Reitz, 2017/11/10
- Re: [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Fam Zheng, 2017/11/10
- Re: [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Kevin Wolf, 2017/11/10
- Re: [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Max Reitz, 2017/11/10
- Re: [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Kevin Wolf, 2017/11/10
- Re: [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Max Reitz, 2017/11/10
Re: [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Stefan Hajnoczi, 2017/11/10