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

[Max Reitz]

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