Re: [Qemu-devel] bug introduced by "block: Move throttling fields from BDS to BB"

[Kevin Wolf]

Am 17.10.2016 um 10:49 hat Alberto Garcia geschrieben:
> On Fri 14 Oct 2016 04:11:46 PM CEST, Paolo Bonzini wrote:
> > Here is next_throttle_token:
> >
> > -ThrottleGroup *tg = container_of(blk_bs(blk)->throttle_state,
> > - ThrottleGroup, ts);
> > +BlockBackendPublic *blkp = blk_get_public(blk);
> > +ThrottleGroup *tg = container_of(blkp->throttle_state, ThrottleGroup, 
> > ts);
> >  BlockBackend *token, *start;
> >  
> >  start = token = tg->tokens[is_write];
> >  
> >  /* get next bs round in round robin style */
> >  token = throttle_group_next_blk(token);
> > -while (token != start && !blk_bs(token)->pending_reqs[is_write]) {
> > +while (token != start && !blkp->pending_reqs[is_write]) {
> >  token = throttle_group_next_blk(token);
> >  }
> >
> >
> > blkp isn't updated every time token is updated.
> 
> You're right, I'll write a patch. I'd also try to check why this was not
> detected by any iotest.
> 
> Thanks!

Thanks a lot, Berto! (Both for fixing my bug and thinking of test cases)

Kevin

Re: [Qemu-devel] bug introduced by "block: Move throttling fields from BDS to BB"

[Alberto Garcia]

On Fri 14 Oct 2016 04:11:46 PM CEST, Paolo Bonzini wrote:
> Here is next_throttle_token:
>
> -ThrottleGroup *tg = container_of(blk_bs(blk)->throttle_state,
> - ThrottleGroup, ts);
> +BlockBackendPublic *blkp = blk_get_public(blk);
> +ThrottleGroup *tg = container_of(blkp->throttle_state, ThrottleGroup, 
> ts);
>  BlockBackend *token, *start;
>  
>  start = token = tg->tokens[is_write];
>  
>  /* get next bs round in round robin style */
>  token = throttle_group_next_blk(token);
> -while (token != start && !blk_bs(token)->pending_reqs[is_write]) {
> +while (token != start && !blkp->pending_reqs[is_write]) {
>  token = throttle_group_next_blk(token);
>  }
>
>
> blkp isn't updated every time token is updated.

You're right, I'll write a patch. I'd also try to check why this was not
detected by any iotest.

Thanks!

Berto

Re: [Qemu-devel] bug introduced by "block: Move throttling fields from BDS to BB"

[Paolo Bonzini]

On 14/10/2016 16:11, Paolo Bonzini wrote:
> -ThrottleGroup *tg = container_of(blk_bs(blk)->throttle_state,
> - ThrottleGroup, ts);
> +BlockBackendPublic *blkp = blk_get_public(blk);
> +ThrottleGroup *tg = container_of(blkp->throttle_state, ThrottleGroup, 
> ts);
>  BlockBackend *token, *start;
>  
>  start = token = tg->tokens[is_write];
>  
>  /* get next bs round in round robin style */
>  token = throttle_group_next_blk(token);
> -while (token != start && !blk_bs(token)->pending_reqs[is_write]) {
> +while (token != start && !blkp->pending_reqs[is_write]) {
>  token = throttle_group_next_blk(token);
>  }
> 
> 
> blkp isn't updated every time token is updated.

BTW, the simplest fix is probably to introduce a function

static inline bool blk_has_pending_reqs(BlockBackend *blk,
bool is_write)

Paolo

[Qemu-devel] bug introduced by "block: Move throttling fields from BDS to BB"

[Paolo Bonzini]

Here is next_throttle_token:

-ThrottleGroup *tg = container_of(blk_bs(blk)->throttle_state,
- ThrottleGroup, ts);
+BlockBackendPublic *blkp = blk_get_public(blk);
+ThrottleGroup *tg = container_of(blkp->throttle_state, ThrottleGroup, ts);
 BlockBackend *token, *start;
 
 start = token = tg->tokens[is_write];
 
 /* get next bs round in round robin style */
 token = throttle_group_next_blk(token);
-while (token != start && !blk_bs(token)->pending_reqs[is_write]) {
+while (token != start && !blkp->pending_reqs[is_write]) {
 token = throttle_group_next_blk(token);
 }


blkp isn't updated every time token is updated.

The same bug occurs in schedule_next_request.  This patch was only
in 2.7.x.

Thanks,

Paolo