[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] block: add watermark event
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH] block: add watermark event |
Date: |
Fri, 1 Aug 2014 12:39:40 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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.
> + 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).
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.
> + }
> +
> + return 0; /* should always let other notifiers run */
> +}
> +
> +void bdrv_set_watermark_perc(BlockDriverState *bs,
> + int watermark_perc)
> +{
> + NotifierWithReturn before_write = {
> + .notify = watermark_before_write_notify,
> + };
> +
> + if (watermark_perc <= 0) {
> + return;
> + }
> +
> + if (bs->wr_watermark_perc == 0) {
> + bdrv_add_before_write_notifier(bs, &before_write);
before_write must be a BlockDriverState field so it has the correct
lifetime. In this patch before_write is allocated on the stack and will
cause invalid memory accesses once we leave this function.
pgplYK3a9m50C.pgp
Description: PGP signature
- Re: [Qemu-devel] [PATCH] block: add watermark event,
Stefan Hajnoczi <=