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

[PATCH for-5.0 v3 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.

v3:
- Call blk_inc/dec_in_flight() in blk_prw() rather than inside the
  coroutines [Max]

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 and sync interfaces
  block: Fix blk->in_flight during blk_wait_while_drained()

 include/sysemu/block-backend.h |   1 -
 block/block-backend.c          | 206 ++++++++++++++++++++-------------
 2 files changed, 128 insertions(+), 79 deletions(-)

-- 
2.20.1

[PATCH for-5.0 v3 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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@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

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

[Kevin Wolf]

External callers of blk_co_*() and of the synchronous blk_*() functions
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          | 103 +++++++++++++++++++++++++--------
 2 files changed, 80 insertions(+), 24 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..610dbfa0b2 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)
@@ -1234,7 +1263,7 @@ static void blk_read_entry(void *opaque)
     BlkRwCo *rwco = opaque;
     QEMUIOVector *qiov = rwco->iobuf;
 
-    rwco->ret = blk_co_preadv(rwco->blk, rwco->offset, qiov->size,
+    rwco->ret = blk_do_preadv(rwco->blk, rwco->offset, qiov->size,
                               qiov, rwco->flags);
     aio_wait_kick();
 }
@@ -1244,8 +1273,8 @@ static void blk_write_entry(void *opaque)
     BlkRwCo *rwco = opaque;
     QEMUIOVector *qiov = rwco->iobuf;
 
-    rwco->ret = blk_co_pwritev(rwco->blk, rwco->offset, qiov->size,
-                               qiov, rwco->flags);
+    rwco->ret = blk_do_pwritev_part(rwco->blk, rwco->offset, qiov->size,
+                                    qiov, 0, rwco->flags);
     aio_wait_kick();
 }
 
@@ -1262,6 +1291,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);
@@ -1270,6 +1300,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;
 }
@@ -1394,7 +1425,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 +1443,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 +1529,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 +1547,7 @@ 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);
+    rwco->ret = blk_do_ioctl(rwco->blk, rwco->offset, qiov->iov[0].iov_base);
     aio_wait_kick();
 }
 
@@ -1529,7 +1561,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 +1572,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 +1593,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,12 +1605,23 @@ 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;
     QEMUIOVector *qiov = rwco->iobuf;
 
-    rwco->ret = blk_co_pdiscard(rwco->blk, rwco->offset, qiov->size);
+    rwco->ret = blk_do_pdiscard(rwco->blk, rwco->offset, qiov->size);
     aio_wait_kick();
 }
 
@@ -1585,7 +1630,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 +1647,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,10 +1657,21 @@ 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;
-    rwco->ret = blk_co_flush(rwco->blk);
+    rwco->ret = blk_do_flush(rwco->blk);
     aio_wait_kick();
 }
 
-- 
2.20.1

[PATCH for-5.0 v3 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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@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 610dbfa0b2..38ae413826 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);
     }
 }
 
@@ -1418,12 +1423,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);
@@ -1436,12 +1435,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 v3 2/3] block: Increase BB.in_flight for coroutine and sync interfaces

[Max Reitz]

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

On 07.04.20 14:12, Kevin Wolf wrote:
> External callers of blk_co_*() and of the synchronous blk_*() functions
> 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          | 103 +++++++++++++++++++++++++--------
>  2 files changed, 80 insertions(+), 24 deletions(-)

Thanks!

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


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

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

[Vladimir Sementsov-Ogievskiy]

07.04.2020 15:12, Kevin Wolf wrote:
> External callers of blk_co_*() and of the synchronous blk_*() functions
> 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>

side question:

Should we inc/dec in blk_make_zero, blk_truncate?



-- 
Best regards,
Vladimir

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

[Kevin Wolf]

Am 07.04.2020 um 16:22 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 07.04.2020 15:12, Kevin Wolf wrote:
> > External callers of blk_co_*() and of the synchronous blk_*() functions
> > 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>
> 
> side question:
> 
> Should we inc/dec in blk_make_zero, blk_truncate?

I don't think it's necessary. They call into their bdrv_* counterpart
immediately, so the node-level counter should be enough.

Kevin

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

[Vladimir Sementsov-Ogievskiy]

07.04.2020 17:42, Kevin Wolf wrote:
> Am 07.04.2020 um 16:22 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 07.04.2020 15:12, Kevin Wolf wrote:
>>> External callers of blk_co_*() and of the synchronous blk_*() functions
>>> 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>
>>
>> side question:
>>
>> Should we inc/dec in blk_make_zero, blk_truncate?
> 
> I don't think it's necessary. They call into their bdrv_* counterpart
> immediately, so the node-level counter should be enough.
> 

bdrv_make_zero is not one request, it does block_status/pwrite_zeroes in a loop. So drained section may occur during bdrv_make_zero. Possibly, nothing bad in it?

blk_truncate may do coroutine_enter before incrementing node-level counter, which may only schedule it..



-- 
Best regards,
Vladimir

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

[Kevin Wolf]

Am 07.04.2020 um 16:56 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 07.04.2020 17:42, Kevin Wolf wrote:
> > Am 07.04.2020 um 16:22 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 07.04.2020 15:12, Kevin Wolf wrote:
> > > > External callers of blk_co_*() and of the synchronous blk_*() functions
> > > > 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>
> > > 
> > > side question:
> > > 
> > > Should we inc/dec in blk_make_zero, blk_truncate?
> > 
> > I don't think it's necessary. They call into their bdrv_* counterpart
> > immediately, so the node-level counter should be enough.
> > 
> 
> bdrv_make_zero is not one request, it does block_status/pwrite_zeroes
> in a loop. So drained section may occur during bdrv_make_zero.
> Possibly, nothing bad in it?

It would potentially be a problem if it were called in coroutine
context. But it's a synchronous function that must be called in the main
thread (and also only used in qemu-img), so I don't see how drain could
happen while it runs.

If we did want to make it safe for use in coroutine context, it would be
by using bdrv_inc/dec_in_flight() in bdrv_make_zero().

> blk_truncate may do coroutine_enter before incrementing node-level
> counter, which may only schedule it..

This is bdrv_truncate(), not blk_truncate(). If you address it in
blk_truncate(), you miss the direct callers of bdrv_truncate().

But you're right that it could potentially be a problem. Not sure if it
really is, but maybe better safe than sorry, so if you want to send a
patch, go ahead.

Kevin

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

[Vladimir Sementsov-Ogievskiy]

07.04.2020 19:27, Kevin Wolf wrote:
> Am 07.04.2020 um 16:56 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 07.04.2020 17:42, Kevin Wolf wrote:
>>> Am 07.04.2020 um 16:22 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> 07.04.2020 15:12, Kevin Wolf wrote:
>>>>> External callers of blk_co_*() and of the synchronous blk_*() functions
>>>>> 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>
>>>>
>>>> side question:
>>>>
>>>> Should we inc/dec in blk_make_zero, blk_truncate?
>>>
>>> I don't think it's necessary. They call into their bdrv_* counterpart
>>> immediately, so the node-level counter should be enough.
>>>
>>
>> bdrv_make_zero is not one request, it does block_status/pwrite_zeroes
>> in a loop. So drained section may occur during bdrv_make_zero.
>> Possibly, nothing bad in it?
> 
> It would potentially be a problem if it were called in coroutine
> context. But it's a synchronous function that must be called in the main
> thread (and also only used in qemu-img), so I don't see how drain could
> happen while it runs.
> 
> If we did want to make it safe for use in coroutine context, it would be
> by using bdrv_inc/dec_in_flight() in bdrv_make_zero().
> 
>> blk_truncate may do coroutine_enter before incrementing node-level
>> counter, which may only schedule it..
> 
> This is bdrv_truncate(), not blk_truncate(). If you address it in
> blk_truncate(), you miss the direct callers of bdrv_truncate().
> 
> But you're right that it could potentially be a problem. Not sure if it
> really is, but maybe better safe than sorry, so if you want to send a
> patch, go ahead.
> 

Hmm. Same thing may be said about all other coroutine-enter wrappers in block/io.c. OK, I'll make a patch.


-- 
Best regards,
Vladimir