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

[Vladimir Sementsov-Ogievskiy]

26.08.2020 11:36, Max Reitz wrote:
> On 26.08.20 08:26, Vladimir Sementsov-Ogievskiy wrote:
> > 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.
>
> OK.  I thought that maybe we would want to avoid that other requests
> might have to wait for the preallocation write.  (Of course, we can’t
> avoid that altogether, but if we already know of such requests at the
> beginning of the request...)

Hmm, I see your point now. Hmm actually, what we want:

1. make preallocation on write
2. serialize the request
3. if there are conflicting requests do nothing, as (most probably) the
conflicting request will do preallocation itself

So, what we can say about intersecting requests, which are waiting for something (and 
therefore are not "conflicting")?
Aha, for sure, they don't preallocate (otherwise they don't wait). So [3] is 
still satisfied..

There was not original aim to not make the other parallel request wait for 
preallocation. So, it may be done later.. Intuitively, I don't expect the 
benefit: if there is no concurrent preallocate request, who are these 
intersecting requests? For example, EOF-crossing READS, waiting for some 
serialized not preallocating (so fit into file-size) WRITE. But this shouldn't 
happen often :)

> Well, if the only thing to look out for is that preallocation writes
> themselves do not wait:

So, let's assume that yes, we care that they don't wait themselves (mostly to 
avoid concurrent preallocation requests) and don't care about other 
intersecting requests.

> Reviewed-by: Max Reitz <mreitz@redhat.com>

Thanks!

> > 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.
>
> Sure.


--
Best regards,
Vladimir