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@xxxxxxxxx> wrote: > > On Tue, Aug 9, 2011 at 5:17 AM, Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx> wrote: > >> Signed-off-by: Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx> > >> --- > >> 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
