[Qemu-block] [PATCH v2 1/4] block: introduce aio task pool
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
---
block/aio_task.h| 52 +++
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
@@ -0,0 +1,52 @@
+/*
+ * Aio tasks loops
+ *
+ * Copyright (c) 2019 Virtuozzo International GmbH.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef BLOCK_AIO_TASK_H
+#define BLOCK_AIO_TASK_H
+
+#include "qemu/coroutine.h"
+
+typedef struct AioTaskPool AioTaskPool;
+typedef struct AioTask AioTask;
+typedef int (*AioTaskFunc)(AioTask *task);
+struct AioTask {
+AioTaskPool *pool;
+AioTaskFunc func;
+int ret;
+};
+
+/*
+ * aio_task_pool_new
+ *
+ * The caller is responsible to g_free AioTaskPool pointer after use.
+ */
+AioTaskPool *aio_task_pool_new(int max_busy_tasks);
+int aio_task_pool_status(AioTaskPool *pool);
+bool aio_task_pool_empty(AioTaskPool *pool);
+void aio_task_pool_start_task(AioTaskPool *pool, AioTask *task);
+void aio_task_pool_wait_slot(AioTaskPool *pool);
+void aio_task_pool_wait_one(AioTaskPool *pool);
+void aio_task_pool_wait_all(AioTaskPool *pool);
+
+#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
@@ -0,0 +1,119 @@
+/*
+ * Aio tasks loops
+ *
+ * Copyright (c) 2019 Virtuozzo International GmbH.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "block/aio.h"
+#include "aio_task.h"
+
+struct AioTaskPool {
+Coroutine *main_co;
+int status;
+int max_busy_tasks;
+int busy_tasks;
+bool wait_done;
+};
+
+static void aio_task_co(void *opaque)
+{
+AioTask *task = opaque;
+AioTaskPool *pool = task->pool;
+
+assert(pool->busy_tasks < pool->max_busy_tasks);
+pool->busy_tasks++;
+
+task->ret = task->func(task);
+
+pool->busy_tasks--;
+
+if (task->ret < 0 && pool->status == 0) {
+pool->status = task->ret;
+}
+
+g_free(task);
+
+if (pool->wait_done) {
+pool->wait_done = false;
+aio_co_wake(pool->main_co);
+}
+}
+
+void aio_task_pool_wait_one(AioTaskPool *pool)
+{
+assert(pool->busy_tasks > 0);
+assert(qemu_coroutine_self() == pool->main_co);
+
+pool->wait_done = true;
+qemu_coroutine_yield();
+
+assert(!pool->wait_done);
+assert(pool->busy_tasks < pool->max_busy_tasks);
+}
+
+void aio_task_pool_wait_slot(AioTaskPool *pool)
+{
+if (pool->busy_tasks < pool->max_
Re: [Qemu-block] [PATCH v2 1/4] block: introduce aio task pool
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
Re: [Qemu-block] [PATCH v2 1/4] block: introduce aio task pool
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
