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 <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().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
