From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35535) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YAceA-0003c3-De for qemu-devel@nongnu.org; Mon, 12 Jan 2015 05:55:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YAce5-0000Xe-SO for qemu-devel@nongnu.org; Mon, 12 Jan 2015 05:54:58 -0500 Received: from mx4-phx2.redhat.com ([209.132.183.25]:38260) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YAce5-0000X7-Kz for qemu-devel@nongnu.org; Mon, 12 Jan 2015 05:54:53 -0500 Date: Mon, 12 Jan 2015 05:54:49 -0500 (EST) From: Francesco Romani Message-ID: <1525282013.5895612.1421060089601.JavaMail.zimbra@redhat.com> In-Reply-To: <54B007D0.9050109@redhat.com> References: <1417683548-15845-1-git-send-email-fromani@redhat.com> <54B007D0.9050109@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4] block: add event when disk usage exceeds threshold List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, lcapitulino@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, mdroth@linux.vnet.ibm.com Hi, thanks for the review! ----- Original Message ----- > From: "Eric Blake" > To: "Francesco Romani" , qemu-devel@nongnu.org > Cc: kwolf@redhat.com, mdroth@linux.vnet.ibm.com, stefanha@redhat.com, lcapitulino@redhat.com > Sent: Friday, January 9, 2015 5:54:40 PM > Subject: Re: [Qemu-devel] [PATCH v4] block: add event when disk usage exceeds threshold > > On 12/04/2014 01:59 AM, Francesco Romani wrote: > > Managing applications, like oVirt (http://www.ovirt.org), make extensive > > use of thin-provisioned disk images. > > To let the guest run smoothly and be not unnecessarily paused, oVirt sets > > a disk usage threshold (so called 'high water mark') based on the > > occupation > > of the device, and automatically extends the image once the threshold > > is reached or exceeded. > > > > In order to detect the crossing of the threshold, oVirt has no choice but > > aggressively polling the QEMU monitor using the query-blockstats command. > > This lead to unnecessary system load, and is made even worse under scale: > > deployments with hundreds of VMs are no longer rare. > > > > To fix this, this patch adds: > > * A new monitor command to set a write threshold for a given block device. > > * A new event to report if a block device usage exceeds the threshold. > > Please also mention the names of those two things in the commit message, > to make it easier to find them when doing 'git log' archaeology. Sure, will do. > > This will allow the managing application to use smarter and more > > efficient monitoring, greatly reducing the need of polling. > > > > A followup patch is planned to allow to add the write threshold at > > device creation. > > > > Signed-off-by: Francesco Romani > > --- > > > --- /dev/null > > +++ b/block/write-threshold.c > > @@ -0,0 +1,125 @@ > > +/* > > + * QEMU System Emulator block write threshold notification > > + * > > + * Copyright Red Hat, Inc. 2014 > > I've been so slow on the review that you may want to add 2015. IANAL, but since most (~99%) of code was written in 2014, I'll just leave as that. > > +bool bdrv_write_threshold_is_set(const BlockDriverState *bs) > > +{ > > + return !!(bs->write_threshold_offset > 0); > > The !! is spurious; use of > already guarantees a bool result. Will remove. > > +++ b/qapi/block-core.json > > @@ -239,6 +239,9 @@ > > # > > # @iops_size: #optional an I/O size in bytes (Since 1.7) > > # > > +# @write_threshold: configured write threshold for the device. > > +# 0 if disabled. (Since 2.3) > > +# > > # Since: 0.14.0 > > # > > ## > > @@ -253,7 +256,7 @@ > > '*bps_max': 'int', '*bps_rd_max': 'int', > > '*bps_wr_max': 'int', '*iops_max': 'int', > > '*iops_rd_max': 'int', '*iops_wr_max': 'int', > > - '*iops_size': 'int' } } > > + '*iops_size': 'int', 'write_threshold': 'uint64' } } > > 'int' works as well as 'uint64'; since this is an output parameter, we > aren't gaining any stricter input parsing by using a more-specific type. I found one case on which the usage 'int' vs 'uint64' made the code generator emit different code - and the one using uint64 was more correct. Can't recall if that is the case; I'll retry, and add a comment here to document the behaviour if I stumble on this again. > My findings are minor, so I'm okay if you post a v5 that addresses them > and includes: > Reviewed-by: Eric Blake Yep, will post ASAP. Thanks and best regards. -- Francesco Romani RedHat Engineering Virtualization R & D Phone: 8261328 IRC: fromani