Re: [PATCH v2 3/7] block/block-copy: factor out block_copy_find_inflight_req

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2328 bytes --]

On 27.11.19 19:08, Vladimir Sementsov-Ogievskiy wrote:
> Split block_copy_find_inflight_req to be used in seprate.

*separate, but separate what?

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/block-copy.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 74295d93d5..94e7e855ef 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -24,23 +24,30 @@
>  #define BLOCK_COPY_MAX_BUFFER (1 * MiB)
>  #define BLOCK_COPY_MAX_MEM (128 * MiB)
>  
> +static BlockCopyInFlightReq *block_copy_find_inflight_req(BlockCopyState *s,

Hm.  Long already, but the name begs the question what in-flight
requests this is about.  Maybe drop the block_copy prefix (unless you
plan to make this function global) and call it “find_inflight_conflict”?
 (Or “find_conflicting_inflight_req” to be more verbose)

> +                                                          int64_t start,
> +                                                          int64_t end)
> +{
> +    BlockCopyInFlightReq *req;
> +
> +    QLIST_FOREACH(req, &s->inflight_reqs, list) {
> +        if (end > req->start_byte && start < req->end_byte) {
> +            return req;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
>  static void coroutine_fn block_copy_wait_inflight_reqs(BlockCopyState *s,
>                                                         int64_t start,
>                                                         int64_t end)
>  {
>      BlockCopyInFlightReq *req;
> -    bool waited;
> -
> -    do {
> -        waited = false;
> -        QLIST_FOREACH(req, &s->inflight_reqs, list) {
> -            if (end > req->start_byte && start < req->end_byte) {
> -                qemu_co_queue_wait(&req->wait_queue, NULL);
> -                waited = true;
> -                break;
> -            }
> -        }
> -    } while (waited);
> +
> +    while ((req = block_copy_find_inflight_req(s, start, end))) {
> +        qemu_co_queue_wait(&req->wait_queue, NULL);
> +    }
>  }

The change itself looks rather nice to me.  Even without other users of
this new function, it turns what’s happening into a more obvious pattern.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]