Re: [Qemu-devel] [PATCH v4] block: add event when disk usage exceeds threshold

[Francesco Romani]

Hi,

thanks for the review!

----- Original Message -----
> From: "Eric Blake" <address@hidden>
> To: "Francesco Romani" <address@hidden>, address@hidden
> Cc: address@hidden, address@hidden, address@hidden, address@hidden
> 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 <address@hidden>
> > ---
> 
> > --- /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 <address@hidden>

Yep, will post ASAP.

Thanks and best regards.

-- 
Francesco Romani
RedHat Engineering Virtualization R & D
Phone: 8261328
IRC: fromani