Re: [PATCH v2 1/9] block/write-threshold: don't use write notifiers

[Max Reitz <mreitz@redhat.com>]

On 04.05.21 10:25, Vladimir Sementsov-Ogievskiy wrote:
> write-notifiers are used only for write-threshold. New code for such
> purpose should create filters.
> 
> Let's better special-case write-threshold and drop write notifiers at
> all. (Actually, write-threshold is special-cased anyway, as the only
> user of write-notifiers)

Not noted here: That write-threshold.c is also reorganized.  (Doesn’t 
seem entirely necessary to do right in this patch, but why not.)

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/block_int.h       |  1 -
>   include/block/write-threshold.h |  9 +++++
>   block/io.c                      |  5 ++-
>   block/write-threshold.c         | 68 +++++++--------------------------
>   4 files changed, 25 insertions(+), 58 deletions(-)

[...]

> diff --git a/include/block/write-threshold.h b/include/block/write-threshold.h
> index c646f267a4..7942dcc368 100644
> --- a/include/block/write-threshold.h
> +++ b/include/block/write-threshold.h
> @@ -59,4 +59,13 @@ bool bdrv_write_threshold_is_set(const BlockDriverState *bs);
>   uint64_t bdrv_write_threshold_exceeded(const BlockDriverState *bs,
>                                          const BdrvTrackedRequest *req);
>   
> +/*
> + * bdrv_write_threshold_check_write
> + *
> + * Check, does specified request exceeds write threshold. If it is, send

I’d prefer either “Check: Does the specified request exceed the write 
threshold?” or “Check whether the specified request exceeds the write 
threshold.”

> + * corresponding event and unset write threshold.

I’d call it “disable write threshold checking” instead of “unset write 
threshold”, because I don’t it immediately clear what unsetting the 
threshold means.

> + */
> +void bdrv_write_threshold_check_write(BlockDriverState *bs, int64_t offset,
> +                                      int64_t bytes);
> +
>   #endif

[...]

> @@ -122,3 +68,15 @@ void qmp_block_set_write_threshold(const char *node_name,
>   
>       aio_context_release(aio_context);
>   }
> +
> +void bdrv_write_threshold_check_write(BlockDriverState *bs, int64_t offset,
> +                                      int64_t bytes)
> +{
> +    int64_t end = offset + bytes;
> +    uint64_t wtr = bs->write_threshold_offset;
> +
> +    if (wtr > 0 && end > wtr) {
> +        qapi_event_send_block_write_threshold(bs->node_name, end - wtr, wtr);

OK, don’t understand why bdrv_write_threshold_exceeded() had two cases 
(one for offset > wtr, one for end > wtr).  Perhaps overflow 
considerations?  Shouldn’t matter though.

> +        bdrv_write_threshold_set(bs, 0);

I’d keep the comment from before_write_notify() here (i.e. “autodisable 
to avoid flooding the monitor”).

But other than that, I have no complaints:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +    }
> +}
>