Re: [PATCH v2 9/9] block/io: expand in_flight inc/dec section: bdrv_make_zero

2020-05-01 Thread Eric Blake

On 4/27/20 9:39 AM, Vladimir Sementsov-Ogievskiy wrote:

It's safer to expand in_flight request to start before enter to
coroutine in synchronous wrappers and end after BDRV_POLL_WHILE loop.
Note that qemu_coroutine_enter may only schedule the coroutine in some
circumstances.


See my wording suggestions earlier in the series.



bdrv_make_zero update includes refactoring: move the whole loop into
coroutine, which has additional benefit of not create/enter new
coroutine on each iteration.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/io.c | 54 +++---
  1 file changed, 51 insertions(+), 3 deletions(-)




+int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
+{
+int ret;
+
+bdrv_inc_in_flight(child->bs);
+
+if (qemu_in_coroutine()) {
+/* Fast-path if already in coroutine context */
+ret = bdrv_do_make_zero(child, flags);
+} else {
+BdrvDoMakeZeroData data = {
+.child = child,
+.flags = flags,
+.done = false,


Another case where the line '.done = false,' is optional, thanks to C 
semantics, but does not hurt to leave it in.


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org




[PATCH v2 9/9] block/io: expand in_flight inc/dec section: bdrv_make_zero

2020-04-27 Thread Vladimir Sementsov-Ogievskiy
It's safer to expand in_flight request to start before enter to
coroutine in synchronous wrappers and end after BDRV_POLL_WHILE loop.
Note that qemu_coroutine_enter may only schedule the coroutine in some
circumstances.

bdrv_make_zero update includes refactoring: move the whole loop into
coroutine, which has additional benefit of not create/enter new
coroutine on each iteration.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/io.c | 54 +++---
 1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/block/io.c b/block/io.c
index 3bc0daec33..cd5374e6c7 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2740,8 +2740,11 @@ int bdrv_is_allocated_above(BlockDriverState *top, 
BlockDriverState *base,
  * BDRV_REQ_FUA).
  *
  * Returns < 0 on error, 0 on success. For error codes see bdrv_write().
+ *
+ * To be called between exactly one pair of bdrv_inc/dec_in_flight()
  */
-int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
+static int coroutine_fn
+bdrv_do_make_zero(BdrvChild *child, BdrvRequestFlags flags)
 {
 int ret;
 int64_t target_size, bytes, offset = 0;
@@ -2757,7 +2760,8 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags 
flags)
 if (bytes <= 0) {
 return 0;
 }
-ret = bdrv_block_status(bs, offset, bytes, , NULL, NULL);
+ret = bdrv_co_block_status(bs, true, false,
+   offset, bytes, , NULL, NULL);
 if (ret < 0) {
 return ret;
 }
@@ -2765,7 +2769,7 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags 
flags)
 offset += bytes;
 continue;
 }
-ret = bdrv_pwrite_zeroes(child, offset, bytes, flags);
+ret = bdrv_do_pwrite_zeroes(child, offset, bytes, flags);
 if (ret < 0) {
 return ret;
 }
@@ -2773,6 +2777,50 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags 
flags)
 }
 }
 
+typedef struct BdrvDoMakeZeroData {
+BdrvChild *child;
+BdrvRequestFlags flags;
+int ret;
+bool done;
+} BdrvDoMakeZeroData;
+
+/* To be called between exactly one pair of bdrv_inc/dec_in_flight() */
+static void coroutine_fn bdrv_make_zero_co_entry(void *opaque)
+{
+BdrvDoMakeZeroData *data = opaque;
+
+data->ret = bdrv_do_make_zero(data->child, data->flags);
+data->done = true;
+aio_wait_kick();
+}
+
+int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
+{
+int ret;
+
+bdrv_inc_in_flight(child->bs);
+
+if (qemu_in_coroutine()) {
+/* Fast-path if already in coroutine context */
+ret = bdrv_do_make_zero(child, flags);
+} else {
+BdrvDoMakeZeroData data = {
+.child = child,
+.flags = flags,
+.done = false,
+};
+Coroutine *co = qemu_coroutine_create(bdrv_make_zero_co_entry, );
+
+bdrv_coroutine_enter(child->bs, co);
+BDRV_POLL_WHILE(child->bs, !data.done);
+ret = data.ret;
+}
+
+bdrv_dec_in_flight(child->bs);
+
+return ret;
+}
+
 typedef struct BdrvVmstateCo {
 BlockDriverState*bs;
 QEMUIOVector*qiov;
-- 
2.21.0