[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH for-2.11] block: Keep strong reference when drai
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH for-2.11] block: Keep strong reference when draining all BDS |
Date: |
Fri, 10 Nov 2017 16:26:49 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 |
On 2017-11-10 10:19, Stefan Hajnoczi wrote:
> 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().
These don't. It's BDRV_POLL_WHILE() in bdrv_drain_recurse(), as written
in the commit message.
You cannot add a bdrv_ref()/bdrv_unref() pair there because
bdrv_drain_recurse() is called from bdrv_close() with a refcount of 0,
so having a bdrv_unref() there would cause an infinite recursion.
I think it's reasonable to expect callers of any bdrv_* function (except
for bdrv_unref()) to make sure that the BDS isn't deleted over the
course of that function. Therefore, I think that the actual issue is
here and we need to make sure here that we have a strong reference
before invoking bdrv_drain_recurse().
Max
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-block] [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, (continued)
- Re: [Qemu-block] [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Fam Zheng, 2017/11/09
- Re: [Qemu-block] [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Kevin Wolf, 2017/11/10
- Re: [Qemu-block] [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Fam Zheng, 2017/11/10
- Re: [Qemu-block] [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Max Reitz, 2017/11/10
- Re: [Qemu-block] [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Fam Zheng, 2017/11/10
- Re: [Qemu-block] [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Kevin Wolf, 2017/11/10
- Re: [Qemu-block] [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Max Reitz, 2017/11/10
- Re: [Qemu-block] [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Kevin Wolf, 2017/11/10
- Re: [Qemu-block] [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Max Reitz, 2017/11/10
Re: [Qemu-block] [PATCH for-2.11] block: Keep strong reference when draining all BDS, Stefan Hajnoczi, 2017/11/10
- Re: [Qemu-block] [PATCH for-2.11] block: Keep strong reference when draining all BDS,
Max Reitz <=