Re: [Qemu-block] [PATCH v2 2/2] block: add filter driver to block/write-threshold.c

[Kevin Wolf]

Am 15.08.2017 um 10:18 hat Manos Pitsidianakis geschrieben:
> With runtime insertion and removal of filters, write-threshold.c can
> provide more flexible deliveries of BLOCK_WRITE_THRESHOLD events. After
> the event trigger, the filter nodes are no longer useful and must be
> removed.
> The existing write-threshold cannot be easily converted to using the
> filter driver, so it is not affected.
> 
> This is part of deprecating before write notifiers, which are hard coded
> into the block layer. Block filter drivers are inserted into the graph
> only when a feature is needed. This makes the block layer more modular
> and reuses the block driver abstraction that is already present.
> 
> Signed-off-by: Manos Pitsidianakis <address@hidden>
> ---
>  block/qapi.c                    |   2 +-
>  block/write-threshold.c         | 264 
> +++++++++++++++++++++++++++++++++++-----
>  include/block/write-threshold.h |  22 ++--
>  qapi/block-core.json            |  19 ++-
>  tests/test-write-threshold.c    |  40 +++---
>  5 files changed, 281 insertions(+), 66 deletions(-)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index 2be44a6758..fe6cf2eae5 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -122,7 +122,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>          info->group = g_strdup(throttle_group_get_name(tgm));
>      }
>  
> -    info->write_threshold = bdrv_write_threshold_get(bs);
> +    info->write_threshold = bdrv_write_threshold_get_legacy(bs);
>  
>      bs0 = bs;
>      p_image_info = &info->image;
> diff --git a/block/write-threshold.c b/block/write-threshold.c
> index 0bd1a01c86..4a67188ea3 100644
> --- a/block/write-threshold.c
> +++ b/block/write-threshold.c
> @@ -2,9 +2,11 @@
>   * QEMU System Emulator block write threshold notification
>   *
>   * Copyright Red Hat, Inc. 2014
> + * Copyright 2017 Manos Pitsidianakis
>   *
>   * Authors:
>   *  Francesco Romani <address@hidden>
> + *  Manos Pitsidianakis <address@hidden>
>   *
>   * This work is licensed under the terms of the GNU LGPL, version 2 or later.
>   * See the COPYING.LIB file in the top-level directory.
> @@ -19,46 +21,35 @@
>  #include "qmp-commands.h"
>  
>  
> -uint64_t bdrv_write_threshold_get(const BlockDriverState *bs)
> +uint64_t bdrv_write_threshold_get_legacy(const BlockDriverState *bs)
>  {
>      return bs->write_threshold_offset;
>  }
>  
> -bool bdrv_write_threshold_is_set(const BlockDriverState *bs)
> +bool bdrv_write_threshold_is_set_legacy(const BlockDriverState *bs)
>  {
>      return bs->write_threshold_offset > 0;
>  }
>  
> -static void write_threshold_disable(BlockDriverState *bs)
> +static void write_threshold_disable_legacy(BlockDriverState *bs)
>  {
> -    if (bdrv_write_threshold_is_set(bs)) {
> +    if (bdrv_write_threshold_is_set_legacy(bs)) {
>          notifier_with_return_remove(&bs->write_threshold_notifier);
>          bs->write_threshold_offset = 0;
>      }
>  }
>  
> -uint64_t bdrv_write_threshold_exceeded(const BlockDriverState *bs,
> -                                       const BdrvTrackedRequest *req)
> -{
> -    if (bdrv_write_threshold_is_set(bs)) {
> -        if (req->offset > bs->write_threshold_offset) {
> -            return (req->offset - bs->write_threshold_offset) + req->bytes;
> -        }
> -        if ((req->offset + req->bytes) > bs->write_threshold_offset) {
> -            return (req->offset + req->bytes) - bs->write_threshold_offset;
> -        }
> -    }
> -    return 0;
> -}
> -
>  static int coroutine_fn before_write_notify(NotifierWithReturn *notifier,
>                                              void *opaque)
>  {
>      BdrvTrackedRequest *req = opaque;
>      BlockDriverState *bs = req->bs;
>      uint64_t amount = 0;
> +    uint64_t threshold = bdrv_write_threshold_get_legacy(bs);
> +    uint64_t offset = req->offset;
> +    uint64_t bytes = req->bytes;
>  
> -    amount = bdrv_write_threshold_exceeded(bs, req);
> +    amount = bdrv_write_threshold_exceeded(threshold, offset, bytes);
>      if (amount > 0) {
>          qapi_event_send_block_write_threshold(
>              bs->node_name,
> @@ -67,7 +58,7 @@ static int coroutine_fn 
> before_write_notify(NotifierWithReturn *notifier,
>              &error_abort);
>  
>          /* autodisable to avoid flooding the monitor */
> -        write_threshold_disable(bs);
> +        write_threshold_disable_legacy(bs);
>      }
>  
>      return 0; /* should always let other notifiers run */
> @@ -79,25 +70,26 @@ static void 
> write_threshold_register_notifier(BlockDriverState *bs)
>      bdrv_add_before_write_notifier(bs, &bs->write_threshold_notifier);
>  }
>  
> -static void write_threshold_update(BlockDriverState *bs,
> -                                   int64_t threshold_bytes)
> +static void write_threshold_update_legacy(BlockDriverState *bs,
> +                                          int64_t threshold_bytes)
>  {
>      bs->write_threshold_offset = threshold_bytes;
>  }
>  
> -void bdrv_write_threshold_set(BlockDriverState *bs, uint64_t threshold_bytes)
> +void bdrv_write_threshold_set_legacy(BlockDriverState *bs,
> +                                     uint64_t threshold_bytes)
>  {
> -    if (bdrv_write_threshold_is_set(bs)) {
> +    if (bdrv_write_threshold_is_set_legacy(bs)) {
>          if (threshold_bytes > 0) {
> -            write_threshold_update(bs, threshold_bytes);
> +            write_threshold_update_legacy(bs, threshold_bytes);
>          } else {
> -            write_threshold_disable(bs);
> +            write_threshold_disable_legacy(bs);
>          }
>      } else {
>          if (threshold_bytes > 0) {
>              /* avoid multiple registration */
>              write_threshold_register_notifier(bs);
> -            write_threshold_update(bs, threshold_bytes);
> +            write_threshold_update_legacy(bs, threshold_bytes);
>          }
>          /* discard bogus disable request */
>      }
> @@ -119,7 +111,223 @@ void qmp_block_set_write_threshold(const char 
> *node_name,
>      aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(aio_context);
>  
> -    bdrv_write_threshold_set(bs, threshold_bytes);
> +    bdrv_write_threshold_set_legacy(bs, threshold_bytes);
>  
>      aio_context_release(aio_context);
>  }
> +
> +
> +/* The write-threshold filter drivers delivers a one-time 
> BLOCK_WRITE_THRESHOLD
> + * event when a passing write request exceeds the configured write threshold
> + * offset of the filter.
> + *
> + * This is useful to transparently resize thin-provisioned drives without
> + * the guest OS noticing.
> + */
> +
> +#define QEMU_OPT_WRITE_THRESHOLD "write-threshold"
> +static BlockDriver write_threshold;

This forward declaration is unnecessary, the variable is never used
before the actual definition.

> +static QemuOptsList write_threshold_opts = {
> +    .name = "write-threshold",
> +    .head = QTAILQ_HEAD_INITIALIZER(write_threshold_opts.head),
> +    .desc = {
> +        {
> +            .name = QEMU_OPT_WRITE_THRESHOLD,
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "configured threshold for the block device, bytes. Use 0"
> +                    "to disable the threshold",
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
> +static bool bdrv_write_threshold_is_set(const BlockDriverState *bs)
> +{
> +    uint64_t threshold = *(uint64_t *)bs->opaque;

I think I would prefer using a struct for bs->opaque, even if it only
contains a single uint64_t for now.

> +    return threshold > 0;
> +}

> +static int64_t coroutine_fn write_threshold_co_get_block_status(
> +                                                       BlockDriverState *bs,
> +                                                       int64_t sector_num,
> +                                                       int nb_sectors,
> +                                                       int *pnum,
> +                                                       BlockDriverState 
> **file)
> +{
> +    assert(child_bs(bs));
> +    *pnum = nb_sectors;
> +    *file = child_bs(bs);
> +    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
> +           (sector_num << BDRV_SECTOR_BITS);
> +}

This is bdrv_co_get_block_status_from_file() again.

Kevin