[for-5.0,1/2] block: Don't blk_wait_while_drained() in blk_co_preadv/pwritev
diff mbox series

Message ID 20200403104415.20963-2-kwolf@redhat.com
State New
Headers show
Series
  • block: Fix blk->in_flight during blk_wait_while_drained()
Related show

Commit Message

Kevin Wolf April 3, 2020, 10:44 a.m. UTC
Calling blk_wait_while_drained() while blk->in_flight is increased for
the current request is wrong because it will cause the drain operation
to deadlock.

In blk_co_preadv() and blk_co_pwritev_part(), this deadlock is easily
fixed by simply removing the blk_wait_while_drained() call. We already
wait in blk_aio_read_entry() and blk_aio_write_entry(), and if a request
didn't wait there because it started basically at the same time as the
drain, we can simply let it complete.

We still do need the wait for emulating synchronous operations, which
don't have a second call yet, so add blk_wait_while_drained() calls
there.

Fixes: cf3129323f900ef5ddbccbe86e4fa801e88c566e
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Max Reitz April 3, 2020, 12:40 p.m. UTC | #1
On 03.04.20 12:44, Kevin Wolf wrote:
> Calling blk_wait_while_drained() while blk->in_flight is increased for
> the current request is wrong because it will cause the drain operation
> to deadlock.
> 
> In blk_co_preadv() and blk_co_pwritev_part(), this deadlock is easily
> fixed by simply removing the blk_wait_while_drained() call. We already
> wait in blk_aio_read_entry() and blk_aio_write_entry(), and if a request
> didn't wait there because it started basically at the same time as the
> drain, we can simply let it complete.
> 
> We still do need the wait for emulating synchronous operations, which
> don't have a second call yet, so add blk_wait_while_drained() calls
> there.
> 
> Fixes: cf3129323f900ef5ddbccbe86e4fa801e88c566e
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

I suppose we need this patch regardless of the next one because before
it, blk_co_preadv() and blk_co_pwritev_part() are sometimes called with
in_flight increased, and sometimes not, so we’d still need to make sure
it’s always increased.  Or, alternatively, just move it out of
blk_co_preadv() and blk_co_pwritev_part(), as this patch does.

Reviewed-by: Max Reitz <mreitz@redhat.com>

Patch
diff mbox series

diff --git a/block/block-backend.c b/block/block-backend.c
index 8b8f2a80a0..3124e367b3 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1154,8 +1154,6 @@  int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
     int ret;
     BlockDriverState *bs;
 
-    blk_wait_while_drained(blk);
-
     /* Call blk_bs() only after waiting, the graph may have changed */
     bs = blk_bs(blk);
     trace_blk_co_preadv(blk, bs, offset, bytes, flags);
@@ -1186,8 +1184,6 @@  int coroutine_fn blk_co_pwritev_part(BlockBackend *blk, int64_t offset,
     int ret;
     BlockDriverState *bs;
 
-    blk_wait_while_drained(blk);
-
     /* Call blk_bs() only after waiting, the graph may have changed */
     bs = blk_bs(blk);
     trace_blk_co_pwritev(blk, bs, offset, bytes, flags);
@@ -1234,6 +1230,7 @@  static void blk_read_entry(void *opaque)
     BlkRwCo *rwco = opaque;
     QEMUIOVector *qiov = rwco->iobuf;
 
+    blk_wait_while_drained(rwco->blk);
     rwco->ret = blk_co_preadv(rwco->blk, rwco->offset, qiov->size,
                               qiov, rwco->flags);
     aio_wait_kick();
@@ -1244,6 +1241,7 @@  static void blk_write_entry(void *opaque)
     BlkRwCo *rwco = opaque;
     QEMUIOVector *qiov = rwco->iobuf;
 
+    blk_wait_while_drained(rwco->blk);
     rwco->ret = blk_co_pwritev(rwco->blk, rwco->offset, qiov->size,
                                qiov, rwco->flags);
     aio_wait_kick();