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

[Max Reitz]

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

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

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

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

signature.asc
Description: OpenPGP digital signature