Re: [Qemu-devel] [PATCH] block: add watermark event

[Francesco Romani]

----- Original Message -----
> From: "Stefan Hajnoczi" <address@hidden>
> To: "Kevin Wolf" <address@hidden>
> Cc: address@hidden, "Francesco Romani" <address@hidden>, address@hidden, 
> address@hidden
> 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