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

2017-11-10 Thread 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 
>> ---
>>  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(); bs; bs = bdrv_next()) {
>> +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] [PATCH for-2.11] block: Keep strong reference when draining all BDS

2017-11-10 Thread Stefan Hajnoczi 
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 
> ---
>  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(); bs; bs = bdrv_next()) {
> +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

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

2017-11-09 Thread Max Reitz 
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 
---
 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(); bs; bs = bdrv_next()) {
+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(); bs; bs = bdrv_next()) {
-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