[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/4] block: introduce aio task pool
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/4] block: introduce aio task pool |
Date: |
Tue, 13 Aug 2019 22:47:29 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 |
On 30.07.19 16:18, Vladimir Sementsov-Ogievskiy wrote:
> Common interface for aio task loops. To be used for improving
> performance of synchronous io loops in qcow2, block-stream,
> copy-on-read, and may be other places.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
Looks good to me overall.
> block/aio_task.h | 52 +++++++++++++++++++
I’ve move this to include/block/.
> block/aio_task.c | 119 ++++++++++++++++++++++++++++++++++++++++++++
> block/Makefile.objs | 2 +
> 3 files changed, 173 insertions(+)
> create mode 100644 block/aio_task.h
> create mode 100644 block/aio_task.c
>
> diff --git a/block/aio_task.h b/block/aio_task.h
> new file mode 100644
> index 0000000000..933af1d8e7
> --- /dev/null
> +++ b/block/aio_task.h
[...]
> +typedef struct AioTaskPool AioTaskPool;
> +typedef struct AioTask AioTask;
> +typedef int (*AioTaskFunc)(AioTask *task);
+coroutine_fn
> +struct AioTask {
> + AioTaskPool *pool;
> + AioTaskFunc func;
> + int ret;
> +};
> +
> +/*
> + * aio_task_pool_new
> + *
> + * The caller is responsible to g_free AioTaskPool pointer after use.
s/to g_free/for g_freeing/ or something similar.
Or you’d just add aio_task_pool_free().
> + */
> +AioTaskPool *aio_task_pool_new(int max_busy_tasks);
> +int aio_task_pool_status(AioTaskPool *pool);
A comment wouldn’t hurt. It wasn’t immediately clear to me that status
refers to the error code of a failing task (or 0), although it wasn’t
too much of a surprise either.
> +bool aio_task_pool_empty(AioTaskPool *pool);
> +void aio_task_pool_start_task(AioTaskPool *pool, AioTask *task);
Maybe make a note that task->pool will be set automatically?
> +void aio_task_pool_wait_slot(AioTaskPool *pool);
> +void aio_task_pool_wait_one(AioTaskPool *pool);
> +void aio_task_pool_wait_all(AioTaskPool *pool);
Shouldn’t all of these but aio_task_pool_empty() and
aio_task_pool_status() be coroutine_fns?
> +#endif /* BLOCK_AIO_TASK_H */
> diff --git a/block/aio_task.c b/block/aio_task.c
> new file mode 100644
> index 0000000000..807be8deb5
> --- /dev/null
> +++ b/block/aio_task.c
[...]
> +static void aio_task_co(void *opaque)
+coroutine_fn
[...]
> +void aio_task_pool_wait_one(AioTaskPool *pool)
> +{
> + assert(pool->busy_tasks > 0);
> + assert(qemu_coroutine_self() == pool->main_co);
> +
> + pool->wait_done = true;
Hmmm, but the wait actually isn’t done yet. :-)
Maybe s/wait_done/waiting/?
Max
> + qemu_coroutine_yield();
> +
> + assert(!pool->wait_done);
> + assert(pool->busy_tasks < pool->max_busy_tasks);
> +}
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v2 1/4] block: introduce aio task pool,
Max Reitz <=