[PATCH for-5.0 0/2] block: Fix blk->in_flight during blk_wait_while_drained()

[PATCH for-5.0 0/2] block: Fix blk->in_flight during blk_wait_while_drained()

[Kevin Wolf]

This fixes deadlocks when draining a BlockBackend in an iothread that
receives new requests at the same time.

Kevin Wolf (2):
  block: Don't blk_wait_while_drained() in blk_co_preadv/pwritev
  block: Fix blk->in_flight during blk_wait_while_drained()

 block/block-backend.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

-- 
2.20.1

[PATCH for-5.0 1/2] block: Don't blk_wait_while_drained() in blk_co_preadv/pwritev

[Kevin Wolf]

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(-)

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();
-- 
2.20.1

Re: [PATCH for-5.0 1/2] block: Don't blk_wait_while_drained() in blk_co_preadv/pwritev

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1301 bytes --]

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>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH for-5.0 2/2] block: Fix blk->in_flight during blk_wait_while_drained()

[Kevin Wolf]

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.

Many callers of blk_wait_while_drained() have already increased
blk->in_flight when called in a blk_aio_*() path, but can also be called
in synchonous code paths where blk->in_flight isn't increased. This
means that these calls of blk_wait_while_drained() are wrong at least in
some cases.

In order to fix this, increase blk->in_flight even for synchronous
operations and temporarily decrease the counter again in
blk_wait_while_drained().

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

diff --git a/block/block-backend.c b/block/block-backend.c
index 3124e367b3..7bd16402b8 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1143,7 +1143,9 @@ static int blk_check_byte_request(BlockBackend *blk, int64_t offset,
 static void coroutine_fn blk_wait_while_drained(BlockBackend *blk)
 {
     if (blk->quiesce_counter && !blk->disable_request_queuing) {
+        blk_dec_in_flight(blk);
         qemu_co_queue_wait(&blk->queued_requests, NULL);
+        blk_inc_in_flight(blk);
     }
 }
 
@@ -1260,6 +1262,7 @@ static int blk_prw(BlockBackend *blk, int64_t offset, uint8_t *buf,
         .ret    = NOT_DONE,
     };
 
+    blk_inc_in_flight(blk);
     if (qemu_in_coroutine()) {
         /* Fast-path if already in coroutine context */
         co_entry(&rwco);
@@ -1268,6 +1271,7 @@ static int blk_prw(BlockBackend *blk, int64_t offset, uint8_t *buf,
         bdrv_coroutine_enter(blk_bs(blk), co);
         BDRV_POLL_WHILE(blk_bs(blk), rwco.ret == NOT_DONE);
     }
+    blk_dec_in_flight(blk);
 
     return rwco.ret;
 }
@@ -1386,9 +1390,7 @@ static void blk_aio_read_entry(void *opaque)
     QEMUIOVector *qiov = rwco->iobuf;
 
     if (rwco->blk->quiesce_counter) {
-        blk_dec_in_flight(rwco->blk);
         blk_wait_while_drained(rwco->blk);
-        blk_inc_in_flight(rwco->blk);
     }
 
     assert(qiov->size == acb->bytes);
@@ -1404,9 +1406,7 @@ static void blk_aio_write_entry(void *opaque)
     QEMUIOVector *qiov = rwco->iobuf;
 
     if (rwco->blk->quiesce_counter) {
-        blk_dec_in_flight(rwco->blk);
         blk_wait_while_drained(rwco->blk);
-        blk_inc_in_flight(rwco->blk);
     }
 
     assert(!qiov || qiov->size == acb->bytes);
-- 
2.20.1

Re: [PATCH for-5.0 2/2] block: Fix blk->in_flight during blk_wait_while_drained()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1022 bytes --]

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.
> 
> Many callers of blk_wait_while_drained() have already increased
> blk->in_flight when called in a blk_aio_*() path, but can also be called
> in synchonous code paths where blk->in_flight isn't increased. This
> means that these calls of blk_wait_while_drained() are wrong at least in
> some cases.
> 
> In order to fix this, increase blk->in_flight even for synchronous
> operations and temporarily decrease the counter again in
> blk_wait_while_drained().
> 
> Fixes: cf3129323f900ef5ddbccbe86e4fa801e88c566e
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

blk_co_pdiscard() and blk_co_flush() are called from outside of
block-backend.c (namely from mirror.c and nbd/server.c).  Is that OK?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH for-5.0 2/2] block: Fix blk->in_flight during blk_wait_while_drained()

[Kevin Wolf]

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

Am 03.04.2020 um 14:42 hat Max Reitz geschrieben:
> 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.
> > 
> > Many callers of blk_wait_while_drained() have already increased
> > blk->in_flight when called in a blk_aio_*() path, but can also be called
> > in synchonous code paths where blk->in_flight isn't increased. This
> > means that these calls of blk_wait_while_drained() are wrong at least in
> > some cases.
> > 
> > In order to fix this, increase blk->in_flight even for synchronous
> > operations and temporarily decrease the counter again in
> > blk_wait_while_drained().
> > 
> > Fixes: cf3129323f900ef5ddbccbe86e4fa801e88c566e
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/block-backend.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> blk_co_pdiscard() and blk_co_flush() are called from outside of
> block-backend.c (namely from mirror.c and nbd/server.c).  Is that OK?

Hm... I think you're right that the NBD server has a problem now because
we might now decrease blk->in_flight without having increased it.
(Mirror should be fine anyway because it sets disable_request_queuing.)

At first I was going to suggest that we could do the opposite of this
patch and just move the dec/wait/inc sequence (which this patch removes
for read/write) to all coroutine entry functions, so direct calls
wouldn't incorrectly decrease the counter.

But this is not what we want either, we do want to queue requests for
drained BlockBackends even in the blk_co_*() API.

Do you have another idea or do we have to turn blk_co_*() into wrappers
around the existing functions, which would gain an additional bool
parameter that tells whether we need to dec/inc or not?

Kevin

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

Re: [PATCH for-5.0 2/2] block: Fix blk->in_flight during blk_wait_while_drained()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2572 bytes --]

On 03.04.20 16:50, Kevin Wolf wrote:
> Am 03.04.2020 um 14:42 hat Max Reitz geschrieben:
>> 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.
>>>
>>> Many callers of blk_wait_while_drained() have already increased
>>> blk->in_flight when called in a blk_aio_*() path, but can also be called
>>> in synchonous code paths where blk->in_flight isn't increased. This
>>> means that these calls of blk_wait_while_drained() are wrong at least in
>>> some cases.
>>>
>>> In order to fix this, increase blk->in_flight even for synchronous
>>> operations and temporarily decrease the counter again in
>>> blk_wait_while_drained().
>>>
>>> Fixes: cf3129323f900ef5ddbccbe86e4fa801e88c566e
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  block/block-backend.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> blk_co_pdiscard() and blk_co_flush() are called from outside of
>> block-backend.c (namely from mirror.c and nbd/server.c).  Is that OK?
> 
> Hm... I think you're right that the NBD server has a problem now because
> we might now decrease blk->in_flight without having increased it.
> (Mirror should be fine anyway because it sets disable_request_queuing.)
> 
> At first I was going to suggest that we could do the opposite of this
> patch and just move the dec/wait/inc sequence (which this patch removes
> for read/write) to all coroutine entry functions, so direct calls
> wouldn't incorrectly decrease the counter.
> 
> But this is not what we want either, we do want to queue requests for
> drained BlockBackends even in the blk_co_*() API.
> 
> Do you have another idea or do we have to turn blk_co_*() into wrappers
> around the existing functions, which would gain an additional bool
> parameter that tells whether we need to dec/inc or not?

So that whenever blk_co_* is called from outside of block-backend.c, we
don’t dec/inc?

Sounds reasonable to me.

The only alternative I see would be ensuring we call
blk_wait_while_drained() only outside of in_flight sections (without
having to dec/inc around it).  But we can’t call it in synchronous
sections.  And for those synchronous calls, we also have to wrap the
in_flight section around the whole asynchronous boilerplate, so there is
no place where they can call bdrv_wait_while_drained() without dec/inc
around it.

So I can’t think of another way either.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]