Re: [Qemu-devel] [PATCH V5 3/5] block: Enable the new throttling code in the block layer.

[Fam Zheng <famz@redhat.com>]

On Wed, 08/14 17:31, Fam Zheng wrote:
> On Mon, 08/12 18:53, Benoît Canet wrote:
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > @@ -1262,7 +1260,7 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
> >                                 int64_t bps_wr, int64_t iops, int64_t iops_rd,
> >                                 int64_t iops_wr, Error **errp)
> >  {
> > -    BlockIOLimit io_limits;
> > +    ThrottleConfig cfg;
> >      BlockDriverState *bs;
> >  
> >      bs = bdrv_find(device);
> > @@ -1271,27 +1269,37 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
> >          return;
> >      }
> >  
> > -    io_limits.bps[BLOCK_IO_LIMIT_TOTAL] = bps;
> > -    io_limits.bps[BLOCK_IO_LIMIT_READ]  = bps_rd;
> > -    io_limits.bps[BLOCK_IO_LIMIT_WRITE] = bps_wr;
> > -    io_limits.iops[BLOCK_IO_LIMIT_TOTAL]= iops;
> > -    io_limits.iops[BLOCK_IO_LIMIT_READ] = iops_rd;
> > -    io_limits.iops[BLOCK_IO_LIMIT_WRITE]= iops_wr;
> > +    cfg.buckets[THROTTLE_BPS_TOTAL].ups = bps;
> > +    cfg.buckets[THROTTLE_BPS_READ].ups  = bps_rd;
> > +    cfg.buckets[THROTTLE_BPS_WRITE].ups = bps_wr;
> > +
> > +    cfg.buckets[THROTTLE_OPS_TOTAL].ups = iops;
> > +    cfg.buckets[THROTTLE_OPS_READ].ups  = iops_rd;
> > +    cfg.buckets[THROTTLE_OPS_WRITE].ups = iops_wr;
> > +
> > +    cfg.buckets[THROTTLE_BPS_TOTAL].max = 0;
> > +    cfg.buckets[THROTTLE_BPS_READ].max  = 0;
> > +    cfg.buckets[THROTTLE_BPS_WRITE].max = 0;
> > +
> > +    cfg.buckets[THROTTLE_OPS_TOTAL].max = 0;
> > +    cfg.buckets[THROTTLE_OPS_READ].max  = 0;
> > +    cfg.buckets[THROTTLE_OPS_WRITE].max = 0;
> >  
> > -    if (!do_check_io_limits(&io_limits, errp)) {
> > +    cfg.unit_size = BDRV_SECTOR_SIZE;
> 
> Does this mean user set bps limit in sector now? I think we should be
> consistent to existing parameter unit.
> 
> > +    cfg.op_size = 0;
> 
> Why not set op_size to 1?
> 
Never mind. You se it to 0 here and it's the condition of setting
unit=1.

Fam