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

[PATCH for-5.0 v2 0/3] 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.

v2:
- Rework the whole thing so that direct callers of blk_co_*() aren't
  broken after the series [Max]

Kevin Wolf (3):
  block-backend: Reorder flush/pdiscard function definitions
  block: Increase BB.in_flight for coroutine interfaces
  block: Fix blk->in_flight during blk_wait_while_drained()

 include/sysemu/block-backend.h |   1 -
 block/block-backend.c          | 199 +++++++++++++++++++++------------
 2 files changed, 125 insertions(+), 75 deletions(-)

-- 
2.20.1

[PATCH for-5.0 v2 1/3] block-backend: Reorder flush/pdiscard function definitions

[Kevin Wolf]

Move all variants of the flush/pdiscard functions to a single place and
put the blk_co_*() version first because it is called by all other
variants (and will become static in the next patch).

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

diff --git a/block/block-backend.c b/block/block-backend.c
index 8b8f2a80a0..17b2e87afa 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1488,38 +1488,6 @@ BlockAIOCB *blk_aio_pwritev(BlockBackend *blk, int64_t offset,
                         blk_aio_write_entry, flags, cb, opaque);
 }
 
-static void blk_aio_flush_entry(void *opaque)
-{
-    BlkAioEmAIOCB *acb = opaque;
-    BlkRwCo *rwco = &acb->rwco;
-
-    rwco->ret = blk_co_flush(rwco->blk);
-    blk_aio_complete(acb);
-}
-
-BlockAIOCB *blk_aio_flush(BlockBackend *blk,
-                          BlockCompletionFunc *cb, void *opaque)
-{
-    return blk_aio_prwv(blk, 0, 0, NULL, blk_aio_flush_entry, 0, cb, opaque);
-}
-
-static void blk_aio_pdiscard_entry(void *opaque)
-{
-    BlkAioEmAIOCB *acb = opaque;
-    BlkRwCo *rwco = &acb->rwco;
-
-    rwco->ret = blk_co_pdiscard(rwco->blk, rwco->offset, acb->bytes);
-    blk_aio_complete(acb);
-}
-
-BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk,
-                             int64_t offset, int bytes,
-                             BlockCompletionFunc *cb, void *opaque)
-{
-    return blk_aio_prwv(blk, offset, bytes, NULL, blk_aio_pdiscard_entry, 0,
-                        cb, opaque);
-}
-
 void blk_aio_cancel(BlockAIOCB *acb)
 {
     bdrv_aio_cancel(acb);
@@ -1586,6 +1554,37 @@ int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int bytes)
     return bdrv_co_pdiscard(blk->root, offset, bytes);
 }
 
+static void blk_aio_pdiscard_entry(void *opaque)
+{
+    BlkAioEmAIOCB *acb = opaque;
+    BlkRwCo *rwco = &acb->rwco;
+
+    rwco->ret = blk_co_pdiscard(rwco->blk, rwco->offset, acb->bytes);
+    blk_aio_complete(acb);
+}
+
+BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk,
+                             int64_t offset, int bytes,
+                             BlockCompletionFunc *cb, void *opaque)
+{
+    return blk_aio_prwv(blk, offset, bytes, NULL, blk_aio_pdiscard_entry, 0,
+                        cb, opaque);
+}
+
+static void blk_pdiscard_entry(void *opaque)
+{
+    BlkRwCo *rwco = opaque;
+    QEMUIOVector *qiov = rwco->iobuf;
+
+    rwco->ret = blk_co_pdiscard(rwco->blk, rwco->offset, qiov->size);
+    aio_wait_kick();
+}
+
+int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes)
+{
+    return blk_prw(blk, offset, NULL, bytes, blk_pdiscard_entry, 0);
+}
+
 int blk_co_flush(BlockBackend *blk)
 {
     blk_wait_while_drained(blk);
@@ -1597,6 +1596,21 @@ int blk_co_flush(BlockBackend *blk)
     return bdrv_co_flush(blk_bs(blk));
 }
 
+static void blk_aio_flush_entry(void *opaque)
+{
+    BlkAioEmAIOCB *acb = opaque;
+    BlkRwCo *rwco = &acb->rwco;
+
+    rwco->ret = blk_co_flush(rwco->blk);
+    blk_aio_complete(acb);
+}
+
+BlockAIOCB *blk_aio_flush(BlockBackend *blk,
+                          BlockCompletionFunc *cb, void *opaque)
+{
+    return blk_aio_prwv(blk, 0, 0, NULL, blk_aio_flush_entry, 0, cb, opaque);
+}
+
 static void blk_flush_entry(void *opaque)
 {
     BlkRwCo *rwco = opaque;
@@ -2083,20 +2097,6 @@ int blk_truncate(BlockBackend *blk, int64_t offset, bool exact,
     return bdrv_truncate(blk->root, offset, exact, prealloc, errp);
 }
 
-static void blk_pdiscard_entry(void *opaque)
-{
-    BlkRwCo *rwco = opaque;
-    QEMUIOVector *qiov = rwco->iobuf;
-
-    rwco->ret = blk_co_pdiscard(rwco->blk, rwco->offset, qiov->size);
-    aio_wait_kick();
-}
-
-int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes)
-{
-    return blk_prw(blk, offset, NULL, bytes, blk_pdiscard_entry, 0);
-}
-
 int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
                      int64_t pos, int size)
 {
-- 
2.20.1

Re: [PATCH for-5.0 v2 1/3] block-backend: Reorder flush/pdiscard function definitions

[Vladimir Sementsov-Ogievskiy]

06.04.2020 20:14, Kevin Wolf wrote:
> Move all variants of the flush/pdiscard functions to a single place and
> put the blk_co_*() version first because it is called by all other
> variants (and will become static in the next patch).
> 
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH for-5.0 v2 1/3] block-backend: Reorder flush/pdiscard function definitions

[Max Reitz]

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

On 06.04.20 19:14, Kevin Wolf wrote:
> Move all variants of the flush/pdiscard functions to a single place and
> put the blk_co_*() version first because it is called by all other
> variants (and will become static in the next patch).
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c | 92 +++++++++++++++++++++----------------------
>  1 file changed, 46 insertions(+), 46 deletions(-)

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


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

[PATCH for-5.0 v2 2/3] block: Increase BB.in_flight for coroutine interfaces

[Kevin Wolf]

External callers of blk_co_*() don't currently increase the
BlockBackend.in_flight counter, but calls from blk_aio_*() do, so there
is an inconsistency whether the counter has been increased or not.

This patch moves the actual operations to static functions that can
later know they will always be called with in_flight increased exactly
once, even for external callers using the blk_co_*() coroutine
interfaces.

If the public blk_co_*() interface is unused, remove it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/sysemu/block-backend.h |  1 -
 block/block-backend.c          | 94 +++++++++++++++++++++++++++-------
 2 files changed, 76 insertions(+), 19 deletions(-)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index b198deca0b..9bbdbd63d7 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -171,7 +171,6 @@ BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk, int64_t offset, int bytes,
                              BlockCompletionFunc *cb, void *opaque);
 void blk_aio_cancel(BlockAIOCB *acb);
 void blk_aio_cancel_async(BlockAIOCB *acb);
-int blk_co_ioctl(BlockBackend *blk, unsigned long int req, void *buf);
 int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf);
 BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf,
                           BlockCompletionFunc *cb, void *opaque);
diff --git a/block/block-backend.c b/block/block-backend.c
index 17b2e87afa..d330e08b05 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1147,9 +1147,10 @@ static void coroutine_fn blk_wait_while_drained(BlockBackend *blk)
     }
 }
 
-int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
-                               unsigned int bytes, QEMUIOVector *qiov,
-                               BdrvRequestFlags flags)
+/* To be called between exactly one pair of blk_inc/dec_in_flight() */
+static int coroutine_fn
+blk_do_preadv(BlockBackend *blk, int64_t offset, unsigned int bytes,
+              QEMUIOVector *qiov, BdrvRequestFlags flags)
 {
     int ret;
     BlockDriverState *bs;
@@ -1178,10 +1179,24 @@ int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
     return ret;
 }
 
-int coroutine_fn blk_co_pwritev_part(BlockBackend *blk, int64_t offset,
-                                     unsigned int bytes,
-                                     QEMUIOVector *qiov, size_t qiov_offset,
-                                     BdrvRequestFlags flags)
+int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
+                               unsigned int bytes, QEMUIOVector *qiov,
+                               BdrvRequestFlags flags)
+{
+    int ret;
+
+    blk_inc_in_flight(blk);
+    ret = blk_do_preadv(blk, offset, bytes, qiov, flags);
+    blk_dec_in_flight(blk);
+
+    return ret;
+}
+
+/* To be called between exactly one pair of blk_inc/dec_in_flight() */
+static int coroutine_fn
+blk_do_pwritev_part(BlockBackend *blk, int64_t offset, unsigned int bytes,
+                    QEMUIOVector *qiov, size_t qiov_offset,
+                    BdrvRequestFlags flags)
 {
     int ret;
     BlockDriverState *bs;
@@ -1214,6 +1229,20 @@ int coroutine_fn blk_co_pwritev_part(BlockBackend *blk, int64_t offset,
     return ret;
 }
 
+int coroutine_fn blk_co_pwritev_part(BlockBackend *blk, int64_t offset,
+                                     unsigned int bytes,
+                                     QEMUIOVector *qiov, size_t qiov_offset,
+                                     BdrvRequestFlags flags)
+{
+    int ret;
+
+    blk_inc_in_flight(blk);
+    ret = blk_do_pwritev_part(blk, offset, bytes, qiov, qiov_offset, flags);
+    blk_dec_in_flight(blk);
+
+    return ret;
+}
+
 int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
                                 unsigned int bytes, QEMUIOVector *qiov,
                                 BdrvRequestFlags flags)
@@ -1394,7 +1423,7 @@ static void blk_aio_read_entry(void *opaque)
     }
 
     assert(qiov->size == acb->bytes);
-    rwco->ret = blk_co_preadv(rwco->blk, rwco->offset, acb->bytes,
+    rwco->ret = blk_do_preadv(rwco->blk, rwco->offset, acb->bytes,
                               qiov, rwco->flags);
     blk_aio_complete(acb);
 }
@@ -1412,8 +1441,8 @@ static void blk_aio_write_entry(void *opaque)
     }
 
     assert(!qiov || qiov->size == acb->bytes);
-    rwco->ret = blk_co_pwritev(rwco->blk, rwco->offset, acb->bytes,
-                               qiov, rwco->flags);
+    rwco->ret = blk_do_pwritev_part(rwco->blk, rwco->offset, acb->bytes,
+                                    qiov, 0, rwco->flags);
     blk_aio_complete(acb);
 }
 
@@ -1498,7 +1527,9 @@ void blk_aio_cancel_async(BlockAIOCB *acb)
     bdrv_aio_cancel_async(acb);
 }
 
-int blk_co_ioctl(BlockBackend *blk, unsigned long int req, void *buf)
+/* To be called between exactly one pair of blk_inc/dec_in_flight() */
+static int coroutine_fn
+blk_do_ioctl(BlockBackend *blk, unsigned long int req, void *buf)
 {
     blk_wait_while_drained(blk);
 
@@ -1514,8 +1545,10 @@ static void blk_ioctl_entry(void *opaque)
     BlkRwCo *rwco = opaque;
     QEMUIOVector *qiov = rwco->iobuf;
 
-    rwco->ret = blk_co_ioctl(rwco->blk, rwco->offset,
-                             qiov->iov[0].iov_base);
+    blk_inc_in_flight(rwco->blk);
+    rwco->ret = blk_do_ioctl(rwco->blk, rwco->offset, qiov->iov[0].iov_base);
+    blk_dec_in_flight(rwco->blk);
+
     aio_wait_kick();
 }
 
@@ -1529,7 +1562,7 @@ static void blk_aio_ioctl_entry(void *opaque)
     BlkAioEmAIOCB *acb = opaque;
     BlkRwCo *rwco = &acb->rwco;
 
-    rwco->ret = blk_co_ioctl(rwco->blk, rwco->offset, rwco->iobuf);
+    rwco->ret = blk_do_ioctl(rwco->blk, rwco->offset, rwco->iobuf);
 
     blk_aio_complete(acb);
 }
@@ -1540,7 +1573,9 @@ BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf,
     return blk_aio_prwv(blk, req, 0, buf, blk_aio_ioctl_entry, 0, cb, opaque);
 }
 
-int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int bytes)
+/* To be called between exactly one pair of blk_inc/dec_in_flight() */
+static int coroutine_fn
+blk_do_pdiscard(BlockBackend *blk, int64_t offset, int bytes)
 {
     int ret;
 
@@ -1559,7 +1594,7 @@ static void blk_aio_pdiscard_entry(void *opaque)
     BlkAioEmAIOCB *acb = opaque;
     BlkRwCo *rwco = &acb->rwco;
 
-    rwco->ret = blk_co_pdiscard(rwco->blk, rwco->offset, acb->bytes);
+    rwco->ret = blk_do_pdiscard(rwco->blk, rwco->offset, acb->bytes);
     blk_aio_complete(acb);
 }
 
@@ -1571,6 +1606,17 @@ BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk,
                         cb, opaque);
 }
 
+int coroutine_fn blk_co_pdiscard(BlockBackend *blk, int64_t offset, int bytes)
+{
+    int ret;
+
+    blk_inc_in_flight(blk);
+    ret = blk_do_pdiscard(blk, offset, bytes);
+    blk_dec_in_flight(blk);
+
+    return ret;
+}
+
 static void blk_pdiscard_entry(void *opaque)
 {
     BlkRwCo *rwco = opaque;
@@ -1585,7 +1631,8 @@ int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes)
     return blk_prw(blk, offset, NULL, bytes, blk_pdiscard_entry, 0);
 }
 
-int blk_co_flush(BlockBackend *blk)
+/* To be called between exactly one pair of blk_inc/dec_in_flight() */
+static int coroutine_fn blk_do_flush(BlockBackend *blk)
 {
     blk_wait_while_drained(blk);
 
@@ -1601,7 +1648,7 @@ static void blk_aio_flush_entry(void *opaque)
     BlkAioEmAIOCB *acb = opaque;
     BlkRwCo *rwco = &acb->rwco;
 
-    rwco->ret = blk_co_flush(rwco->blk);
+    rwco->ret = blk_do_flush(rwco->blk);
     blk_aio_complete(acb);
 }
 
@@ -1611,6 +1658,17 @@ BlockAIOCB *blk_aio_flush(BlockBackend *blk,
     return blk_aio_prwv(blk, 0, 0, NULL, blk_aio_flush_entry, 0, cb, opaque);
 }
 
+int coroutine_fn blk_co_flush(BlockBackend *blk)
+{
+    int ret;
+
+    blk_inc_in_flight(blk);
+    ret = blk_do_flush(blk);
+    blk_dec_in_flight(blk);
+
+    return ret;
+}
+
 static void blk_flush_entry(void *opaque)
 {
     BlkRwCo *rwco = opaque;
-- 
2.20.1

Re: [PATCH for-5.0 v2 2/3] block: Increase BB.in_flight for coroutine interfaces

[Vladimir Sementsov-Ogievskiy]

06.04.2020 20:14, Kevin Wolf wrote:
> External callers of blk_co_*() don't currently increase the
> BlockBackend.in_flight counter, but calls from blk_aio_*() do, so there
> is an inconsistency whether the counter has been increased or not.
> 
> This patch moves the actual operations to static functions that can
> later know they will always be called with in_flight increased exactly
> once, even for external callers using the blk_co_*() coroutine
> interfaces.
> 
> If the public blk_co_*() interface is unused, remove it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


Still, did you consider instead just move inc/dec to _co_ functions, like


@@ -1154,6 +1154,7 @@ int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
      int ret;
      BlockDriverState *bs;

+    blk_inc_in_flight(blk);
      blk_wait_while_drained(blk);

      /* Call blk_bs() only after waiting, the graph may have changed */
@@ -1175,6 +1176,7 @@ int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,

      ret = bdrv_co_preadv(blk->root, offset, bytes, qiov, flags);
      bdrv_dec_in_flight(bs);
+    blk_dec_in_flight(blk);
      return ret;
  }

@@ -1337,7 +1339,6 @@ static void blk_aio_complete(BlkAioEmAIOCB *acb)
  {
      if (acb->has_returned) {
          acb->common.cb(acb->common.opaque, acb->rwco.ret);
-        blk_dec_in_flight(acb->rwco.blk);
          qemu_aio_unref(acb);
      }
  }
@@ -1357,7 +1358,6 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset, int bytes,
      BlkAioEmAIOCB *acb;
      Coroutine *co;

-    blk_inc_in_flight(blk);
      acb = blk_aio_get(&blk_aio_em_aiocb_info, blk, cb, opaque);
      acb->rwco = (BlkRwCo) {
          .blk    = blk,
@@ -1388,9 +1388,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);



(and same for write, ioctl, flush, discard). It seems more consistent.. Should it work?


-- 
Best regards,
Vladimir

Re: [PATCH for-5.0 v2 2/3] block: Increase BB.in_flight for coroutine interfaces

[Kevin Wolf]

Am 07.04.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 06.04.2020 20:14, Kevin Wolf wrote:
> > External callers of blk_co_*() don't currently increase the
> > BlockBackend.in_flight counter, but calls from blk_aio_*() do, so there
> > is an inconsistency whether the counter has been increased or not.
> > 
> > This patch moves the actual operations to static functions that can
> > later know they will always be called with in_flight increased exactly
> > once, even for external callers using the blk_co_*() coroutine
> > interfaces.
> > 
> > If the public blk_co_*() interface is unused, remove it.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Still, did you consider instead just move inc/dec to _co_ functions, like
> [...]
> (and same for write, ioctl, flush, discard). It seems more
> consistent.. Should it work?

No, it would be wrong because it would be too late. The main purpose of
blk_inc_in_flight() is to keep the request covered during the first and
the last phase outside of blk_co_*(), which can potentially involve BHs
like blk_aio_complete_bh().

Kevin

Re: [PATCH for-5.0 v2 2/3] block: Increase BB.in_flight for coroutine interfaces

[Vladimir Sementsov-Ogievskiy]

07.04.2020 11:52, Kevin Wolf wrote:
> Am 07.04.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 06.04.2020 20:14, Kevin Wolf wrote:
>>> External callers of blk_co_*() don't currently increase the
>>> BlockBackend.in_flight counter, but calls from blk_aio_*() do, so there
>>> is an inconsistency whether the counter has been increased or not.
>>>
>>> This patch moves the actual operations to static functions that can
>>> later know they will always be called with in_flight increased exactly
>>> once, even for external callers using the blk_co_*() coroutine
>>> interfaces.
>>>
>>> If the public blk_co_*() interface is unused, remove it.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> Still, did you consider instead just move inc/dec to _co_ functions, like
>> [...]
>> (and same for write, ioctl, flush, discard). It seems more
>> consistent.. Should it work?
> 
> No, it would be wrong because it would be too late. The main purpose of
> blk_inc_in_flight() is to keep the request covered during the first and
> the last phase outside of blk_co_*(), which can potentially involve BHs
> like blk_aio_complete_bh().
> 
> Kevin
> 

OK, thanks, I see now, we want to caver possible completion BH.
Hmm, not too late but too early (for decrement).. As nothing interesting
happening between increment in blk_aio_prwv and entering the coroutine with
_do_ function.

-- 
Best regards,
Vladimir

Re: [PATCH for-5.0 v2 2/3] block: Increase BB.in_flight for coroutine interfaces

[Kevin Wolf]

Am 07.04.2020 um 11:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 07.04.2020 11:52, Kevin Wolf wrote:
> > Am 07.04.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 06.04.2020 20:14, Kevin Wolf wrote:
> > > > External callers of blk_co_*() don't currently increase the
> > > > BlockBackend.in_flight counter, but calls from blk_aio_*() do, so there
> > > > is an inconsistency whether the counter has been increased or not.
> > > > 
> > > > This patch moves the actual operations to static functions that can
> > > > later know they will always be called with in_flight increased exactly
> > > > once, even for external callers using the blk_co_*() coroutine
> > > > interfaces.
> > > > 
> > > > If the public blk_co_*() interface is unused, remove it.
> > > > 
> > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > 
> > > 
> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > 
> > > Still, did you consider instead just move inc/dec to _co_ functions, like
> > > [...]
> > > (and same for write, ioctl, flush, discard). It seems more
> > > consistent.. Should it work?
> > 
> > No, it would be wrong because it would be too late. The main purpose of
> > blk_inc_in_flight() is to keep the request covered during the first and
> > the last phase outside of blk_co_*(), which can potentially involve BHs
> > like blk_aio_complete_bh().
> 
> OK, thanks, I see now, we want to caver possible completion BH.
> Hmm, not too late but too early (for decrement).. As nothing interesting
> happening between increment in blk_aio_prwv and entering the coroutine with
> _do_ function.

Basically it covers everything that isn't yet covered by
bdrv_inc/dec_in_flight() on the node level, but completion is probably
the most important part.

Start, too, because actually something very interesting is happening
between blk_aio_prwv() and blk_do_*(): It uses bdrv_coroutine_enter(),
which in some circumstances may end up only scheduling the coroutine
instead of immediately entering it.

Kevin

Re: [PATCH for-5.0 v2 2/3] block: Increase BB.in_flight for coroutine interfaces

[Vladimir Sementsov-Ogievskiy]

07.04.2020 12:48, Kevin Wolf wrote:
> Am 07.04.2020 um 11:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 07.04.2020 11:52, Kevin Wolf wrote:
>>> Am 07.04.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> 06.04.2020 20:14, Kevin Wolf wrote:
>>>>> External callers of blk_co_*() don't currently increase the
>>>>> BlockBackend.in_flight counter, but calls from blk_aio_*() do, so there
>>>>> is an inconsistency whether the counter has been increased or not.
>>>>>
>>>>> This patch moves the actual operations to static functions that can
>>>>> later know they will always be called with in_flight increased exactly
>>>>> once, even for external callers using the blk_co_*() coroutine
>>>>> interfaces.
>>>>>
>>>>> If the public blk_co_*() interface is unused, remove it.
>>>>>
>>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>>>
>>>>
>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>
>>>> Still, did you consider instead just move inc/dec to _co_ functions, like
>>>> [...]
>>>> (and same for write, ioctl, flush, discard). It seems more
>>>> consistent.. Should it work?
>>>
>>> No, it would be wrong because it would be too late. The main purpose of
>>> blk_inc_in_flight() is to keep the request covered during the first and
>>> the last phase outside of blk_co_*(), which can potentially involve BHs
>>> like blk_aio_complete_bh().
>>
>> OK, thanks, I see now, we want to caver possible completion BH.
>> Hmm, not too late but too early (for decrement).. As nothing interesting
>> happening between increment in blk_aio_prwv and entering the coroutine with
>> _do_ function.
> 
> Basically it covers everything that isn't yet covered by
> bdrv_inc/dec_in_flight() on the node level, but completion is probably
> the most important part.
> 
> Start, too, because actually something very interesting is happening
> between blk_aio_prwv() and blk_do_*(): It uses bdrv_coroutine_enter(),
> which in some circumstances may end up only scheduling the coroutine
> instead of immediately entering it.
> 

I missed this fact, I thought bdrv_coroutine_enter is simpler. OK, thanks. You answered my question in next too.


-- 
Best regards,
Vladimir

Re: [PATCH for-5.0 v2 2/3] block: Increase BB.in_flight for coroutine interfaces

[Max Reitz]

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

On 06.04.20 19:14, Kevin Wolf wrote:
> External callers of blk_co_*() don't currently increase the
> BlockBackend.in_flight counter, but calls from blk_aio_*() do, so there
> is an inconsistency whether the counter has been increased or not.
> 
> This patch moves the actual operations to static functions that can
> later know they will always be called with in_flight increased exactly
> once, even for external callers using the blk_co_*() coroutine
> interfaces.
> 
> If the public blk_co_*() interface is unused, remove it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/sysemu/block-backend.h |  1 -
>  block/block-backend.c          | 94 +++++++++++++++++++++++++++-------
>  2 files changed, 76 insertions(+), 19 deletions(-)

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


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

Re: [PATCH for-5.0 v2 2/3] block: Increase BB.in_flight for coroutine interfaces

[Max Reitz]

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

On 07.04.20 12:04, Max Reitz wrote:
> On 06.04.20 19:14, Kevin Wolf wrote:
>> External callers of blk_co_*() don't currently increase the
>> BlockBackend.in_flight counter, but calls from blk_aio_*() do, so there
>> is an inconsistency whether the counter has been increased or not.
>>
>> This patch moves the actual operations to static functions that can
>> later know they will always be called with in_flight increased exactly
>> once, even for external callers using the blk_co_*() coroutine
>> interfaces.
>>
>> If the public blk_co_*() interface is unused, remove it.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  include/sysemu/block-backend.h |  1 -
>>  block/block-backend.c          | 94 +++++++++++++++++++++++++++-------
>>  2 files changed, 76 insertions(+), 19 deletions(-)
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>

On second thought (I assumed this would be addressed by the third
patch), blk_prw() no longer increments in_flight, but the blk_co_*
functions do that now.  In v1, blk_prw() did that.

I thought we’d want blk_prw() to set in_flight, just like blk_aio_prwv()
does, and then let the synchronous functions that use blk_prw() pass the
blk_do_* functions to it.

Max


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

Re: [PATCH for-5.0 v2 2/3] block: Increase BB.in_flight for coroutine interfaces

[Kevin Wolf]

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

Am 07.04.2020 um 12:15 hat Max Reitz geschrieben:
> On 07.04.20 12:04, Max Reitz wrote:
> > On 06.04.20 19:14, Kevin Wolf wrote:
> >> External callers of blk_co_*() don't currently increase the
> >> BlockBackend.in_flight counter, but calls from blk_aio_*() do, so there
> >> is an inconsistency whether the counter has been increased or not.
> >>
> >> This patch moves the actual operations to static functions that can
> >> later know they will always be called with in_flight increased exactly
> >> once, even for external callers using the blk_co_*() coroutine
> >> interfaces.
> >>
> >> If the public blk_co_*() interface is unused, remove it.
> >>
> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >> ---
> >>  include/sysemu/block-backend.h |  1 -
> >>  block/block-backend.c          | 94 +++++++++++++++++++++++++++-------
> >>  2 files changed, 76 insertions(+), 19 deletions(-)
> > 
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> On second thought (I assumed this would be addressed by the third
> patch), blk_prw() no longer increments in_flight, but the blk_co_*
> functions do that now.  In v1, blk_prw() did that.
> 
> I thought we’d want blk_prw() to set in_flight, just like blk_aio_prwv()
> does, and then let the synchronous functions that use blk_prw() pass the
> blk_do_* functions to it.

Does it make a difference, though?

But the change should be easy enough (inc/dec in blk_prw() and the let
*_entry() call blk_do_*() instead of blk_co_*()) that I guess I can just
do it and send a v3.

Kevin

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

Re: [PATCH for-5.0 v2 2/3] block: Increase BB.in_flight for coroutine interfaces

[Max Reitz]

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

On 07.04.20 13:13, Kevin Wolf wrote:
> Am 07.04.2020 um 12:15 hat Max Reitz geschrieben:
>> On 07.04.20 12:04, Max Reitz wrote:
>>> On 06.04.20 19:14, Kevin Wolf wrote:
>>>> External callers of blk_co_*() don't currently increase the
>>>> BlockBackend.in_flight counter, but calls from blk_aio_*() do, so there
>>>> is an inconsistency whether the counter has been increased or not.
>>>>
>>>> This patch moves the actual operations to static functions that can
>>>> later know they will always be called with in_flight increased exactly
>>>> once, even for external callers using the blk_co_*() coroutine
>>>> interfaces.
>>>>
>>>> If the public blk_co_*() interface is unused, remove it.
>>>>
>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>>> ---
>>>>  include/sysemu/block-backend.h |  1 -
>>>>  block/block-backend.c          | 94 +++++++++++++++++++++++++++-------
>>>>  2 files changed, 76 insertions(+), 19 deletions(-)
>>>
>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>> On second thought (I assumed this would be addressed by the third
>> patch), blk_prw() no longer increments in_flight, but the blk_co_*
>> functions do that now.  In v1, blk_prw() did that.
>>
>> I thought we’d want blk_prw() to set in_flight, just like blk_aio_prwv()
>> does, and then let the synchronous functions that use blk_prw() pass the
>> blk_do_* functions to it.
> 
> Does it make a difference, though?

You mean because blk_prw() has a fast path for “already in coroutine”?
Perhaps not.  Still, feels a bit weird to me not to have in_flight
incremented around a potential POLL loop.

Maybe mostly I was wondering why v1 did and v2 didn’t. *shrug*

Max

> But the change should be easy enough (inc/dec in blk_prw() and the let
> *_entry() call blk_do_*() instead of blk_co_*()) that I guess I can just
> do it and send a v3.
> 
> Kevin
> 



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

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

[Kevin Wolf]

Waiting in 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.

This patch makes sure that blk_wait_while_drained() is called with
blk->in_flight increased exactly once for the current request, and that
it temporarily decreases the counter while it waits.

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

diff --git a/block/block-backend.c b/block/block-backend.c
index d330e08b05..f621435f0b 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1140,10 +1140,15 @@ static int blk_check_byte_request(BlockBackend *blk, int64_t offset,
     return 0;
 }
 
+/* To be called between exactly one pair of blk_inc/dec_in_flight() */
 static void coroutine_fn blk_wait_while_drained(BlockBackend *blk)
 {
+    assert(blk->in_flight > 0);
+
     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);
     }
 }
 
@@ -1416,12 +1421,6 @@ static void blk_aio_read_entry(void *opaque)
     BlkRwCo *rwco = &acb->rwco;
     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);
     rwco->ret = blk_do_preadv(rwco->blk, rwco->offset, acb->bytes,
                               qiov, rwco->flags);
@@ -1434,12 +1433,6 @@ static void blk_aio_write_entry(void *opaque)
     BlkRwCo *rwco = &acb->rwco;
     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);
     rwco->ret = blk_do_pwritev_part(rwco->blk, rwco->offset, acb->bytes,
                                     qiov, 0, rwco->flags);
-- 
2.20.1

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

[Vladimir Sementsov-Ogievskiy]

06.04.2020 20:14, Kevin Wolf wrote:
> Waiting in 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.
> 
> This patch makes sure that blk_wait_while_drained() is called with
> blk->in_flight increased exactly once for the current request, and that
> it temporarily decreases the counter while it waits.
> 
> Fixes: cf3129323f900ef5ddbccbe86e4fa801e88c566e
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/block-backend.c | 17 +++++------------
>   1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index d330e08b05..f621435f0b 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1140,10 +1140,15 @@ static int blk_check_byte_request(BlockBackend *blk, int64_t offset,
>       return 0;
>   }
>   
> +/* To be called between exactly one pair of blk_inc/dec_in_flight() */
>   static void coroutine_fn blk_wait_while_drained(BlockBackend *blk)
>   {
> +    assert(blk->in_flight > 0);

Hmm. You promise to make sure that in_flight increased exactly once. Shouldn't it be assert(blk->in_flight == 1) ?

> +
>       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);
>       }
>   }
>   
> @@ -1416,12 +1421,6 @@ static void blk_aio_read_entry(void *opaque)
>       BlkRwCo *rwco = &acb->rwco;
>       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);
> -    }

Hm, you drop it as it's called from blk_do_preadv too. I think it worth mentioning in commit message still.

> -
>       assert(qiov->size == acb->bytes);
>       rwco->ret = blk_do_preadv(rwco->blk, rwco->offset, acb->bytes,
>                                 qiov, rwco->flags);
> @@ -1434,12 +1433,6 @@ static void blk_aio_write_entry(void *opaque)
>       BlkRwCo *rwco = &acb->rwco;
>       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);
>       rwco->ret = blk_do_pwritev_part(rwco->blk, rwco->offset, acb->bytes,
>                                       qiov, 0, rwco->flags);
> 

With assert(blk->in_flight == 1) and mention extra wait removing in commit message:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


And still, if my suggestion in previous patch works, we may fix it all without extra blk_dec/blk_inc pair, by just moving blk_inc_ after blk_wait_while_drained in my previous suggestion.. It looks more native for me (but may be I miss something?):


@@ -1154,6 +1154,7 @@ int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
      int ret;
      BlockDriverState *bs;

      blk_wait_while_drained(blk);
+    blk_inc_in_flight(blk);

      /* Call blk_bs() only after waiting, the graph may have changed */
@@ -1175,6 +1176,7 @@ int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,

      ret = bdrv_co_preadv(blk->root, offset, bytes, qiov, flags);
      bdrv_dec_in_flight(bs);
+    blk_dec_in_flight(blk);
      return ret;
  }

@@ -1337,7 +1339,6 @@ static void blk_aio_complete(BlkAioEmAIOCB *acb)
  {
      if (acb->has_returned) {
          acb->common.cb(acb->common.opaque, acb->rwco.ret);
-        blk_dec_in_flight(acb->rwco.blk);
          qemu_aio_unref(acb);
      }
  }
@@ -1357,7 +1358,6 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset, int bytes,
      BlkAioEmAIOCB *acb;
      Coroutine *co;

-    blk_inc_in_flight(blk);
      acb = blk_aio_get(&blk_aio_em_aiocb_info, blk, cb, opaque);
      acb->rwco = (BlkRwCo) {
          .blk    = blk,
@@ -1388,9 +1388,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);


(and same for write, ioctl, flush, discard)

-- 
Best regards,
Vladimir

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

[Kevin Wolf]

Am 07.04.2020 um 08:52 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 06.04.2020 20:14, Kevin Wolf wrote:
> > Waiting in 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.
> > 
> > This patch makes sure that blk_wait_while_drained() is called with
> > blk->in_flight increased exactly once for the current request, and that
> > it temporarily decreases the counter while it waits.
> > 
> > Fixes: cf3129323f900ef5ddbccbe86e4fa801e88c566e
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   block/block-backend.c | 17 +++++------------
> >   1 file changed, 5 insertions(+), 12 deletions(-)
> > 
> > diff --git a/block/block-backend.c b/block/block-backend.c
> > index d330e08b05..f621435f0b 100644
> > --- a/block/block-backend.c
> > +++ b/block/block-backend.c
> > @@ -1140,10 +1140,15 @@ static int blk_check_byte_request(BlockBackend *blk, int64_t offset,
> >       return 0;
> >   }
> > +/* To be called between exactly one pair of blk_inc/dec_in_flight() */
> >   static void coroutine_fn blk_wait_while_drained(BlockBackend *blk)
> >   {
> > +    assert(blk->in_flight > 0);
> 
> Hmm. You promise to make sure that in_flight increased exactly once.
> Shouldn't it be assert(blk->in_flight == 1) ?

Exactly once for this specific request, but if you have multiple
requests in flight, blk->in_flight will be the sum of all requests.

Just asserting > 0 should still catch potential bugs because you won't
always have multiple requests in flight.

> > +
> >       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);
> >       }
> >   }
> > @@ -1416,12 +1421,6 @@ static void blk_aio_read_entry(void *opaque)
> >       BlkRwCo *rwco = &acb->rwco;
> >       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);
> > -    }
> 
> Hm, you drop it as it's called from blk_do_preadv too. I think it
> worth mentioning in commit message still.

Okay, I can add a sentence like "The blk_wait_while_drained() call in
blk_aio_read/write_entry is redundant with the one in blk_co_*(), so
drop it."

> > -
> >       assert(qiov->size == acb->bytes);
> >       rwco->ret = blk_do_preadv(rwco->blk, rwco->offset, acb->bytes,
> >                                 qiov, rwco->flags);
> > @@ -1434,12 +1433,6 @@ static void blk_aio_write_entry(void *opaque)
> >       BlkRwCo *rwco = &acb->rwco;
> >       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);
> >       rwco->ret = blk_do_pwritev_part(rwco->blk, rwco->offset, acb->bytes,
> >                                       qiov, 0, rwco->flags);
> > 
> 
> With assert(blk->in_flight == 1) and mention extra wait removing in commit message:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks, and I hope you agree with blk->in_flight > 0 now.

Kevin

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

[Vladimir Sementsov-Ogievskiy]

07.04.2020 11:59, Kevin Wolf wrote:
> Am 07.04.2020 um 08:52 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 06.04.2020 20:14, Kevin Wolf wrote:
>>> Waiting in 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.
>>>
>>> This patch makes sure that blk_wait_while_drained() is called with
>>> blk->in_flight increased exactly once for the current request, and that
>>> it temporarily decreases the counter while it waits.
>>>
>>> Fixes: cf3129323f900ef5ddbccbe86e4fa801e88c566e
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>    block/block-backend.c | 17 +++++------------
>>>    1 file changed, 5 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/block/block-backend.c b/block/block-backend.c
>>> index d330e08b05..f621435f0b 100644
>>> --- a/block/block-backend.c
>>> +++ b/block/block-backend.c
>>> @@ -1140,10 +1140,15 @@ static int blk_check_byte_request(BlockBackend *blk, int64_t offset,
>>>        return 0;
>>>    }
>>> +/* To be called between exactly one pair of blk_inc/dec_in_flight() */
>>>    static void coroutine_fn blk_wait_while_drained(BlockBackend *blk)
>>>    {
>>> +    assert(blk->in_flight > 0);
>>
>> Hmm. You promise to make sure that in_flight increased exactly once.
>> Shouldn't it be assert(blk->in_flight == 1) ?
> 
> Exactly once for this specific request, but if you have multiple
> requests in flight, blk->in_flight will be the sum of all requests.
> 
> Just asserting > 0 should still catch potential bugs because you won't
> always have multiple requests in flight.
> 
>>> +
>>>        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);
>>>        }
>>>    }
>>> @@ -1416,12 +1421,6 @@ static void blk_aio_read_entry(void *opaque)
>>>        BlkRwCo *rwco = &acb->rwco;
>>>        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);
>>> -    }
>>
>> Hm, you drop it as it's called from blk_do_preadv too. I think it
>> worth mentioning in commit message still.
> 
> Okay, I can add a sentence like "The blk_wait_while_drained() call in
> blk_aio_read/write_entry is redundant with the one in blk_co_*(), so
> drop it."

Yes, that works
> 
>>> -
>>>        assert(qiov->size == acb->bytes);
>>>        rwco->ret = blk_do_preadv(rwco->blk, rwco->offset, acb->bytes,
>>>                                  qiov, rwco->flags);
>>> @@ -1434,12 +1433,6 @@ static void blk_aio_write_entry(void *opaque)
>>>        BlkRwCo *rwco = &acb->rwco;
>>>        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);
>>>        rwco->ret = blk_do_pwritev_part(rwco->blk, rwco->offset, acb->bytes,
>>>                                        qiov, 0, rwco->flags);
>>>
>>
>> With assert(blk->in_flight == 1) and mention extra wait removing in commit message:
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Thanks, and I hope you agree with blk->in_flight > 0 now.
> 

Yes, I agree.

Hmm still interesting, could we move somehow blk_wait_while_drained out of inc/dec section? It seems there are no yields and no coroutine switches between first in_flight increment and call of blk_wait_while_drained, only initialization of acb.


-- 
Best regards,
Vladimir

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

[Max Reitz]

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

On 06.04.20 19:14, Kevin Wolf wrote:
> Waiting in 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.
> 
> This patch makes sure that blk_wait_while_drained() is called with
> blk->in_flight increased exactly once for the current request, and that
> it temporarily decreases the counter while it waits.
> 
> Fixes: cf3129323f900ef5ddbccbe86e4fa801e88c566e
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)

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


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