From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:33867) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QrNvL-0006sY-Bv for qemu-devel@nongnu.org; Thu, 11 Aug 2011 01:35:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QrNvG-0000Wz-46 for qemu-devel@nongnu.org; Thu, 11 Aug 2011 01:35:19 -0400 Received: from mtagate7.uk.ibm.com ([194.196.100.167]:45322) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QrNvF-0000Wb-PU for qemu-devel@nongnu.org; Thu, 11 Aug 2011 01:35:14 -0400 Received: from d06nrmr1307.portsmouth.uk.ibm.com (d06nrmr1307.portsmouth.uk.ibm.com [9.149.38.129]) by mtagate7.uk.ibm.com (8.13.1/8.13.1) with ESMTP id p7B5ZBoA016948 for ; Thu, 11 Aug 2011 05:35:11 GMT Received: from d06av06.portsmouth.uk.ibm.com (d06av06.portsmouth.uk.ibm.com [9.149.37.217]) by d06nrmr1307.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p7B5Z3BA2494714 for ; Thu, 11 Aug 2011 06:35:11 +0100 Received: from d06av06.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av06.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p7B5Z3ml030891 for ; Wed, 10 Aug 2011 23:35:03 -0600 Date: Thu, 11 Aug 2011 06:35:01 +0100 From: Stefan Hajnoczi Message-ID: <20110811053501.GA27771@stefanha-thinkpad.localdomain> References: <1312863472-6901-1-git-send-email-wuzhy@linux.vnet.ibm.com> <1312863472-6901-2-git-send-email-wuzhy@linux.vnet.ibm.com> <20110810092737.GC28852@stefanha-thinkpad.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v5 1/4] block: add the command line support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Zhi Yong Wu Cc: kwolf@redhat.com, pair@us.ibm.com, kvm@vger.kernel.org, Stefan Hajnoczi , mtosatti@redhat.com, qemu-devel@nongnu.org, ryanh@us.ibm.com, Zhi Yong Wu , luowenj@cn.ibm.com 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 > 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 = wrote: > >> > On Tue, Aug 9, 2011 at 5:17 AM, Zhi Yong Wu wrote: > >> >> Signed-off-by: Zhi Yong Wu > >> >> --- > >> >> =A0Makefile.objs =A0 | =A0 =A02 +- > >> >> =A0blockdev.c =A0 =A0 =A0| =A0 39 +++++++++++++++++++++++++++++++= ++++++++ > >> >> =A0qemu-config.c =A0 | =A0 24 ++++++++++++++++++++++++ > >> >> =A0qemu-option.c =A0 | =A0 17 +++++++++++++++++ > >> >> =A0qemu-option.h =A0 | =A0 =A01 + > >> >> =A0qemu-options.hx | =A0 =A01 + > >> >> =A06 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 +=3D raw.o cow.o qcow.o vdi.o vm= dk.o cloop.o dmg.o bochs.o vpc.o vv > >> >> =A0block-nested-y +=3D qcow2.o qcow2-refcount.o qcow2-cluster.o q= cow2-snapshot.o qcow2-cache.o > >> >> =A0block-nested-y +=3D qed.o qed-gencb.o qed-l2-cache.o qed-table= .o qed-cluster.o > >> >> =A0block-nested-y +=3D qed-check.o > >> >> -block-nested-y +=3D parallels.o nbd.o blkdebug.o sheepdog.o blkv= erify.o > >> >> +block-nested-y +=3D parallels.o nbd.o blkdebug.o sheepdog.o blkv= erify.o blk-queue.o > >> > > >> > This does not build: > >> > =A0LINK =A0qemu-ga > >> > gcc: error: block/blk-queue.o: No such file or directory > >> > > >> > This Makefile.objs change should be in the commit that adds blk-qu= eue.c. > >> > > >> > Each patch in a series should compile cleanly and can only depend = on > >> > previous patches. =A0This is important so that git-bisect(1) can b= e > >> > used, it only works if every commit builds a working program. =A0I= t also > >> > makes patch review easier when the patch series builds up logicall= y. > >> 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 u= p > > patch series. > > > > It also becomes easier once you plan to write patches that follow the= se > > guidelines. > OK > > > >> >> + =A0 =A0/* disk io throttling */ > >> >> + =A0 =A0iol_flag =3D qemu_opt_io_limits_enable_flag(opts, iol_op= ts); > >> >> + =A0 =A0if (iol_flag) { > >> >> + =A0 =A0 =A0 =A0memset(&io_limits, 0, sizeof(BlockIOLimit)); > >> >> + > >> >> + =A0 =A0 =A0 =A0io_limits.bps[BLOCK_IO_LIMIT_TOTAL] =A0=3D > >> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 qemu_opt_ge= t_number(opts, "bps", 0); > >> >> + =A0 =A0 =A0 =A0io_limits.bps[BLOCK_IO_LIMIT_READ] =A0 =3D > >> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 qemu_opt_ge= t_number(opts, "bps_rd", 0); > >> >> + =A0 =A0 =A0 =A0io_limits.bps[BLOCK_IO_LIMIT_WRITE] =A0=3D > >> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 qemu_opt_ge= t_number(opts, "bps_wr", 0); > >> >> + =A0 =A0 =A0 =A0io_limits.iops[BLOCK_IO_LIMIT_TOTAL] =3D > >> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 qemu_opt_ge= t_number(opts, "iops", 0); > >> >> + =A0 =A0 =A0 =A0io_limits.iops[BLOCK_IO_LIMIT_READ] =A0=3D > >> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 qemu_opt_ge= t_number(opts, "iops_rd", 0); > >> >> + =A0 =A0 =A0 =A0io_limits.iops[BLOCK_IO_LIMIT_WRITE] =3D > >> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 qemu_opt_ge= t_number(opts, "iops_wr", 0); > >> >> + > >> >> + =A0 =A0 =A0 =A0if (((io_limits.bps[BLOCK_IO_LIMIT_TOTAL] !=3D 0= ) > >> >> + =A0 =A0 =A0 =A0 =A0 =A0&& ((io_limits.bps[BLOCK_IO_LIMIT_READ] = !=3D 0) > >> >> + =A0 =A0 =A0 =A0 =A0 =A0|| (io_limits.bps[BLOCK_IO_LIMIT_WRITE] = !=3D 0))) > >> >> + =A0 =A0 =A0 =A0 =A0 =A0|| ((io_limits.iops[BLOCK_IO_LIMIT_TOTAL= ] !=3D 0) > >> >> + =A0 =A0 =A0 =A0 =A0 =A0&& ((io_limits.iops[BLOCK_IO_LIMIT_READ]= !=3D 0) > >> >> + =A0 =A0 =A0 =A0 =A0 =A0|| (io_limits.iops[BLOCK_IO_LIMIT_WRITE]= !=3D 0)))) { > >> >> + =A0 =A0 =A0 =A0 =A0 =A0error_report("bps(iops) and bps_rd/bps_w= r(iops_rd/iops_wr) \ > >> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 =A0cannot be used at the same time"); > >> >> + =A0 =A0 =A0 =A0 =A0 =A0return NULL; > >> >> + =A0 =A0 =A0 =A0} > >> >> + =A0 =A0} > >> >> + > >> >> =A0 =A0 on_write_error =3D BLOCK_ERR_STOP_ENOSPC; > >> >> =A0 =A0 if ((buf =3D qemu_opt_get(opts, "werror")) !=3D NULL) { > >> >> =A0 =A0 =A0 =A0 if (type !=3D IF_IDE && type !=3D IF_SCSI && type= !=3D IF_VIRTIO && type !=3D IF_NONE) { > >> >> @@ -483,6 +517,11 @@ DriveInfo *drive_init(QemuOpts *opts, int de= fault_to_scsi) > >> >> > >> >> =A0 =A0 bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_er= ror); > >> >> > >> >> + =A0 =A0/* disk I/O throttling */ > >> >> + =A0 =A0if (iol_flag) { > >> >> + =A0 =A0 =A0 =A0bdrv_set_io_limits(dinfo->bdrv, &io_limits); > >> >> + =A0 =A0} > >> > > >> > iol_flag and qemu_opt_io_limits_enable_flag() are not necessary. =A0= 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 necess= ay here. > > > > There are two possible cases: the user does not set any options or th= e > > user sets at least one option. =A0In 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, whic= h > > means "unlimited". =A0bdrv_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. =A0The 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