Re: [PATCH] block: simplify write-threshold and drop write notifiers

[Max Reitz]

On 22.04.21 00:09, Vladimir Sementsov-Ogievskiy wrote:
> write-notifiers are used only for write-threshold. New code for such
> purpose should create filters.
>
> Let's handle write-threshold simply in generic code and drop write
> notifiers at all.
>
> Also move part of write-threshold API that is used only for testing to
> the test.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> I agree that this could be split into 2-3 parts and not combining
> everything into one. But I'm tired now.

Er...  You should have put it off until the next day then? O:)

It should be multiple patches.  At least one to move the write threshold 
update to block/io.c, and then another to drop write notifiers.

> I can send v2 if needed, so
> consider it as RFC. Or take as is if you think it's not too much-in-one.
>
> I also suggest this as a prepartion (and partly substitution) for
> "[PATCH v2 0/8] Block layer thread-safety, continued"
>
>   include/block/block_int.h         | 12 -----
>   include/block/write-threshold.h   | 24 ---------
>   block.c                           |  1 -
>   block/io.c                        | 21 +++++---
>   block/write-threshold.c           | 87 ++-----------------------------
>   tests/unit/test-write-threshold.c | 38 ++++++++++++++
>   6 files changed, 54 insertions(+), 129 deletions(-)

[...]

> diff --git a/block/io.c b/block/io.c
> index ca2dca3007..e0aa775952 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -36,6 +36,8 @@
>   #include "qemu/main-loop.h"
>   #include "sysemu/replay.h"
>   
> +#include "qapi/qapi-events-block-core.h"
> +
>   /* Maximum bounce buffer for copy-on-read and write zeroes, in bytes */
>   #define MAX_BOUNCE_BUFFER (32768 << BDRV_SECTOR_BITS)
>   
> @@ -1974,6 +1976,8 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, int64_t bytes,
>              child->perm & BLK_PERM_RESIZE);
>   
>       switch (req->type) {
> +        uint64_t write_threshold;
> +

Works, but I think this is the first time I see a variable declared in a 
switch block.  What I usually do for such cases is to put a block after 
the label.  (i.e. case X: { uint64_t write_threshold; ... })

But it wouldn’t hurt to just declare this at the beginning of 
bdrv_co_write_req_prepare(), I think.

>       case BDRV_TRACKED_WRITE:
>       case BDRV_TRACKED_DISCARD:
>           if (flags & BDRV_REQ_WRITE_UNCHANGED) {
> @@ -1981,8 +1985,15 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t 
> offset, int64_t bytes,
>           } else {
>               assert(child->perm & BLK_PERM_WRITE);
>           }
> -        return notifier_with_return_list_notify(&bs->before_write_notifiers,
> -                                                req);
> +        write_threshold = qatomic_read(&bs->write_threshold_offset);
> +        if (write_threshold > 0 && offset + bytes > write_threshold) {
> +            qapi_event_send_block_write_threshold(
> +                bs->node_name,
> +                offset + bytes - write_threshold,
> +                write_threshold);
> +            qatomic_set(&bs->write_threshold_offset, 0);
> +        }

I’d put all of this into a function in block/write-threshold.c that’s 
called from here.

Max

> +        return 0;
>       case BDRV_TRACKED_TRUNCATE:
>           assert(child->perm & BLK_PERM_RESIZE);
>           return 0;
> @@ -3137,12 +3148,6 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, 
> QEMUIOVector *qiov)
>       return true;
>   }
>   
> -void bdrv_add_before_write_notifier(BlockDriverState *bs,
> -                                    NotifierWithReturn *notifier)
> -{
> -    notifier_with_return_list_add(&bs->before_write_notifiers, notifier);
> -}
> -
>   void bdrv_io_plug(BlockDriverState *bs)
>   {
>       BdrvChild *child;