[Qemu-devel] [PATCH RFC 0/2] Fix slow startup with many disks

[Qemu-devel] [PATCH RFC 0/2] Fix slow startup with many disks

[Alexander Yarygin]

During reset the aio_poll() function is called at least amount_of_disks^2 times:

for_each disk
    virtio_blk_reset()
        bdrv_drain_all()
                for_each disk
                         aio_poll()

For example, startup with 1000 disks takes over 13 minutes.

Patch 1/2 removes inner loop by using bdrv_drain() instead of bdrv_drain_all().
bdrv_drain() works on one disk at time.

Since bdrv_drain_all() is still called in other places, patch 2/2 optimizes
it for cases, where there are more disks than iothreads.

Thanks.

Alexander Yarygin (2):
  block-backend: Introduce blk_drain() and replace blk_drain_all()
  block: Let bdrv_drain_all() to call aio_poll() for each AioContext

 block/block-backend.c          |  5 +++++
 block/io.c                     | 42 ++++++++++++++++++++++++++----------------
 hw/block/virtio-blk.c          |  2 +-
 include/sysemu/block-backend.h |  1 +
 4 files changed, 33 insertions(+), 17 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/2] block-backend: Introduce blk_drain() and replace blk_drain_all()

[Alexander Yarygin]

Each call of the virtio_blk_reset() function calls blk_drain_all(),
which works for all existing BlockDriverStates, while only one
BlockDriverState needs to be drained.

This patch introduces the blk_drain() function and replaces
blk_drain_all() on it in virtio_blk_reset().

Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com>
---
 block/block-backend.c          | 5 +++++
 hw/block/virtio-blk.c          | 2 +-
 include/sysemu/block-backend.h | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 93e46f3..aee8a12 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -700,6 +700,11 @@ int blk_flush_all(void)
     return bdrv_flush_all();
 }
 
+void blk_drain(BlockBackend *blk)
+{
+    bdrv_drain(blk->bs);
+}
+
 void blk_drain_all(void)
 {
     bdrv_drain_all();
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e6afe97..abaca58 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -660,7 +660,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
      * This should cancel pending requests, but can't do nicely until there
      * are per-device request lists.
      */
-    blk_drain_all();
+    blk_drain(s->blk);
     blk_set_enable_write_cache(s->blk, s->original_wce);
 }
 
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index b4a4d5e..8fc960f 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -118,6 +118,7 @@ int blk_co_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors);
 int blk_co_flush(BlockBackend *blk);
 int blk_flush(BlockBackend *blk);
 int blk_flush_all(void);
+void blk_drain(BlockBackend *blk);
 void blk_drain_all(void);
 BlockdevOnError blk_get_on_error(BlockBackend *blk, bool is_read);
 BlockErrorAction blk_get_error_action(BlockBackend *blk, bool is_read,
-- 
1.9.1

[Qemu-devel] [PATCH 2/2] block: Let bdrv_drain_all() to call aio_poll() for each AioContext

[Alexander Yarygin]

After the commit 9b536adc ("block: acquire AioContext in
bdrv_drain_all()") the aio_poll() function got called for every
BlockDriverState, in assumption that every device may have its own
AioContext. If we have thousands of disks attached, there are a lot of
BlockDriverStates but only a few AioContexts, leading to tons of
unnecessary aio_poll() calls.

This patch changes the bdrv_drain_all() function allowing it find shared
AioContexts and to call aio_poll() only for unique ones.

Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com>
---
 block/io.c | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/block/io.c b/block/io.c
index e394d92..7502186 100644
--- a/block/io.c
+++ b/block/io.c
@@ -271,17 +271,6 @@ static bool bdrv_requests_pending(BlockDriverState *bs)
     return false;
 }
 
-static bool bdrv_drain_one(BlockDriverState *bs)
-{
-    bool bs_busy;
-
-    bdrv_flush_io_queue(bs);
-    bdrv_start_throttled_reqs(bs);
-    bs_busy = bdrv_requests_pending(bs);
-    bs_busy |= aio_poll(bdrv_get_aio_context(bs), bs_busy);
-    return bs_busy;
-}
-
 /*
  * Wait for pending requests to complete on a single BlockDriverState subtree
  *
@@ -294,8 +283,13 @@ static bool bdrv_drain_one(BlockDriverState *bs)
  */
 void bdrv_drain(BlockDriverState *bs)
 {
-    while (bdrv_drain_one(bs)) {
+    bool busy = true;
+
+    while (busy) {
         /* Keep iterating */
+         bdrv_flush_io_queue(bs);
+         busy = bdrv_requests_pending(bs);
+         busy |= aio_poll(bdrv_get_aio_context(bs), busy);
     }
 }
 
@@ -316,6 +310,7 @@ void bdrv_drain_all(void)
     /* Always run first iteration so any pending completion BHs run */
     bool busy = true;
     BlockDriverState *bs = NULL;
+    GSList *aio_ctxs = NULL, *ctx;
 
     while ((bs = bdrv_next(bs))) {
         AioContext *aio_context = bdrv_get_aio_context(bs);
@@ -325,17 +320,30 @@ void bdrv_drain_all(void)
             block_job_pause(bs->job);
         }
         aio_context_release(aio_context);
+
+        if (!aio_ctxs || !g_slist_find(aio_ctxs, aio_context)) {
+            aio_ctxs = g_slist_prepend(aio_ctxs, aio_context);
+        }
     }
 
     while (busy) {
         busy = false;
-        bs = NULL;
 
-        while ((bs = bdrv_next(bs))) {
-            AioContext *aio_context = bdrv_get_aio_context(bs);
+        for (ctx = aio_ctxs; ctx != NULL; ctx = ctx->next) {
+            AioContext *aio_context = ctx->data;
+            bs = NULL;
 
             aio_context_acquire(aio_context);
-            busy |= bdrv_drain_one(bs);
+            while ((bs = bdrv_next(bs))) {
+                if (aio_context == bdrv_get_aio_context(bs)) {
+                    bdrv_flush_io_queue(bs);
+                    if (bdrv_requests_pending(bs)) {
+                        busy = true;
+                        aio_poll(aio_context, busy);
+                    }
+                }
+            }
+            busy |= aio_poll(aio_context, false);
             aio_context_release(aio_context);
         }
     }
@@ -350,6 +358,7 @@ void bdrv_drain_all(void)
         }
         aio_context_release(aio_context);
     }
+    g_slist_free(aio_ctxs);
 }
 
 /**
@@ -2600,4 +2609,5 @@ void bdrv_flush_io_queue(BlockDriverState *bs)
     } else if (bs->file) {
         bdrv_flush_io_queue(bs->file);
     }
+    bdrv_start_throttled_reqs(bs);
 }
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/2] block-backend: Introduce blk_drain() and replace blk_drain_all()

[Paolo Bonzini]

On 03/06/2015 15:46, Alexander Yarygin wrote:
> Each call of the virtio_blk_reset() function calls blk_drain_all(),
> which works for all existing BlockDriverStates, while only one
> BlockDriverState needs to be drained.
> 
> This patch introduces the blk_drain() function and replaces
> blk_drain_all() on it in virtio_blk_reset().
> 
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com>
> ---
>  block/block-backend.c          | 5 +++++
>  hw/block/virtio-blk.c          | 2 +-
>  include/sysemu/block-backend.h | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 93e46f3..aee8a12 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -700,6 +700,11 @@ int blk_flush_all(void)
>      return bdrv_flush_all();
>  }
>  
> +void blk_drain(BlockBackend *blk)
> +{
> +    bdrv_drain(blk->bs);
> +}
> +
>  void blk_drain_all(void)
>  {
>      bdrv_drain_all();
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index e6afe97..abaca58 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -660,7 +660,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>       * This should cancel pending requests, but can't do nicely until there
>       * are per-device request lists.
>       */
> -    blk_drain_all();
> +    blk_drain(s->blk);
>      blk_set_enable_write_cache(s->blk, s->original_wce);
>  }
>  
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index b4a4d5e..8fc960f 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -118,6 +118,7 @@ int blk_co_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors);
>  int blk_co_flush(BlockBackend *blk);
>  int blk_flush(BlockBackend *blk);
>  int blk_flush_all(void);
> +void blk_drain(BlockBackend *blk);
>  void blk_drain_all(void);
>  BlockdevOnError blk_get_on_error(BlockBackend *blk, bool is_read);
>  BlockErrorAction blk_get_error_action(BlockBackend *blk, bool is_read,
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH 1/2] block-backend: Introduce blk_drain() and replace blk_drain_all()

[Christian Borntraeger]

Am 03.06.2015 um 15:46 schrieb Alexander Yarygin:
> Each call of the virtio_blk_reset() function calls blk_drain_all(),
> which works for all existing BlockDriverStates, while only one
> BlockDriverState needs to be drained.
> 
> This patch introduces the blk_drain() function and replaces
> blk_drain_all() on it in virtio_blk_reset().

> 
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com>

Seems to solve the main issue and causes no visible regression on a quick shot
with my testcase.

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

> ---
>  block/block-backend.c          | 5 +++++
>  hw/block/virtio-blk.c          | 2 +-
>  include/sysemu/block-backend.h | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 93e46f3..aee8a12 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -700,6 +700,11 @@ int blk_flush_all(void)
>      return bdrv_flush_all();
>  }
> 
> +void blk_drain(BlockBackend *blk)
> +{
> +    bdrv_drain(blk->bs);
> +}
> +
>  void blk_drain_all(void)
>  {
>      bdrv_drain_all();
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index e6afe97..abaca58 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -660,7 +660,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>       * This should cancel pending requests, but can't do nicely until there
>       * are per-device request lists.
>       */
> -    blk_drain_all();
> +    blk_drain(s->blk);
>      blk_set_enable_write_cache(s->blk, s->original_wce);
>  }
> 
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index b4a4d5e..8fc960f 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -118,6 +118,7 @@ int blk_co_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors);
>  int blk_co_flush(BlockBackend *blk);
>  int blk_flush(BlockBackend *blk);
>  int blk_flush_all(void);
> +void blk_drain(BlockBackend *blk);
>  void blk_drain_all(void);
>  BlockdevOnError blk_get_on_error(BlockBackend *blk, bool is_read);
>  BlockErrorAction blk_get_error_action(BlockBackend *blk, bool is_read,
> 

Re: [Qemu-devel] [PATCH 2/2] block: Let bdrv_drain_all() to call aio_poll() for each AioContext

[Christian Borntraeger]

Am 03.06.2015 um 15:46 schrieb Alexander Yarygin:
> After the commit 9b536adc ("block: acquire AioContext in
> bdrv_drain_all()") the aio_poll() function got called for every
> BlockDriverState, in assumption that every device may have its own
> AioContext. If we have thousands of disks attached, there are a lot of
> BlockDriverStates but only a few AioContexts, leading to tons of
> unnecessary aio_poll() calls.
> 
> This patch changes the bdrv_drain_all() function allowing it find shared
> AioContexts and to call aio_poll() only for unique ones.
> 
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com>

This seems to help in the case where we have one iothread and hundreds of
disks - maybe others. I dont understand yet why this patch works fine
and your previous patch caused hangs.
Its the same cluelessness on my side for the patch from Paolo that Stefan will revert.

Christian



> ---
>  block/io.c | 42 ++++++++++++++++++++++++++----------------
>  1 file changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index e394d92..7502186 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -271,17 +271,6 @@ static bool bdrv_requests_pending(BlockDriverState *bs)
>      return false;
>  }
> 
> -static bool bdrv_drain_one(BlockDriverState *bs)
> -{
> -    bool bs_busy;
> -
> -    bdrv_flush_io_queue(bs);
> -    bdrv_start_throttled_reqs(bs);
> -    bs_busy = bdrv_requests_pending(bs);
> -    bs_busy |= aio_poll(bdrv_get_aio_context(bs), bs_busy);
> -    return bs_busy;
> -}
> -
>  /*
>   * Wait for pending requests to complete on a single BlockDriverState subtree
>   *
> @@ -294,8 +283,13 @@ static bool bdrv_drain_one(BlockDriverState *bs)
>   */
>  void bdrv_drain(BlockDriverState *bs)
>  {
> -    while (bdrv_drain_one(bs)) {
> +    bool busy = true;
> +
> +    while (busy) {
>          /* Keep iterating */
> +         bdrv_flush_io_queue(bs);
> +         busy = bdrv_requests_pending(bs);
> +         busy |= aio_poll(bdrv_get_aio_context(bs), busy);
>      }
>  }
> 
> @@ -316,6 +310,7 @@ void bdrv_drain_all(void)
>      /* Always run first iteration so any pending completion BHs run */
>      bool busy = true;
>      BlockDriverState *bs = NULL;
> +    GSList *aio_ctxs = NULL, *ctx;
> 
>      while ((bs = bdrv_next(bs))) {
>          AioContext *aio_context = bdrv_get_aio_context(bs);
> @@ -325,17 +320,30 @@ void bdrv_drain_all(void)
>              block_job_pause(bs->job);
>          }
>          aio_context_release(aio_context);
> +
> +        if (!aio_ctxs || !g_slist_find(aio_ctxs, aio_context)) {
> +            aio_ctxs = g_slist_prepend(aio_ctxs, aio_context);
> +        }
>      }
> 
>      while (busy) {
>          busy = false;
> -        bs = NULL;
> 
> -        while ((bs = bdrv_next(bs))) {
> -            AioContext *aio_context = bdrv_get_aio_context(bs);
> +        for (ctx = aio_ctxs; ctx != NULL; ctx = ctx->next) {
> +            AioContext *aio_context = ctx->data;
> +            bs = NULL;
> 
>              aio_context_acquire(aio_context);
> -            busy |= bdrv_drain_one(bs);
> +            while ((bs = bdrv_next(bs))) {
> +                if (aio_context == bdrv_get_aio_context(bs)) {
> +                    bdrv_flush_io_queue(bs);
> +                    if (bdrv_requests_pending(bs)) {
> +                        busy = true;
> +                        aio_poll(aio_context, busy);
> +                    }
> +                }
> +            }
> +            busy |= aio_poll(aio_context, false);
>              aio_context_release(aio_context);
>          }
>      }
> @@ -350,6 +358,7 @@ void bdrv_drain_all(void)
>          }
>          aio_context_release(aio_context);
>      }
> +    g_slist_free(aio_ctxs);
>  }
> 
>  /**
> @@ -2600,4 +2609,5 @@ void bdrv_flush_io_queue(BlockDriverState *bs)
>      } else if (bs->file) {
>          bdrv_flush_io_queue(bs->file);
>      }
> +    bdrv_start_throttled_reqs(bs);
>  }
> 

Re: [Qemu-devel] [PATCH 1/2] block-backend: Introduce blk_drain() and replace blk_drain_all()

[Christian Borntraeger]

Am 03.06.2015 um 15:46 schrieb Alexander Yarygin:
> Each call of the virtio_blk_reset() function calls blk_drain_all(),
> which works for all existing BlockDriverStates, while only one
> BlockDriverState needs to be drained.
> 
> This patch introduces the blk_drain() function and replaces
> blk_drain_all() on it in virtio_blk_reset().
> 
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com>

Stefan/Kevin,

any opinion on these 2 patches? I would like to have this issue fixed
for s390 as having many disks is quite common on s390. 


> ---
>  block/block-backend.c          | 5 +++++
>  hw/block/virtio-blk.c          | 2 +-
>  include/sysemu/block-backend.h | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 93e46f3..aee8a12 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -700,6 +700,11 @@ int blk_flush_all(void)
>      return bdrv_flush_all();
>  }
> 
> +void blk_drain(BlockBackend *blk)
> +{
> +    bdrv_drain(blk->bs);
> +}
> +
>  void blk_drain_all(void)
>  {
>      bdrv_drain_all();
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index e6afe97..abaca58 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -660,7 +660,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>       * This should cancel pending requests, but can't do nicely until there
>       * are per-device request lists.
>       */
> -    blk_drain_all();
> +    blk_drain(s->blk);
>      blk_set_enable_write_cache(s->blk, s->original_wce);
>  }
> 
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index b4a4d5e..8fc960f 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -118,6 +118,7 @@ int blk_co_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors);
>  int blk_co_flush(BlockBackend *blk);
>  int blk_flush(BlockBackend *blk);
>  int blk_flush_all(void);
> +void blk_drain(BlockBackend *blk);
>  void blk_drain_all(void);
>  BlockdevOnError blk_get_on_error(BlockBackend *blk, bool is_read);
>  BlockErrorAction blk_get_error_action(BlockBackend *blk, bool is_read,
> 

Re: [Qemu-devel] [PATCH 1/2] block-backend: Introduce blk_drain() and replace blk_drain_all()

[Stefan Hajnoczi]

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

On Wed, Jun 03, 2015 at 04:46:14PM +0300, Alexander Yarygin wrote:
> Each call of the virtio_blk_reset() function calls blk_drain_all(),
> which works for all existing BlockDriverStates, while only one
> BlockDriverState needs to be drained.
> 
> This patch introduces the blk_drain() function and replaces
> blk_drain_all() on it in virtio_blk_reset().
> 
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com>
> ---
>  block/block-backend.c          | 5 +++++
>  hw/block/virtio-blk.c          | 2 +-
>  include/sysemu/block-backend.h | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 93e46f3..aee8a12 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -700,6 +700,11 @@ int blk_flush_all(void)
>      return bdrv_flush_all();
>  }
>  
> +void blk_drain(BlockBackend *blk)
> +{
> +    bdrv_drain(blk->bs);
> +}
> +
>  void blk_drain_all(void)
>  {
>      bdrv_drain_all();
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index e6afe97..abaca58 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -660,7 +660,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>       * This should cancel pending requests, but can't do nicely until there
>       * are per-device request lists.
>       */
> -    blk_drain_all();
> +    blk_drain(s->blk);
>      blk_set_enable_write_cache(s->blk, s->original_wce);
>  }

I noticed a bug in the current code when reviewing this:

If the 'backup' block job is active when the virtio-blk device is reset,
bs is moved to the global AioContext while the target stays in the
iothread AioContext.  Accesses will be made to target without acquiring
its AioContext.

This is not your fault but calling blk_drain() increases the chance that
this will deadlock.  The bs request cannot make progress until target
completes the I/O request.  Since target doesn't pump events when we
call aio_poll() on bs, draining bs will result in a hang if there are
write requests pending on bs.

I'll write a patch to address this case.

Stefan

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

Re: [Qemu-devel] [PATCH 1/2] block-backend: Introduce blk_drain() and replace blk_drain_all()

[Kevin Wolf]

Am 03.06.2015 um 15:46 hat Alexander Yarygin geschrieben:
> Each call of the virtio_blk_reset() function calls blk_drain_all(),
> which works for all existing BlockDriverStates, while only one
> BlockDriverState needs to be drained.
> 
> This patch introduces the blk_drain() function and replaces
> blk_drain_all() on it in virtio_blk_reset().
> 
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com>

For the BlockBackend part:

Acked-by: Kevin Wolf <kwolf@redhat.com>

The only thing that I could suggest is that if you respin, it would be
cleaner to split this into two parts (as small as they may be). That you
have an "and" in your subject line is a strong hint for this.

Kevin

Re: [Qemu-devel] [PATCH 1/2] block-backend: Introduce blk_drain() and replace blk_drain_all()

[Fam Zheng]

On Wed, 06/03 16:46, Alexander Yarygin wrote:
> Each call of the virtio_blk_reset() function calls blk_drain_all(),
> which works for all existing BlockDriverStates, while only one
> BlockDriverState needs to be drained.
> 
> This patch introduces the blk_drain() function and replaces
> blk_drain_all() on it in virtio_blk_reset().
> 
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com>

I'd like to ask for a split of this patch to two. One for introducing
blk_drain and one for virtio-blk. For the latter please also move the blk_drain
to above virtio_blk_data_plane_stop(), otherwise the restoring to
vblk->complete_request will be wrong.

Fam

> ---
>  block/block-backend.c          | 5 +++++
>  hw/block/virtio-blk.c          | 2 +-
>  include/sysemu/block-backend.h | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 93e46f3..aee8a12 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -700,6 +700,11 @@ int blk_flush_all(void)
>      return bdrv_flush_all();
>  }
>  
> +void blk_drain(BlockBackend *blk)
> +{
> +    bdrv_drain(blk->bs);
> +}
> +
>  void blk_drain_all(void)
>  {
>      bdrv_drain_all();
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index e6afe97..abaca58 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -660,7 +660,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>       * This should cancel pending requests, but can't do nicely until there
>       * are per-device request lists.
>       */
> -    blk_drain_all();
> +    blk_drain(s->blk);
>      blk_set_enable_write_cache(s->blk, s->original_wce);
>  }
>  
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index b4a4d5e..8fc960f 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -118,6 +118,7 @@ int blk_co_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors);
>  int blk_co_flush(BlockBackend *blk);
>  int blk_flush(BlockBackend *blk);
>  int blk_flush_all(void);
> +void blk_drain(BlockBackend *blk);
>  void blk_drain_all(void);
>  BlockdevOnError blk_get_on_error(BlockBackend *blk, bool is_read);
>  BlockErrorAction blk_get_error_action(BlockBackend *blk, bool is_read,
> -- 
> 1.9.1
> 
>