Re: [PATCH v8 2/4] block: add the command line support

[Kevin Wolf <kwolf@redhat.com>]

Am 08.09.2011 12:11, schrieb Zhi Yong Wu:
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> ---
>  block.c         |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  block.h         |    5 ++++
>  block_int.h     |    3 ++
>  blockdev.c      |   29 +++++++++++++++++++++++++++
>  qemu-config.c   |   24 ++++++++++++++++++++++
>  qemu-options.hx |    1 +
>  6 files changed, 121 insertions(+), 0 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 43742b7..cd75183 100644
> --- a/block.c
> +++ b/block.c
> @@ -104,6 +104,57 @@ int is_windows_drive(const char *filename)
>  }
>  #endif
>  
> +/* throttling disk I/O limits */
> +void bdrv_io_limits_disable(BlockDriverState *bs)
> +{
> +    bs->io_limits_enabled = false;
> +
> +    if (bs->block_queue) {
> +        qemu_block_queue_flush(bs->block_queue);
> +        qemu_del_block_queue(bs->block_queue);
> +        bs->block_queue = NULL;
> +    }
> +
> +    if (bs->block_timer) {
> +        qemu_del_timer(bs->block_timer);
> +        qemu_free_timer(bs->block_timer);
> +        bs->block_timer     = NULL;
> +    }
> +
> +    bs->slice_start = 0;
> +
> +    bs->slice_end   = 0;

Remove the empty line between slice_start and slice_end?

> +}
> +
> +static void bdrv_block_timer(void *opaque)
> +{
> +    BlockDriverState *bs = opaque;
> +    BlockQueue *queue    = bs->block_queue;
> +
> +    qemu_block_queue_flush(queue);

Hm, didn't really notice it while reading patch 1, but
qemu_block_queue_flush() is misleading. It's really something like
qemu_block_queue_submit().

> +}
> +
> +void bdrv_io_limits_enable(BlockDriverState *bs)
> +{
> +    bs->block_queue = qemu_new_block_queue();
> +    bs->block_timer = qemu_new_timer_ns(vm_clock, bdrv_block_timer, bs);
> +
> +    bs->slice_start = qemu_get_clock_ns(vm_clock);
> +
> +    bs->slice_end   = bs->slice_start + BLOCK_IO_SLICE_TIME;
> +}

Same as above.

> +
> +bool bdrv_io_limits_enabled(BlockDriverState *bs)
> +{
> +    BlockIOLimit *io_limits = &bs->io_limits;
> +    return io_limits->bps[BLOCK_IO_LIMIT_READ]
> +         || io_limits->bps[BLOCK_IO_LIMIT_WRITE]
> +         || io_limits->bps[BLOCK_IO_LIMIT_TOTAL]
> +         || io_limits->iops[BLOCK_IO_LIMIT_READ]
> +         || io_limits->iops[BLOCK_IO_LIMIT_WRITE]
> +         || io_limits->iops[BLOCK_IO_LIMIT_TOTAL];
> +}
> +
>  /* check if the path starts with "<protocol>:" */
>  static int path_has_protocol(const char *path)
>  {
> @@ -1453,6 +1504,14 @@ void bdrv_get_geometry_hint(BlockDriverState *bs,
>      *psecs = bs->secs;
>  }
>  
> +/* throttling disk io limits */
> +void bdrv_set_io_limits(BlockDriverState *bs,
> +                            BlockIOLimit *io_limits)
> +{
> +    bs->io_limits = *io_limits;
> +    bs->io_limits_enabled = bdrv_io_limits_enabled(bs);
> +}
> +
>  /* Recognize floppy formats */
>  typedef struct FDFormat {
>      FDriveType drive;
> diff --git a/block.h b/block.h
> index 3ac0b94..a3e69db 100644
> --- a/block.h
> +++ b/block.h
> @@ -58,6 +58,11 @@ void bdrv_info(Monitor *mon, QObject **ret_data);
>  void bdrv_stats_print(Monitor *mon, const QObject *data);
>  void bdrv_info_stats(Monitor *mon, QObject **ret_data);
>  
> +/* disk I/O throttling */
> +void bdrv_io_limits_enable(BlockDriverState *bs);
> +void bdrv_io_limits_disable(BlockDriverState *bs);
> +bool bdrv_io_limits_enabled(BlockDriverState *bs);
> +
>  void bdrv_init(void);
>  void bdrv_init_with_whitelist(void);
>  BlockDriver *bdrv_find_protocol(const char *filename);
> diff --git a/block_int.h b/block_int.h
> index 201e635..368c776 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -257,6 +257,9 @@ void qemu_aio_release(void *p);
>  
>  void *qemu_blockalign(BlockDriverState *bs, size_t size);
>  
> +void bdrv_set_io_limits(BlockDriverState *bs,
> +                            BlockIOLimit *io_limits);
> +
>  #ifdef _WIN32
>  int is_windows_drive(const char *filename);
>  #endif
> diff --git a/blockdev.c b/blockdev.c
> index 2602591..619ae9f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -236,6 +236,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>      int on_read_error, on_write_error;
>      const char *devaddr;
>      DriveInfo *dinfo;
> +    BlockIOLimit io_limits;
>      int snapshot = 0;
>      int ret;
>  
> @@ -354,6 +355,31 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>          }
>      }
>  
> +    /* disk I/O throttling */
> +    io_limits.bps[BLOCK_IO_LIMIT_TOTAL]  =
> +                           qemu_opt_get_number(opts, "bps", 0);
> +    io_limits.bps[BLOCK_IO_LIMIT_READ]   =
> +                           qemu_opt_get_number(opts, "bps_rd", 0);
> +    io_limits.bps[BLOCK_IO_LIMIT_WRITE]  =
> +                           qemu_opt_get_number(opts, "bps_wr", 0);
> +    io_limits.iops[BLOCK_IO_LIMIT_TOTAL] =
> +                           qemu_opt_get_number(opts, "iops", 0);
> +    io_limits.iops[BLOCK_IO_LIMIT_READ]  =
> +                           qemu_opt_get_number(opts, "iops_rd", 0);
> +    io_limits.iops[BLOCK_IO_LIMIT_WRITE] =
> +                           qemu_opt_get_number(opts, "iops_wr", 0);
> +
> +    if (((io_limits.bps[BLOCK_IO_LIMIT_TOTAL] != 0)
> +            && ((io_limits.bps[BLOCK_IO_LIMIT_READ] != 0)
> +            || (io_limits.bps[BLOCK_IO_LIMIT_WRITE] != 0)))
> +            || ((io_limits.iops[BLOCK_IO_LIMIT_TOTAL] != 0)
> +            && ((io_limits.iops[BLOCK_IO_LIMIT_READ] != 0)
> +            || (io_limits.iops[BLOCK_IO_LIMIT_WRITE] != 0)))) {

-EWRITEONLY

Seriously, break this up into some temporary bool variables if you want
it to be readable.

> +        error_report("bps(iops) and bps_rd/bps_wr(iops_rd/iops_wr)"
> +                     "cannot be used at the same time");
> +        return NULL;
> +    }
> +
>      on_write_error = BLOCK_ERR_STOP_ENOSPC;
>      if ((buf = qemu_opt_get(opts, "werror")) != NULL) {
>          if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type != IF_NONE) {
> @@ -461,6 +487,9 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>  
>      bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error);
>  
> +    /* disk I/O throttling */
> +    bdrv_set_io_limits(dinfo->bdrv, &io_limits);
> +
>      switch(type) {
>      case IF_IDE:
>      case IF_SCSI:
> diff --git a/qemu-config.c b/qemu-config.c
> index 7a7854f..405e587 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -85,6 +85,30 @@ static QemuOptsList qemu_drive_opts = {
>              .name = "readonly",
>              .type = QEMU_OPT_BOOL,
>              .help = "open drive file as read-only",
> +        },{
> +            .name = "iops",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit total I/O operations per second",
> +        },{
> +            .name = "iops_rd",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit read operations per second",
> +        },{
> +            .name = "iops_wr",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit write operations per second",
> +        },{
> +            .name = "bps",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit total bytes per second",
> +        },{
> +            .name = "bps_rd",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit read bytes per second",
> +        },{
> +            .name = "bps_wr",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit write bytes per second",
>          },
>          { /* end of list */ }
>      },
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 659ecb2..2e42c5c 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -136,6 +136,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
>      "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
>      "       [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
>      "       [,readonly=on|off]\n"
> +    "       [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]][[,iops=i]|[[,iops_rd=r][,iops_wr=w]]\n"
>      "                use 'file' as a drive image\n", QEMU_ARCH_ALL)
>  STEXI
>  @item -drive @var{option}[,@var{option}[,@var{option}[,...]]]

Kevin