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

[Vladimir Sementsov-Ogievskiy]

07.07.2020 19:10, Stefan Hajnoczi wrote:
> 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()?

OK.

May be _in_locked_range ? A bit longer, but more understandable.

--
Best regards,
Vladimir