Re: [PATCH 2/5] block/io: introduce bdrv_co_range_try_lock

[Stefan Hajnoczi]

On Sat, Jun 20, 2020 at 05:36:46PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> @@ -83,6 +84,12 @@ typedef struct BdrvTrackedRequest {
>      CoQueue wait_queue; /* coroutines blocked on this request */
>  
>      struct BdrvTrackedRequest *waiting_for;
> +
> +    /*
> +     * If non-zero, the request is under lock, so it's allowed to intersect
> +     * (actully it must be inside) the @lock request.

s/actully/actually/

> @@ -745,15 +747,26 @@ static bool coroutine_fn 
> wait_or_find_conflicts(BdrvTrackedRequest *self,
>              if (tracked_request_overlaps(req, self->overlap_offset,
>                                           self->overlap_bytes))
>              {
> -                /* Hitting this means there was a reentrant request, for
> -                 * example, a block driver issuing nested requests.  This 
> must
> -                 * never happen since it means deadlock.
> +                if (self->lock == req) {
> +                    /* This is atomic request under range_lock */
> +                    assert(req->type == BDRV_TRACKED_LOCK);
> +                    assert(self->offset >= req->offset);
> +                    assert(self->bytes <= req->bytes);

These assertions do not catch requests that start within the locked
region but span beyond the end of the region. How about:

  assert(self->offset + self->bytes - req->offset >= req->bytes);

> +int coroutine_fn bdrv_co_pwrite_zeroes_locked(BdrvChild *child, int64_t 
> offset,
> +                                              int bytes, BdrvRequestFlags 
> flags,
> +                                              BdrvTrackedRequest *lock)

The name is confusing because _locked() normally means that a mutex
should be held. Functions using that naming convention already exist in
block/io.c. It would be nice to distinguish between functions that need
BdrvTrackedRequest and functions that must be called with a mutex held.

How about bdrv_co_pwrite_zeroes_with_lock()?

signature.asc
Description: PGP signature