Re: [Qemu-devel] [PATCH v5 1/4] block: add the command line support

[Stefan Hajnoczi]

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/

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