[Qemu-devel] [RFC] block-backend: fix double inc/dec inflight requests number

[Qemu-devel] [RFC] block-backend: fix double inc/dec inflight requests number

[Vladimir Sementsov-Ogievskiy]

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

Re: [Qemu-devel] [Qemu-block] [RFC] block-backend: fix double inc/dec inflight requests number

[Stefan Hajnoczi]

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

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

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

Re: [Qemu-devel] [Qemu-block] [RFC] block-backend: fix double inc/dec inflight requests number

[Kevin Wolf]

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

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

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