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

[Vladimir Sementsov-Ogievskiy]

05.05.2021 15:37, Max Reitz wrote:
> 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.

I don't think it helps with overflow:) Still, offset + bytes should never 
overflow in block layer, requests are checked at the entrance.

> > +        bdrv_write_threshold_set(bs, 0);
>
> I’d keep the comment from before_write_notify() here (i.e. “autodisable to 
> avoid flooding the monitor”).

I'm OK with it and both your rewording suggestions. Will apply if v3 needed.

> But other than that, I have no complaints:
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Thanks for reviewing my patches!


--
Best regards,
Vladimir