Re: [PATCH v5 06/10] block: introduce BDRV_REQ_NO_WAIT flag

[Vladimir Sementsov-Ogievskiy]

25.08.2020 16:10, Max Reitz wrote:
> On 21.08.20 16:11, Vladimir Sementsov-Ogievskiy wrote:
> > Add flag to make serialising request no wait: if there are conflicting
> > requests, just return error immediately. It's will be used in upcoming
> > preallocate filter.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   include/block/block.h |  9 ++++++++-
> >   block/io.c            | 11 ++++++++++-
> >   2 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/block/block.h b/include/block/block.h
> > index b8f4e86e8d..877fda06a4 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -67,8 +67,15 @@ typedef enum {
> >        * written to qiov parameter which may be NULL.
> >        */
> >       BDRV_REQ_PREFETCH  = 0x200,
> > +
> > +    /*
> > +     * If we need to wait for other requests, just fail immediately. Used
> > +     * only together with BDRV_REQ_SERIALISING.
> > +     */
> > +    BDRV_REQ_NO_WAIT = 0x400,
> > +
> >       /* Mask of valid flags */
> > -    BDRV_REQ_MASK               = 0x3ff,
> > +    BDRV_REQ_MASK               = 0x7ff,
> >   } BdrvRequestFlags;
> >   
> >   typedef struct BlockSizes {
> > diff --git a/block/io.c b/block/io.c
> > index dd28befb08..c93b1e98a3 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -1912,9 +1912,18 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t 
> > offset, uint64_t bytes,
> >       assert(!(bs->open_flags & BDRV_O_INACTIVE));
> >       assert((bs->open_flags & BDRV_O_NO_IO) == 0);
> >       assert(!(flags & ~BDRV_REQ_MASK));
> > +    assert(!((flags & BDRV_REQ_NO_WAIT) && !(flags & BDRV_REQ_SERIALISING)));
> >   
> >       if (flags & BDRV_REQ_SERIALISING) {
> > -        bdrv_make_request_serialising(req, bdrv_get_cluster_size(bs));
> > +        QEMU_LOCK_GUARD(&bs->reqs_lock);
> > +
> > +        tracked_request_set_serialising(req, bdrv_get_cluster_size(bs));
> > +
> > +        if ((flags & BDRV_REQ_NO_WAIT) && bdrv_find_conflicting_request(req)) {
>
> bdrv_find_conflicting_request() will return NULL even if there are
> conflicting requests, but those have a non-NULL waiting_for.  Is that
> something to consider?
>
> (I would like to think that will never have a real impact because then
> we must find some other conflicting request; but isn’t is possible that
> we find an overlapping request that waits for another request with which
> it overlaps, but our request does not?)

Actually check in bdrv_find_conflicting_request() is the same like in the 
following
bdrv_wait_serialising_requests_locked(), so, if bdrv_find_conflicting_request() 
returns
NULL, it means that in bdrv_wait_serialising_requests_locked() it will return 
NULL
again (as there are no yield points) and we will not wait, so all is OK.

And, why is it OK to ignore already waiting requests in
bdrv_wait_serialising_requests_locked(): just because if we proceed now with 
our request,
these waiting requests will have to wait for us, when they wake and go to the 
next iteration
of waiting loop.

> > +            return -EBUSY;
> > +        }
> > +
> > +        bdrv_wait_serialising_requests_locked(req);
> >       } else {
> >           bdrv_wait_serialising_requests(req);
> >       }


--
Best regards,
Vladimir