Re: [Qemu-block] [PATCH v2 1/4] block: introduce aio task pool

[Vladimir Sementsov-Ogievskiy]

13.08.2019 23:47, Max Reitz wrote:
> 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 
>> ---
> 
> 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 00..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 00..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/?
> 


Aha, really bad variable name. I meant "wait for one task done". Just "waiting" 
would be appropriate.

Thanks for reviewing!

-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v2 1/4] block: introduce aio task pool

[Max Reitz]

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

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