Re: [Qemu-block] [PATCH v10 5/9] block: treat BDRV_REQ_ALLOCATE as serialising

[Vladimir Sementsov-Ogievskiy]

03.12.2018 13:14, Anton Nefedov wrote:
> The idea is that ALLOCATE requests may overlap with other requests.

please, describe why

> Reuse the existing block layer infrastructure for serialising requests.
> Use the following approach:
>    - mark ALLOCATE also SERIALISING, so subsequent requests to the area wait
>    - ALLOCATE request itself must never wait if another request is in flight
>      already. Return EAGAIN, let the caller reconsider.
> 
> Signed-off-by: Anton Nefedov <address@hidden>
> ---
>   block/io.c | 31 ++++++++++++++++++++++++-------
>   1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index d9d7644858..6ff946f63d 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -720,12 +720,13 @@ void bdrv_dec_in_flight(BlockDriverState *bs)
>       bdrv_wakeup(bs);
>   }
>   
> -static bool coroutine_fn wait_serialising_requests(BdrvTrackedRequest *self)
> +static bool coroutine_fn find_or_wait_serialising_requests(
> +    BdrvTrackedRequest *self, bool wait)
>   {
>       BlockDriverState *bs = self->bs;
>       BdrvTrackedRequest *req;
>       bool retry;
> -    bool waited = false;
> +    bool found = false;
>   
>       if (!atomic_read(&bs->serialising_in_flight)) {
>           return false;
> @@ -751,11 +752,14 @@ static bool coroutine_fn 
> wait_serialising_requests(BdrvTrackedRequest *self)
>                    * will wait for us as soon as it wakes up, then just go on
>                    * (instead of producing a deadlock in the former case). */
>                   if (!req->waiting_for) {
> +                    found = true;
> +                    if (!wait) {
> +                        break;
> +                    }
>                       self->waiting_for = req;
>                       qemu_co_queue_wait(&req->wait_queue, &bs->reqs_lock);
>                       self->waiting_for = NULL;
>                       retry = true;
> -                    waited = true;
>                       break;
>                   }
>               }
> @@ -763,7 +767,12 @@ static bool coroutine_fn 
> wait_serialising_requests(BdrvTrackedRequest *self)
>           qemu_co_mutex_unlock(&bs->reqs_lock);
>       } while (retry);
>   
> -    return waited;
> +    return found;
> +}
> +
> +static bool coroutine_fn wait_serialising_requests(BdrvTrackedRequest *self)
> +{
> +    return find_or_wait_serialising_requests(self, true);
>   }
>   
>   static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
> @@ -1585,7 +1594,7 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t 
> offset, uint64_t bytes,
>                             BdrvTrackedRequest *req, int flags)
>   {
>       BlockDriverState *bs = child->bs;
> -    bool waited;
> +    bool found;
>       int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
>   
>       if (bs->read_only) {
> @@ -1602,9 +1611,13 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t 
> offset, uint64_t bytes,
>           mark_request_serialising(req, bdrv_get_cluster_size(bs));
>       }
>   
> -    waited = wait_serialising_requests(req);
> +    found = find_or_wait_serialising_requests(req,
> +                                              !(flags & BDRV_REQ_ALLOCATE));
> +    if (found && (flags & BDRV_REQ_ALLOCATE)) {
> +        return -EAGAIN;
> +    }
>   
> -    assert(!waited || !req->serialising ||
> +    assert(!found || !req->serialising ||
>              is_request_serialising_and_aligned(req));
>       assert(req->overlap_offset <= offset);
>       assert(offset + bytes <= req->overlap_offset + req->overlap_bytes);
> @@ -1866,6 +1879,10 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
>       assert(!((flags & BDRV_REQ_ALLOCATE) && (flags & BDRV_REQ_MAY_UNMAP)));
>       assert(!((flags & BDRV_REQ_ALLOCATE) && !(flags & 
> BDRV_REQ_ZERO_WRITE)));
>   
> +    if (flags & BDRV_REQ_ALLOCATE) {
> +        flags |= BDRV_REQ_SERIALISING;
> +    }
> +
>       trace_bdrv_co_pwritev(child->bs, offset, bytes, flags);
>   
>       if (!bs->drv) {
> 

patch looks correct technically, I just don't know the reasoning..

the only thing, is that it would be good to add a comment in BDRV flags 
definition section, that _ALLOCATE implies _SERIALIZE.


-- 
Best regards,
Vladimir