Re: [Qemu-devel] [RFC][PATCH v2] block: add write threshold reporting for block devices

[Stefan Hajnoczi <stefanha@gmail.com>]

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

On Fri, Nov 07, 2014 at 02:12:13PM +0100, Francesco Romani wrote:

Sorry for the long review delay.  Looks pretty good, just one real issue
to think about at the bottom.

> +static void usage_threshold_disable(BlockDriverState *bs)
> +{

It would be safest to make this idempotent:

if (!usage_threshold_is_set(bs)) {
    return;
}

That way it can be called multiple times.

> +    notifier_with_return_remove(&bs->wr_usage_threshold_notifier);
> +    bs->wr_offset_threshold = 0;
> +}
> +
> +static int usage_threshold_is_set(const BlockDriverState *bs)
> +{
> +    return !!(bs->wr_offset_threshold > 0);
> +}

Please use the bool type instead of an int return value.

> +
> +static int64_t usage_threshold_exceeded(const BlockDriverState *bs,
> +                                        const BdrvTrackedRequest *req)
> +{
> +    if (usage_threshold_is_set(bs)) {
> +        int64_t amount = req->offset + req->bytes - bs->wr_offset_threshold;
> +        if (amount > 0) {
> +            return amount;
> +        }
> +    }
> +    return 0;
> +}
> +
> +static int coroutine_fn before_write_notify(NotifierWithReturn *notifier,
> +                                            void *opaque)
> +{
> +    BdrvTrackedRequest *req = opaque;
> +    BlockDriverState *bs = req->bs;
> +    int64_t amount = 0;
> +
> +    assert((req->offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> +    assert((req->bytes & (BDRV_SECTOR_SIZE - 1)) == 0);

Does the code still make these assumptions or are the asserts left over
from previous versions of the patch?

> +
> +    amount = usage_threshold_exceeded(bs, req);
> +    if (amount > 0) {
> +        qapi_event_send_block_usage_threshold(
> +            bdrv_get_device_name(bs), /* FIXME: this does not work */
> +            amount,
> +            bs->wr_offset_threshold,
> +            &error_abort);
> +
> +        /* autodisable to avoid to flood the monitor */
> +        usage_threshold_disable(bs);
> +    }
> +
> +    return 0; /* should always let other notifiers run */
> +}
> +
> +static void usage_threshold_register_notifier(BlockDriverState *bs)
> +{
> +    bs->wr_usage_threshold_notifier.notify = before_write_notify;
> +    notifier_with_return_list_add(&bs->before_write_notifiers,
> +                                  &bs->wr_usage_threshold_notifier);
> +}
> +
> +void bdrv_set_usage_threshold(BlockDriverState *bs, int64_t threshold_bytes)
> +{
> +    BlockDriverState *target_bs = bs;
> +    if (bs->file) {
> +        target_bs = bs->file;
> +    }

Hmm...I think now I understand why you are trying to use bs->file.  This
is an attempt to make image formats work with the threshold.

Unfortunately the BlockDriverState topology can be more complicated than
just 1 level.

If we hardcode a strategy to traverse bs->file then it will work in most
cases:

  while (bs->file) {
      bs = bs->file;
  }

But there are cases like VMDK extent files where a BlockDriverState
actually has multiple children.

One way to solve this is to require that the management tool tells QEMU
which exact BlockDriverState node the threshold applies to.  Then QEMU
doesn't need any hardcoded policy.  But I'm not sure how realistic that
it at the moment (whether management tools are uses node names for each
node yet), so it may be best to hardcode the bs->file traversal that
I've suggested.

Kevin: Do you agree?

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