| Message ID | 1312863472-6901-2-git-send-email-wuzhy@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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 >
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
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}[,...]]]
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(-)