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

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3050 bytes --]

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.

> > +        if (sector_num >= watermark) {
> > +            return true;
> > +        }
> > +    }
> > +    return false;
> > +}
> > +
> > +static int coroutine_fn watermark_before_write_notify(NotifierWithReturn *notifier,
> > +                                                      void *opaque)
> > +{
> > +    BdrvTrackedRequest *req = opaque;
> > +    int64_t sector_num = req->offset >> BDRV_SECTOR_BITS;
> > +    int nb_sectors = req->bytes >> BDRV_SECTOR_BITS;
> > +
> > +/*  FIXME: needed? */
> > +    assert((req->offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> > +    assert((req->bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> 
> Not really needed here.  Emulated storage controllers either get
> requests in block units (i.e. they are automatically aligned) or check
> them (like virtio-blk).

But we don't only get requests from emulated storage controllers. You
cannot reasonably assume anything here (the requests are aligned
according to the requirements of the backend, but that may just be 1).
Again, do your calculations in a byte granularity to be safe here.

> I guess there's no harm in checking, but I would drop it.
> 
> > +
> > +    if (watermark_exceeded(req->bs, sector_num, nb_sectors)) {
> > +        BlockDriverState *bs = req->bs;
> > +        qapi_event_send_block_watermark(
> > +            bdrv_get_device_name(bs),
> > +            sector_num,
> > +            bs->wr_highest_sector,
> > +            &error_abort);
> 
> How do you prevent flooding events if every write request exceeds the
> watermark?
> 
> Perhaps the watermark should be disabled until block-set-watermark is
> called again.

I agree.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]