Patchwork [v5,1/4] block: add the command line support

login
register
mail settings
Submitter Zhi Yong Wu
Date Aug. 9, 2011, 4:17 a.m.
Message ID <1312863472-6901-2-git-send-email-wuzhy@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/109123/
State New
Headers show

Comments

Zhi Yong Wu - Aug. 9, 2011, 4:17 a.m.
Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
 Makefile.objs   |    2 +-
 blockdev.c      |   39 +++++++++++++++++++++++++++++++++++++++
 qemu-config.c   |   24 ++++++++++++++++++++++++
 qemu-option.c   |   17 +++++++++++++++++
 qemu-option.h   |    1 +
 qemu-options.hx |    1 +
 6 files changed, 83 insertions(+), 1 deletions(-)
Stefan Hajnoczi - Aug. 9, 2011, 12:25 p.m.
On Tue, Aug 9, 2011 at 5:17 AM, Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> wrote:
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> ---
>  Makefile.objs   |    2 +-
>  blockdev.c      |   39 +++++++++++++++++++++++++++++++++++++++
>  qemu-config.c   |   24 ++++++++++++++++++++++++
>  qemu-option.c   |   17 +++++++++++++++++
>  qemu-option.h   |    1 +
>  qemu-options.hx |    1 +
>  6 files changed, 83 insertions(+), 1 deletions(-)
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 9f99ed4..06f2033 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -23,7 +23,7 @@ block-nested-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vv
>  block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
>  block-nested-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>  block-nested-y += qed-check.o
> -block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
> +block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o blk-queue.o

This does not build:
  LINK  qemu-ga
gcc: error: block/blk-queue.o: No such file or directory

This Makefile.objs change should be in the commit that adds blk-queue.c.

Each patch in a series should compile cleanly and can only depend on
previous patches.  This is important so that git-bisect(1) can be
used, it only works if every commit builds a working program.  It also
makes patch review easier when the patch series builds up logically.

>  block-nested-$(CONFIG_WIN32) += raw-win32.o
>  block-nested-$(CONFIG_POSIX) += raw-posix.o
>  block-nested-$(CONFIG_CURL) += curl.o
> diff --git a/blockdev.c b/blockdev.c
> index c263663..9c78548 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -238,6 +238,10 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>     int on_read_error, on_write_error;
>     const char *devaddr;
>     DriveInfo *dinfo;
> +    BlockIOLimit io_limits;

This structure is not undefined at this point in the patch series.

> +    bool iol_flag = false;
> +    const char *iol_opts[7] = {"bps", "bps_rd", "bps_wr",
> +                                "iops", "iops_rd", "iops_wr"};
>     int is_extboot = 0;
>     int snapshot = 0;
>     int ret;
> @@ -372,6 +376,36 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>         return NULL;
>     }
>
> +    /* disk io throttling */
> +    iol_flag = qemu_opt_io_limits_enable_flag(opts, iol_opts);
> +    if (iol_flag) {
> +        memset(&io_limits, 0, sizeof(BlockIOLimit));
> +
> +        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)))) {
> +            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) {
> @@ -483,6 +517,11 @@ 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 */
> +    if (iol_flag) {
> +        bdrv_set_io_limits(dinfo->bdrv, &io_limits);
> +    }

iol_flag and qemu_opt_io_limits_enable_flag() are not necessary.  If
no limits were set then all fields will be 0 (unlimited).

Stefan
Zhiyong Wu - Aug. 10, 2011, 5:20 a.m.
On Tue, Aug 9, 2011 at 8:25 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Aug 9, 2011 at 5:17 AM, Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> wrote:
>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> ---
>>  Makefile.objs   |    2 +-
>>  blockdev.c      |   39 +++++++++++++++++++++++++++++++++++++++
>>  qemu-config.c   |   24 ++++++++++++++++++++++++
>>  qemu-option.c   |   17 +++++++++++++++++
>>  qemu-option.h   |    1 +
>>  qemu-options.hx |    1 +
>>  6 files changed, 83 insertions(+), 1 deletions(-)
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 9f99ed4..06f2033 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -23,7 +23,7 @@ block-nested-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vv
>>  block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
>>  block-nested-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>>  block-nested-y += qed-check.o
>> -block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
>> +block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o blk-queue.o
>
> This does not build:
>  LINK  qemu-ga
> gcc: error: block/blk-queue.o: No such file or directory
>
> This Makefile.objs change should be in the commit that adds blk-queue.c.
>
> Each patch in a series should compile cleanly and can only depend on
> previous patches.  This is important so that git-bisect(1) can be
> used, it only works if every commit builds a working program.  It also
> makes patch review easier when the patch series builds up logically.
It seems that it will take a bit much time if we strictly stage the
hunks into each corresponding patch.:)
OK, i will.
>
>>  block-nested-$(CONFIG_WIN32) += raw-win32.o
>>  block-nested-$(CONFIG_POSIX) += raw-posix.o
>>  block-nested-$(CONFIG_CURL) += curl.o
>> diff --git a/blockdev.c b/blockdev.c
>> index c263663..9c78548 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -238,6 +238,10 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>     int on_read_error, on_write_error;
>>     const char *devaddr;
>>     DriveInfo *dinfo;
>> +    BlockIOLimit io_limits;
>
> This structure is not undefined at this point in the patch series.
The hunk contained this structure will be staged in this patch.

>
>> +    bool iol_flag = false;
>> +    const char *iol_opts[7] = {"bps", "bps_rd", "bps_wr",
>> +                                "iops", "iops_rd", "iops_wr"};
>>     int is_extboot = 0;
>>     int snapshot = 0;
>>     int ret;
>> @@ -372,6 +376,36 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>         return NULL;
>>     }
>>
>> +    /* disk io throttling */
>> +    iol_flag = qemu_opt_io_limits_enable_flag(opts, iol_opts);
>> +    if (iol_flag) {
>> +        memset(&io_limits, 0, sizeof(BlockIOLimit));
>> +
>> +        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)))) {
>> +            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) {
>> @@ -483,6 +517,11 @@ 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 */
>> +    if (iol_flag) {
>> +        bdrv_set_io_limits(dinfo->bdrv, &io_limits);
>> +    }
>
> iol_flag and qemu_opt_io_limits_enable_flag() are not necessary.  If
> no limits were set then all fields will be 0 (unlimited).
Are they not necessary here? why? qemu_opt_io_limits_enable_flag is
used to determine if io_limits is enabled.
If yes, iol_flag will be set to ONE. So i think that they are necessay here.
>
> Stefan
>
Stefan Hajnoczi - Aug. 10, 2011, 9:27 a.m.
On Wed, Aug 10, 2011 at 01:20:22PM +0800, Zhi Yong Wu wrote:
> On Tue, Aug 9, 2011 at 8:25 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Tue, Aug 9, 2011 at 5:17 AM, Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> wrote:
> >> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> >> ---
> >>  Makefile.objs   |    2 +-
> >>  blockdev.c      |   39 +++++++++++++++++++++++++++++++++++++++
> >>  qemu-config.c   |   24 ++++++++++++++++++++++++
> >>  qemu-option.c   |   17 +++++++++++++++++
> >>  qemu-option.h   |    1 +
> >>  qemu-options.hx |    1 +
> >>  6 files changed, 83 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/Makefile.objs b/Makefile.objs
> >> index 9f99ed4..06f2033 100644
> >> --- a/Makefile.objs
> >> +++ b/Makefile.objs
> >> @@ -23,7 +23,7 @@ block-nested-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vv
> >>  block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
> >>  block-nested-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
> >>  block-nested-y += qed-check.o
> >> -block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
> >> +block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o blk-queue.o
> >
> > This does not build:
> >  LINK  qemu-ga
> > gcc: error: block/blk-queue.o: No such file or directory
> >
> > This Makefile.objs change should be in the commit that adds blk-queue.c.
> >
> > Each patch in a series should compile cleanly and can only depend on
> > previous patches.  This is important so that git-bisect(1) can be
> > used, it only works if every commit builds a working program.  It also
> > makes patch review easier when the patch series builds up logically.
> It seems that it will take a bit much time if we strictly stage the
> hunks into each corresponding patch.:)
> OK, i will.

Some people like using Stacked Git to manage patch series:
http://www.procode.org/stgit/

I typically just use git rebase -i and git add -i manually to clean up
patch series.

It also becomes easier once you plan to write patches that follow these
guidelines.

> >> +    /* disk io throttling */
> >> +    iol_flag = qemu_opt_io_limits_enable_flag(opts, iol_opts);
> >> +    if (iol_flag) {
> >> +        memset(&io_limits, 0, sizeof(BlockIOLimit));
> >> +
> >> +        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)))) {
> >> +            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) {
> >> @@ -483,6 +517,11 @@ 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 */
> >> +    if (iol_flag) {
> >> +        bdrv_set_io_limits(dinfo->bdrv, &io_limits);
> >> +    }
> >
> > iol_flag and qemu_opt_io_limits_enable_flag() are not necessary.  If
> > no limits were set then all fields will be 0 (unlimited).
> Are they not necessary here? why? qemu_opt_io_limits_enable_flag is
> used to determine if io_limits is enabled.
> If yes, iol_flag will be set to ONE. So i think that they are necessay here.

There are two possible cases: the user does not set any options or the
user sets at least one option.  In both cases io_limits will be
initialized correctly, here is why:

When an option is not specified by the user the value will be 0, which
means "unlimited".  bdrv_set_io_limits() calls
bdrv_io_limits_enabled(bs) to check whether any I/O limit is non-zero.
This means the iol_flag check is already being done by
bdrv_set_io_limits() and there is no need to duplicate it.  The iol_flag
code can be eliminated and the program will behave the same.

The code would look something like this:

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)))) {
    error_report("bps(iops) and bps_rd/bps_wr(iops_rd/iops_wr) "
                 "cannot be used at the same time");
    return NULL;
}

bdrv_set_io_limits(dinfo->bdrv, &io_limits);

Stefan
Zhiyong Wu - Aug. 11, 2011, 4:44 a.m.
On Wed, Aug 10, 2011 at 5:27 PM, Stefan Hajnoczi
<stefanha@linux.vnet.ibm.com> wrote:
> On Wed, Aug 10, 2011 at 01:20:22PM +0800, Zhi Yong Wu wrote:
>> On Tue, Aug 9, 2011 at 8:25 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> > On Tue, Aug 9, 2011 at 5:17 AM, Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> wrote:
>> >> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> >> ---
>> >>  Makefile.objs   |    2 +-
>> >>  blockdev.c      |   39 +++++++++++++++++++++++++++++++++++++++
>> >>  qemu-config.c   |   24 ++++++++++++++++++++++++
>> >>  qemu-option.c   |   17 +++++++++++++++++
>> >>  qemu-option.h   |    1 +
>> >>  qemu-options.hx |    1 +
>> >>  6 files changed, 83 insertions(+), 1 deletions(-)
>> >>
>> >> diff --git a/Makefile.objs b/Makefile.objs
>> >> index 9f99ed4..06f2033 100644
>> >> --- a/Makefile.objs
>> >> +++ b/Makefile.objs
>> >> @@ -23,7 +23,7 @@ block-nested-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vv
>> >>  block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
>> >>  block-nested-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>> >>  block-nested-y += qed-check.o
>> >> -block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
>> >> +block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o blk-queue.o
>> >
>> > This does not build:
>> >  LINK  qemu-ga
>> > gcc: error: block/blk-queue.o: No such file or directory
>> >
>> > This Makefile.objs change should be in the commit that adds blk-queue.c.
>> >
>> > Each patch in a series should compile cleanly and can only depend on
>> > previous patches.  This is important so that git-bisect(1) can be
>> > used, it only works if every commit builds a working program.  It also
>> > makes patch review easier when the patch series builds up logically.
>> It seems that it will take a bit much time if we strictly stage the
>> hunks into each corresponding patch.:)
>> OK, i will.
>
> Some people like using Stacked Git to manage patch series:
> http://www.procode.org/stgit/
Let me try.
>
> I typically just use git rebase -i and git add -i manually to clean up
> patch series.
>
> It also becomes easier once you plan to write patches that follow these
> guidelines.
OK
>
>> >> +    /* disk io throttling */
>> >> +    iol_flag = qemu_opt_io_limits_enable_flag(opts, iol_opts);
>> >> +    if (iol_flag) {
>> >> +        memset(&io_limits, 0, sizeof(BlockIOLimit));
>> >> +
>> >> +        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)))) {
>> >> +            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) {
>> >> @@ -483,6 +517,11 @@ 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 */
>> >> +    if (iol_flag) {
>> >> +        bdrv_set_io_limits(dinfo->bdrv, &io_limits);
>> >> +    }
>> >
>> > iol_flag and qemu_opt_io_limits_enable_flag() are not necessary.  If
>> > no limits were set then all fields will be 0 (unlimited).
>> Are they not necessary here? why? qemu_opt_io_limits_enable_flag is
>> used to determine if io_limits is enabled.
>> If yes, iol_flag will be set to ONE. So i think that they are necessay here.
>
> There are two possible cases: the user does not set any options or the
> user sets at least one option.  In both cases io_limits will be
> initialized correctly, here is why:
>
> When an option is not specified by the user the value will be 0, which
> means "unlimited".  bdrv_set_io_limits() calls
> bdrv_io_limits_enabled(bs) to check whether any I/O limit is non-zero.
> This means the iol_flag check is already being done by
> bdrv_set_io_limits() and there is no need to duplicate it.  The iol_flag
> code can be eliminated and the program will behave the same.
>
> The code would look something like this:
If the user doesn't set any io_limits options, this chunk of codes are
also executed. anyway, that's OK. thanks.
>
> 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)))) {
>     error_report("bps(iops) and bps_rd/bps_wr(iops_rd/iops_wr) "
>                 "cannot be used at the same time");
>    return NULL;
> }
>
> bdrv_set_io_limits(dinfo->bdrv, &io_limits);
>
> Stefan
>
Stefan Hajnoczi - Aug. 11, 2011, 5:35 a.m.
On Thu, Aug 11, 2011 at 12:44:11PM +0800, Zhi Yong Wu wrote:
> On Wed, Aug 10, 2011 at 5:27 PM, Stefan Hajnoczi
> <stefanha@linux.vnet.ibm.com> wrote:
> > On Wed, Aug 10, 2011 at 01:20:22PM +0800, Zhi Yong Wu wrote:
> >> On Tue, Aug 9, 2011 at 8:25 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >> > On Tue, Aug 9, 2011 at 5:17 AM, Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> wrote:
> >> >> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> >> >> ---
> >> >>  Makefile.objs   |    2 +-
> >> >>  blockdev.c      |   39 +++++++++++++++++++++++++++++++++++++++
> >> >>  qemu-config.c   |   24 ++++++++++++++++++++++++
> >> >>  qemu-option.c   |   17 +++++++++++++++++
> >> >>  qemu-option.h   |    1 +
> >> >>  qemu-options.hx |    1 +
> >> >>  6 files changed, 83 insertions(+), 1 deletions(-)
> >> >>
> >> >> diff --git a/Makefile.objs b/Makefile.objs
> >> >> index 9f99ed4..06f2033 100644
> >> >> --- a/Makefile.objs
> >> >> +++ b/Makefile.objs
> >> >> @@ -23,7 +23,7 @@ block-nested-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vv
> >> >>  block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
> >> >>  block-nested-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
> >> >>  block-nested-y += qed-check.o
> >> >> -block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
> >> >> +block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o blk-queue.o
> >> >
> >> > This does not build:
> >> >  LINK  qemu-ga
> >> > gcc: error: block/blk-queue.o: No such file or directory
> >> >
> >> > This Makefile.objs change should be in the commit that adds blk-queue.c.
> >> >
> >> > Each patch in a series should compile cleanly and can only depend on
> >> > previous patches.  This is important so that git-bisect(1) can be
> >> > used, it only works if every commit builds a working program.  It also
> >> > makes patch review easier when the patch series builds up logically.
> >> It seems that it will take a bit much time if we strictly stage the
> >> hunks into each corresponding patch.:)
> >> OK, i will.
> >
> > Some people like using Stacked Git to manage patch series:
> > http://www.procode.org/stgit/
> Let me try.
> >
> > I typically just use git rebase -i and git add -i manually to clean up
> > patch series.
> >
> > It also becomes easier once you plan to write patches that follow these
> > guidelines.
> OK
> >
> >> >> +    /* disk io throttling */
> >> >> +    iol_flag = qemu_opt_io_limits_enable_flag(opts, iol_opts);
> >> >> +    if (iol_flag) {
> >> >> +        memset(&io_limits, 0, sizeof(BlockIOLimit));
> >> >> +
> >> >> +        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)))) {
> >> >> +            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) {
> >> >> @@ -483,6 +517,11 @@ 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 */
> >> >> +    if (iol_flag) {
> >> >> +        bdrv_set_io_limits(dinfo->bdrv, &io_limits);
> >> >> +    }
> >> >
> >> > iol_flag and qemu_opt_io_limits_enable_flag() are not necessary.  If
> >> > no limits were set then all fields will be 0 (unlimited).
> >> Are they not necessary here? why? qemu_opt_io_limits_enable_flag is
> >> used to determine if io_limits is enabled.
> >> If yes, iol_flag will be set to ONE. So i think that they are necessay here.
> >
> > There are two possible cases: the user does not set any options or the
> > user sets at least one option.  In both cases io_limits will be
> > initialized correctly, here is why:
> >
> > When an option is not specified by the user the value will be 0, which
> > means "unlimited".  bdrv_set_io_limits() calls
> > bdrv_io_limits_enabled(bs) to check whether any I/O limit is non-zero.
> > This means the iol_flag check is already being done by
> > bdrv_set_io_limits() and there is no need to duplicate it.  The iol_flag
> > code can be eliminated and the program will behave the same.
> >
> > The code would look something like this:
> If the user doesn't set any io_limits options, this chunk of codes are
> also executed. anyway, that's OK. thanks.

Yes, that's why qemu_opt_get_number() takes a default value.

Stefan

Patch

diff --git a/Makefile.objs b/Makefile.objs
index 9f99ed4..06f2033 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -23,7 +23,7 @@  block-nested-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vv
 block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
 block-nested-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-nested-y += qed-check.o
-block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
+block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o blk-queue.o
 block-nested-$(CONFIG_WIN32) += raw-win32.o
 block-nested-$(CONFIG_POSIX) += raw-posix.o
 block-nested-$(CONFIG_CURL) += curl.o
diff --git a/blockdev.c b/blockdev.c
index c263663..9c78548 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -238,6 +238,10 @@  DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
     int on_read_error, on_write_error;
     const char *devaddr;
     DriveInfo *dinfo;
+    BlockIOLimit io_limits;
+    bool iol_flag = false;
+    const char *iol_opts[7] = {"bps", "bps_rd", "bps_wr",
+                                "iops", "iops_rd", "iops_wr"};
     int is_extboot = 0;
     int snapshot = 0;
     int ret;
@@ -372,6 +376,36 @@  DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
         return NULL;
     }
 
+    /* disk io throttling */
+    iol_flag = qemu_opt_io_limits_enable_flag(opts, iol_opts);
+    if (iol_flag) {
+        memset(&io_limits, 0, sizeof(BlockIOLimit));
+
+        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)))) {
+            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) {
@@ -483,6 +517,11 @@  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 */
+    if (iol_flag) {
+        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 efa892c..9232bbb 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -82,6 +82,30 @@  static QemuOptsList qemu_drive_opts = {
             .name = "boot",
             .type = QEMU_OPT_BOOL,
             .help = "make this a boot drive",
+        },{
+            .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-option.c b/qemu-option.c
index 65db542..c5aa96a 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -562,6 +562,23 @@  uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval)
     return opt->value.uint;
 }
 
+bool qemu_opt_io_limits_enable_flag(QemuOpts *opts, const char **iol_opts)
+{
+    int i;
+    uint64_t opt_val = 0;
+    bool iol_flag    = false;
+
+    for (i = 0; iol_opts[i]; i++) {
+        opt_val = qemu_opt_get_number(opts, iol_opts[i], 0);
+        if (opt_val != 0) {
+            iol_flag = true;
+            break;
+        }
+    }
+
+    return iol_flag;
+}
+
 uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval)
 {
     QemuOpt *opt = qemu_opt_find(opts, name);
diff --git a/qemu-option.h b/qemu-option.h
index b515813..fc909f9 100644
--- a/qemu-option.h
+++ b/qemu-option.h
@@ -107,6 +107,7 @@  struct QemuOptsList {
 const char *qemu_opt_get(QemuOpts *opts, const char *name);
 int qemu_opt_get_bool(QemuOpts *opts, const char *name, int defval);
 uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval);
+bool qemu_opt_io_limits_enable_flag(QemuOpts *opts, const char **iol_opts);
 uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval);
 int qemu_opt_set(QemuOpts *opts, const char *name, const char *value);
 typedef int (*qemu_opt_loopfunc)(const char *name, const char *value, void *opaque);
diff --git a/qemu-options.hx b/qemu-options.hx
index cb3347e..ae219f5 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -121,6 +121,7 @@  DEF("drive", HAS_ARG, QEMU_OPTION_drive,
     "       [,cache=writethrough|writeback|none|unsafe][,format=f]\n"
     "       [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
     "       [,readonly=on|off][,boot=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}[,...]]]