[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 1/4] block: add the command line support
From: |
Zhi Yong Wu |
Subject: |
Re: [Qemu-devel] [PATCH v5 1/4] block: add the command line support |
Date: |
Thu, 11 Aug 2011 12:44:11 +0800 |
On Wed, Aug 10, 2011 at 5:27 PM, Stefan Hajnoczi
<address@hidden> 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 <address@hidden> wrote:
>> > On Tue, Aug 9, 2011 at 5:17 AM, Zhi Yong Wu <address@hidden> wrote:
>> >> Signed-off-by: Zhi Yong Wu <address@hidden>
>> >> ---
>> >> 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
>
--
Regards,
Zhi Yong Wu
[Qemu-devel] [PATCH v5 2/4] block: add the block queue support, Zhi Yong Wu, 2011/08/09
- Re: [Qemu-devel] [PATCH v5 2/4] block: add the block queue support, Ram Pai, 2011/08/09
- Re: [Qemu-devel] [PATCH v5 2/4] block: add the block queue support, Stefan Hajnoczi, 2011/08/09
- Re: [Qemu-devel] [PATCH v5 2/4] block: add the block queue support, Zhi Yong Wu, 2011/08/10
- Re: [Qemu-devel] [PATCH v5 2/4] block: add the block queue support, Stefan Hajnoczi, 2011/08/10
- Re: [Qemu-devel] [PATCH v5 2/4] block: add the block queue support, Zhi Yong Wu, 2011/08/11
- Re: [Qemu-devel] [PATCH v5 2/4] block: add the block queue support, Zhi Yong Wu, 2011/08/11
- Re: [Qemu-devel] [PATCH v5 2/4] block: add the block queue support, Zhi Yong Wu, 2011/08/12