[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