blk_co_preadv and blk_co_pwritev calls bdrv_inc_in_flight and
bdrv_dec_in_flight. But they calls correspondingly bdrv_co_preadv
and bdrv_co_pwritev, which inc/dec in-flight count by themselves.
Counting in-flight requests in blk_ layer is redundant, drop it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
Hi all!
Is it a bug or a feature? Why do we call inc/dec twice for read/write?
We don't do this for flush and discard..
(patch is called RFC, but it may be applied as is, if it is OK)
block/block-backend.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index baef8e7abc..8219f59d93 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1094,17 +1094,13 @@ int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
return ret;
}
- bdrv_inc_in_flight(bs);
-
/* throttling disk I/O */
if (blk->public.throttle_group_member.throttle_state) {
throttle_group_co_io_limits_intercept(&blk->public.throttle_group_member,
bytes, false);
}
- ret = bdrv_co_preadv(blk->root, offset, bytes, qiov, flags);
- bdrv_dec_in_flight(bs);
- return ret;
+ return bdrv_co_preadv(blk->root, offset, bytes, qiov, flags);
}
int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
@@ -1121,7 +1117,6 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
return ret;
}
- bdrv_inc_in_flight(bs);
/* throttling disk I/O */
if (blk->public.throttle_group_member.throttle_state) {
throttle_group_co_io_limits_intercept(&blk->public.throttle_group_member,
@@ -1132,9 +1127,7 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
flags |= BDRV_REQ_FUA;
}
- ret = bdrv_co_pwritev(blk->root, offset, bytes, qiov, flags);
- bdrv_dec_in_flight(bs);
- return ret;
+ return bdrv_co_pwritev(blk->root, offset, bytes, qiov, flags);
}
typedef struct BlkRwCo {
--
2.11.1
On Mon, Jan 22, 2018 at 05:45:49PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Is it a bug or a feature? Why do we call inc/dec twice for read/write? > We don't do this for flush and discard.. It's non-obvious and I asked Paolo the same question previously. > - bdrv_inc_in_flight(bs); > - > /* throttling disk I/O */ > if (blk->public.throttle_group_member.throttle_state) { > throttle_group_co_io_limits_intercept(&blk->public.throttle_group_member, > bytes, false); > } ^^^ HINT HINT HINT ^^^ > > - ret = bdrv_co_preadv(blk->root, offset, bytes, qiov, flags); > - bdrv_dec_in_flight(bs); > - return ret; > + return bdrv_co_preadv(blk->root, offset, bytes, qiov, flags); The problem is what happens if the request is throttled? Even throttled requests must be counted so that bdrv_drain() and friends work. It may be possible to eliminate this now that throttling is a BDS node. It used to be implemented as a completely separate API outside the BDS node graph. Now there is a throttling node in the graph, so maybe we can stop taking the extra reference but some refactoring may be necessary. Stefan
Am 25.01.2018 um 12:37 hat Stefan Hajnoczi geschrieben: > On Mon, Jan 22, 2018 at 05:45:49PM +0300, Vladimir Sementsov-Ogievskiy wrote: > > Is it a bug or a feature? Why do we call inc/dec twice for read/write? > > We don't do this for flush and discard.. > > It's non-obvious and I asked Paolo the same question previously. > > > - bdrv_inc_in_flight(bs); > > - > > /* throttling disk I/O */ > > if (blk->public.throttle_group_member.throttle_state) { > > throttle_group_co_io_limits_intercept(&blk->public.throttle_group_member, > > bytes, false); > > } > > ^^^ HINT HINT HINT ^^^ > > > > > - ret = bdrv_co_preadv(blk->root, offset, bytes, qiov, flags); > > - bdrv_dec_in_flight(bs); > > - return ret; > > + return bdrv_co_preadv(blk->root, offset, bytes, qiov, flags); > > The problem is what happens if the request is throttled? > > Even throttled requests must be counted so that bdrv_drain() and friends > work. > > It may be possible to eliminate this now that throttling is a BDS node. > It used to be implemented as a completely separate API outside the BDS > node graph. Now there is a throttling node in the graph, so maybe we > can stop taking the extra reference but some refactoring may be > necessary. The patches to use a throttling node even when the legacy options are used have not been merged yet. Kevin
© 2016 - 2024 Red Hat, Inc.