From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58836) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XFf7A-00007H-Bl for qemu-devel@nongnu.org; Fri, 08 Aug 2014 04:01:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XFf75-0001lZ-Mh for qemu-devel@nongnu.org; Fri, 08 Aug 2014 04:01:28 -0400 Received: from mx4-phx2.redhat.com ([209.132.183.25]:40757) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XFf75-0001lJ-Et for qemu-devel@nongnu.org; Fri, 08 Aug 2014 04:01:23 -0400 Date: Fri, 8 Aug 2014 04:01:19 -0400 (EDT) From: Francesco Romani Message-ID: <1841263794.17835817.1407484879020.JavaMail.zimbra@redhat.com> In-Reply-To: <20140805130846.GA12251@stefanha-thinkpad.redhat.com> References: <1404830964-10733-1-git-send-email-fromani@redhat.com> <1404830964-10733-2-git-send-email-fromani@redhat.com> <20140801113940.GC7258@stefanha-thinkpad.redhat.com> <20140805084757.GB4391@noname.str.redhat.com> <20140805130846.GA12251@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] block: add watermark event List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , lcapitulino@redhat.com, mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org ----- Original Message ----- > From: "Stefan Hajnoczi" > To: "Kevin Wolf" > Cc: mdroth@linux.vnet.ibm.com, "Francesco Romani" , qemu-devel@nongnu.org, lcapitulino@redhat.com > Sent: Tuesday, August 5, 2014 3:08:46 PM > Subject: Re: [Qemu-devel] [PATCH] block: add watermark event > > On Tue, Aug 05, 2014 at 10:47:57AM +0200, Kevin Wolf wrote: > > Am 01.08.2014 um 13:39 hat Stefan Hajnoczi geschrieben: > > > On Tue, Jul 08, 2014 at 04:49:24PM +0200, Francesco Romani wrote: > > > > @@ -5813,3 +5815,57 @@ void bdrv_flush_io_queue(BlockDriverState *bs) > > > > bdrv_flush_io_queue(bs->file); > > > > } > > > > } > > > > + > > > > +static bool watermark_exceeded(BlockDriverState *bs, > > > > + int64_t sector_num, > > > > + int nb_sectors) > > > > +{ > > > > + > > > > + if (bs->wr_watermark_perc > 0) { > > > > + int64_t watermark = (bs->total_sectors) / 100 * > > > > bs->wr_watermark_perc; > > > > > > bs->total_sectors should not be used directly. > > > > > > Have you considered making the watermark parameter take sector units > > > instead of a percentage? > > > > > > I'm not sure whether a precentage makes sense because 25% of a 10GB > > > image is 2.5 GB so a 75% watermark might be reasonable. 25% of a 1 TB > > > image is 250 GB and that's probably not a reasonable watermark. > > > > > > So let the block-set-watermark caller pass an absolute sector number > > > instead. It keeps things simple for both QEMU and thin provisioning > > > manager. > > > > No sector numbers in external interfaces, please. These units of 512 > > bytes are completely arbitrary and don't make any sense. I hope to get > > rid of BDRV_SECTOR_* eventually even internally. > > > > So for external APIs, please use bytes instead. > > I agree and forgot about that. Please use bytes instead of sectors or a > percentage. > Thanks everyone for the great feedback received! I'll post asap a new patch addressing all the comments. I'll also change the name because 'watermark' may be misleading/wrong jargon :) (http://en.wikipedia.org/wiki/Watermark) Bests, -- Francesco Romani RedHat Engineering Virtualization R & D Phone: 8261328 IRC: fromani