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

[Stefan Hajnoczi <stefanha@redhat.com>]

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

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.

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