[Qemu-devel] [PATCH v3 0/2] block: Fix hang with mirroring qcow2

[Qemu-devel] [PATCH v3 0/2] block: Fix hang with mirroring qcow2

[Fam Zheng]

See patch 1 for the bug analysis.

v3: Make bdrv_co_drain a public function and use it directly in block/mirror.c.
    [Stefan]



Fam Zheng (2):
  block: Fix bdrv_drain in coroutine
  mirror: Replace bdrv_drain(bs) with bdrv_co_drain(bs)

 block/io.c            | 45 +++++++++++++++++++++++++++++++++++++++++++++
 block/mirror.c        |  2 +-
 include/block/block.h |  1 +
 3 files changed, 47 insertions(+), 1 deletion(-)

-- 
2.7.4

[Qemu-devel] [PATCH v3 1/2] block: Fix bdrv_drain in coroutine

[Fam Zheng]

Using the nested aio_poll() in coroutine is a bad idea. This patch
replaces the aio_poll loop in bdrv_drain with a BH, if called in
coroutine.

For example, the bdrv_drain() in mirror.c can hang when a guest issued
request is pending on it in qemu_co_mutex_lock().

Mirror coroutine in this case has just finished a request, and the block
job is about to complete. It calls bdrv_drain() which waits for the
other coroutine to complete. The other coroutine is a scsi-disk request.
The deadlock happens when the latter is in turn pending on the former to
yield/terminate, in qemu_co_mutex_lock(). The state flow is as below
(assuming a qcow2 image):

  mirror coroutine               scsi-disk coroutine
  -------------------------------------------------------------
  do last write

    qcow2:qemu_co_mutex_lock()
    ...
                                 scsi disk read

                                   tracked request begin

                                   qcow2:qemu_co_mutex_lock.enter

    qcow2:qemu_co_mutex_unlock()

  bdrv_drain
    while (has tracked request)
      aio_poll()

In the scsi-disk coroutine, the qemu_co_mutex_lock() will never return
because the mirror coroutine is blocked in the aio_poll(blocking=true).

With this patch, the added qemu_coroutine_yield() allows the scsi-disk
coroutine to make progress as expected:

  mirror coroutine               scsi-disk coroutine
  -------------------------------------------------------------
  do last write

    qcow2:qemu_co_mutex_lock()
    ...
                                 scsi disk read

                                   tracked request begin

                                   qcow2:qemu_co_mutex_lock.enter

    qcow2:qemu_co_mutex_unlock()

  bdrv_drain.enter
>   schedule BH
>   qemu_coroutine_yield()
>                                  qcow2:qemu_co_mutex_lock.return
>                                  ...
                                   tracked request end
    ...
    (resumed from BH callback)
  bdrv_drain.return
  ...

Reported-by: Laurent Vivier <lvivier@redhat.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c            | 45 +++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  1 +
 2 files changed, 46 insertions(+)

diff --git a/block/io.c b/block/io.c
index c4869b9..a7dbf85 100644
--- a/block/io.c
+++ b/block/io.c
@@ -253,6 +253,47 @@ static void bdrv_drain_recurse(BlockDriverState *bs)
     }
 }
 
+typedef struct {
+    Coroutine *co;
+    BlockDriverState *bs;
+    QEMUBH *bh;
+    bool done;
+} BdrvCoDrainData;
+
+static void bdrv_co_drain_bh_cb(void *opaque)
+{
+    BdrvCoDrainData *data = opaque;
+    Coroutine *co = data->co;
+
+    qemu_bh_delete(data->bh);
+    bdrv_drain(data->bs);
+    data->done = true;
+    qemu_coroutine_enter(co, NULL);
+}
+
+void coroutine_fn bdrv_co_drain(BlockDriverState *bs)
+{
+    BdrvCoDrainData data;
+
+    /* Calling bdrv_drain() from a BH ensures the current coroutine yields and
+     * other coroutines run if they were queued from
+     * qemu_co_queue_run_restart(). */
+
+    assert(qemu_in_coroutine());
+    data = (BdrvCoDrainData) {
+        .co = qemu_coroutine_self(),
+        .bs = bs,
+        .done = false,
+        .bh = aio_bh_new(bdrv_get_aio_context(bs), bdrv_co_drain_bh_cb, &data),
+    };
+    qemu_bh_schedule(data.bh);
+
+    qemu_coroutine_yield();
+    /* If we are resumed from some other event (such as an aio completion or a
+     * timer callback), it is a bug in the caller that should be fixed. */
+    assert(data.done);
+}
+
 /*
  * Wait for pending requests to complete on a single BlockDriverState subtree,
  * and suspend block driver's internal I/O until next request arrives.
@@ -269,6 +310,10 @@ void bdrv_drain(BlockDriverState *bs)
     bool busy = true;
 
     bdrv_drain_recurse(bs);
+    if (qemu_in_coroutine()) {
+        bdrv_co_drain(bs);
+        return;
+    }
     while (busy) {
         /* Keep iterating */
          bdrv_flush_io_queue(bs);
diff --git a/include/block/block.h b/include/block/block.h
index 6a39f94..3a73137 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -372,6 +372,7 @@ int bdrv_flush(BlockDriverState *bs);
 int coroutine_fn bdrv_co_flush(BlockDriverState *bs);
 void bdrv_close_all(void);
 void bdrv_drain(BlockDriverState *bs);
+void coroutine_fn bdrv_co_drain(BlockDriverState *bs);
 void bdrv_drain_all(void);
 
 int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
-- 
2.7.4

[Qemu-devel] [PATCH v3 2/2] mirror: Replace bdrv_drain(bs) with bdrv_co_drain(bs)

[Fam Zheng]

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index f64db1a..c2cfc1a 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -650,7 +650,7 @@ static void coroutine_fn mirror_run(void *opaque)
              * mirror_populate runs.
              */
             trace_mirror_before_drain(s, cnt);
-            bdrv_drain(bs);
+            bdrv_co_drain(bs);
             cnt = bdrv_get_dirty_count(s->dirty_bitmap);
         }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3 0/2] block: Fix hang with mirroring qcow2

[Paolo Bonzini]

On 05/04/2016 13:20, Fam Zheng wrote:
> See patch 1 for the bug analysis.
> 
> v3: Make bdrv_co_drain a public function and use it directly in block/mirror.c.
>     [Stefan]

FWIW, no need to send a v4 with bdrv_co_drained_begin.

Paolo

> 
> 
> Fam Zheng (2):
>   block: Fix bdrv_drain in coroutine
>   mirror: Replace bdrv_drain(bs) with bdrv_co_drain(bs)
> 
>  block/io.c            | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  block/mirror.c        |  2 +-
>  include/block/block.h |  1 +
>  3 files changed, 47 insertions(+), 1 deletion(-)
> 

Re: [Qemu-devel] [PATCH v3 0/2] block: Fix hang with mirroring qcow2

[Fam Zheng]

On Tue, 04/05 19:20, Fam Zheng wrote:
> See patch 1 for the bug analysis.
> 
> v3: Make bdrv_co_drain a public function and use it directly in block/mirror.c.
>     [Stefan]

Stefan, are you happy with this version for 2.6?

Fam

> 
> 
> 
> Fam Zheng (2):
>   block: Fix bdrv_drain in coroutine
>   mirror: Replace bdrv_drain(bs) with bdrv_co_drain(bs)
> 
>  block/io.c            | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  block/mirror.c        |  2 +-
>  include/block/block.h |  1 +
>  3 files changed, 47 insertions(+), 1 deletion(-)
> 
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [Qemu-block] [PATCH v3 0/2] block: Fix hang with mirroring qcow2

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 653 bytes --]

On Tue, Apr 05, 2016 at 07:20:51PM +0800, Fam Zheng wrote:
> See patch 1 for the bug analysis.
> 
> v3: Make bdrv_co_drain a public function and use it directly in block/mirror.c.
>     [Stefan]
> 
> 
> 
> Fam Zheng (2):
>   block: Fix bdrv_drain in coroutine
>   mirror: Replace bdrv_drain(bs) with bdrv_co_drain(bs)
> 
>  block/io.c            | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  block/mirror.c        |  2 +-
>  include/block/block.h |  1 +
>  3 files changed, 47 insertions(+), 1 deletion(-)
> 
> -- 
> 2.7.4
> 
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 0/2] block: Fix hang with mirroring qcow2

[Paolo Bonzini]

On 08/04/2016 12:06, Stefan Hajnoczi wrote:
> On Tue, Apr 05, 2016 at 07:20:51PM +0800, Fam Zheng wrote:
>> See patch 1 for the bug analysis.
>>
>> v3: Make bdrv_co_drain a public function and use it directly in block/mirror.c.
>>     [Stefan]
>>
>>
>>
>> Fam Zheng (2):
>>   block: Fix bdrv_drain in coroutine
>>   mirror: Replace bdrv_drain(bs) with bdrv_co_drain(bs)
>>
>>  block/io.c            | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>  block/mirror.c        |  2 +-
>>  include/block/block.h |  1 +
>>  3 files changed, 47 insertions(+), 1 deletion(-)
>>
>> -- 
>> 2.7.4
> 
> Thanks, applied to my block tree:
> https://github.com/stefanha/qemu/commits/block

FWIW, I've now rebased my bdrv_drain patches on top of this, and the
need for the extra bottom half is gone!  This is a much better solution
for that other problem with mirror.c.

Paolo