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

[Vladimir Sementsov-Ogievskiy]

30.04.2021 13:04, Max Reitz wrote:
> 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:)

Yes, sorry. I wanted to send that day... Don't remember why :) Checked now, 
that was not Friday.. I wanted to drop write notifiers long ago, and when I 
finally do it I couldn't wait, so cool it seemed to me :)

Thanks for comments, I'll send v2 soon.

> 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;


--
Best regards,
Vladimir