Re: [PATCH v2 1/8] block: prepare write threshold code for thread safety

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2920 bytes --]

On Mon, Apr 19, 2021 at 10:55:34AM +0200, Emanuele Giuseppe Esposito wrote:

No commit description. What about write thresholds makes them thread
unsafe? Without a commit description reviewers have to reverse-engineer
the patch to figure out the author's intention, which can lead to
misunderstandings and bugs slipping through.

My guess is the point of this patch was to stop accessing fields in bs
directly?

> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  block/write-threshold.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/block/write-threshold.c b/block/write-threshold.c
> index 85b78dc2a9..63040fa4f2 100644
> --- a/block/write-threshold.c
> +++ b/block/write-threshold.c
> @@ -37,18 +37,22 @@ static void write_threshold_disable(BlockDriverState *bs)
>      }
>  }
>  
> +static uint64_t treshold_overage(const BdrvTrackedRequest *req,
> +                                uint64_t thres)
> +{
> +    if (thres > 0 && req->offset + req->bytes > thres) {
> +        return req->offset + req->bytes - thres;
> +    } else {
> +        return 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;
> +    uint64_t thres = bdrv_write_threshold_get(bs);
> +
> +    return treshold_overage(req, thres);
>  }

Hmm...this function is only used by tests now. Should the tests be
updated so that they are exercising the actual code instead of this
test-only interface?

>  
>  static int coroutine_fn before_write_notify(NotifierWithReturn *notifier,
> @@ -56,14 +60,14 @@ static int coroutine_fn before_write_notify(NotifierWithReturn *notifier,
>  {
>      BdrvTrackedRequest *req = opaque;
>      BlockDriverState *bs = req->bs;
> -    uint64_t amount = 0;
> +    uint64_t thres = bdrv_write_threshold_get(bs);
> +    uint64_t amount = treshold_overage(req, thres);
>  
> -    amount = bdrv_write_threshold_exceeded(bs, req);
>      if (amount > 0) {
>          qapi_event_send_block_write_threshold(
>              bs->node_name,
>              amount,
> -            bs->write_threshold_offset);
> +            thres);
>  
>          /* autodisable to avoid flooding the monitor */
>          write_threshold_disable(bs);
> -- 
> 2.30.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]