Re: [PATCH v3 06/19] block: intoduce reqlist

[Hanna Reitz]

On 22.12.21 18:40, Vladimir Sementsov-Ogievskiy wrote:
> Split intersecting-requests functionality out of block-copy to be
> reused in copy-before-write filter.
>
> Note: while being here, fix tiny typo in MAINTAINERS.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/reqlist.h |  67 +++++++++++++++++++++++
>   block/block-copy.c      | 116 +++++++++++++---------------------------
>   block/reqlist.c         |  76 ++++++++++++++++++++++++++
>   MAINTAINERS             |   4 +-
>   block/meson.build       |   1 +
>   5 files changed, 184 insertions(+), 80 deletions(-)
>   create mode 100644 include/block/reqlist.h
>   create mode 100644 block/reqlist.c

Looks good to me, this split makes sense.

I have just minor comments (50 % about pre-existing things) below.

> diff --git a/include/block/reqlist.h b/include/block/reqlist.h
> new file mode 100644
> index 0000000000..b904d80216
> --- /dev/null
> +++ b/include/block/reqlist.h
> @@ -0,0 +1,67 @@
> +/*
> + * reqlist API
> + *
> + * Copyright (C) 2013 Proxmox Server Solutions
> + * Copyright (c) 2021 Virtuozzo International GmbH.
> + *
> + * Authors:
> + *  Dietmar Maurer (dietmar@proxmox.com)
> + *  Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef REQLIST_H
> +#define REQLIST_H
> +
> +#include "qemu/coroutine.h"
> +
> +/*
> + * The API is not thread-safe and shouldn't be. The struct is public to be part
> + * of other structures and protected by third-party locks, see
> + * block/block-copy.c for example.
> + */
> +
> +typedef struct BlockReq {
> +    int64_t offset;
> +    int64_t bytes;
> +
> +    CoQueue wait_queue; /* coroutines blocked on this req */
> +    QLIST_ENTRY(BlockReq) list;
> +} BlockReq;
> +
> +typedef QLIST_HEAD(, BlockReq) BlockReqList;
> +
> +/*
> + * Initialize new request and add it to the list. Caller should be sure that

I’d say s/should/must/, because that is guarded by an assertion.

> + * there are no conflicting requests in the list.
> + */
> +void reqlist_init_req(BlockReqList *reqs, BlockReq *req, int64_t offset,
> +                      int64_t bytes);
> +/* Search for request in the list intersecting with @offset/@bytes area. */
> +BlockReq *reqlist_find_conflict(BlockReqList *reqs, int64_t offset,
> +                                int64_t bytes);
> +
> +/*
> + * If there are no intersecting requests return false. Otherwise, wait for the
> + * first found intersecting request to finish and return true.
> + *
> + * @lock is passed to qemu_co_queue_wait()
> + * False return value proves that lock was NOT released.

I’d say “was released at no point” instead, because when first reading 
this I understood it to mean that lock simply is locked when this 
function returns, and so I thought that this implies that when `true` is 
returned, the lock is released (and remains released).

> + */
> +bool coroutine_fn reqlist_wait_one(BlockReqList *reqs, int64_t offset,
> +                                   int64_t bytes, CoMutex *lock);
> +
> +/*
> + * Shrink request and wake all waiting coroutines (may be some of them are not

s/may be/maybe/

> + * intersecting with shrunk request).
> + */
> +void coroutine_fn reqlist_shrink_req(BlockReq *req, int64_t new_bytes);
> +
> +/*
> + * Remove request and wake all waiting coroutines. Do not release any memory.
> + */
> +void coroutine_fn reqlist_remove_req(BlockReq *req);
> +
> +#endif /* REQLIST_H */

> diff --git a/block/reqlist.c b/block/reqlist.c
> new file mode 100644
> index 0000000000..5e320ba649
> --- /dev/null
> +++ b/block/reqlist.c
> @@ -0,0 +1,76 @@

[...]

> +BlockReq *reqlist_find_conflict(BlockReqList *reqs, int64_t offset,
> +                                int64_t bytes)
> +{
> +    BlockReq *r;
> +
> +    QLIST_FOREACH(r, reqs, list) {
> +        if (offset + bytes > r->offset && offset < r->offset + r->bytes) {

(Late, I know, the old code was exactly this, but:) Why not use 
ranges_overlap()?

> +            return r;
> +        }
> +    }
> +
> +    return NULL;
> +}