| Message ID | 20210707150019.201442-4-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | linux-aio: limit the batch size to reduce queue latency | expand |
On Wed, Jul 07, 2021 at 05:00:19PM +0200, Stefano Garzarella wrote: > @@ -371,7 +375,7 @@ static int laio_do_submit(int fd, struct qemu_laiocb *laiocb, off_t offset, > s->io_q.in_queue++; > if (!s->io_q.blocked && > (!s->io_q.plugged || > - s->io_q.in_flight + s->io_q.in_queue >= MAX_EVENTS)) { > + s->io_q.in_queue >= max_batch)) { Is it safe to drop the MAX_EVENTS case? Perhaps the following can be used: int64_t max_batch = s->aio_context->aio_max_batch ?: DEFAULT_MAX_BATCH; max_batch = MIN_NON_ZERO(MAX_EVENTS - s->io_q.in_flight + s->io_q.in_queue, max_batch); Here we'll only need to check against max_batch but it takes into account MAX_EVENT and in_flight. Stefan
On Tue, Jul 13, 2021 at 03:58:04PM +0100, Stefan Hajnoczi wrote: >On Wed, Jul 07, 2021 at 05:00:19PM +0200, Stefano Garzarella wrote: >> @@ -371,7 +375,7 @@ static int laio_do_submit(int fd, struct qemu_laiocb *laiocb, off_t offset, >> s->io_q.in_queue++; >> if (!s->io_q.blocked && >> (!s->io_q.plugged || >> - s->io_q.in_flight + s->io_q.in_queue >= MAX_EVENTS)) { >> + s->io_q.in_queue >= max_batch)) { > >Is it safe to drop the MAX_EVENTS case? I think it is safe since in ioq_submit() we have this check while dequeueing: QSIMPLEQ_FOREACH(aiocb, &s->io_q.pending, next) { iocbs[len++] = &aiocb->iocb; if (s->io_q.in_flight + len >= MAX_EVENTS) { break; } } But in term of performance, I think is better what you're suggesting, because if we have fewer slots available than `max_batch`, here we were delaying the call to io_submit(). > >Perhaps the following can be used: > > int64_t max_batch = s->aio_context->aio_max_batch ?: DEFAULT_MAX_BATCH; > max_batch = MIN_NON_ZERO(MAX_EVENTS - s->io_q.in_flight + s->io_q.in_queue, max_batch); > Since we will compare `in_queue` with `max_batch`, should we remove it from this expression? I mean: int64_t max_batch = s->aio_context->aio_max_batch ?: DEFAULT_MAX_BATCH; max_batch = MIN_NON_ZERO(MAX_EVENTS - s->io_q.in_flight, max_batch); then as it is in this patch: s->io_q.in_queue++; if (!s->io_q.blocked && (!s->io_q.plugged || s->io_q.in_queue >= max_batch)) { ioq_submit(s); } >Here we'll only need to check against max_batch but it takes into >account MAX_EVENT and in_flight. Thanks, Stefano
On Mon, Jul 19, 2021 at 12:35:53PM +0200, Stefano Garzarella wrote: > On Tue, Jul 13, 2021 at 03:58:04PM +0100, Stefan Hajnoczi wrote: > > On Wed, Jul 07, 2021 at 05:00:19PM +0200, Stefano Garzarella wrote: > > > @@ -371,7 +375,7 @@ static int laio_do_submit(int fd, struct qemu_laiocb *laiocb, off_t offset, > > > s->io_q.in_queue++; > > > if (!s->io_q.blocked && > > > (!s->io_q.plugged || > > > - s->io_q.in_flight + s->io_q.in_queue >= MAX_EVENTS)) { > > > + s->io_q.in_queue >= max_batch)) { > > > > Is it safe to drop the MAX_EVENTS case? > > I think it is safe since in ioq_submit() we have this check while > dequeueing: > > QSIMPLEQ_FOREACH(aiocb, &s->io_q.pending, next) { > iocbs[len++] = &aiocb->iocb; > if (s->io_q.in_flight + len >= MAX_EVENTS) { > break; > } > } > > But in term of performance, I think is better what you're suggesting, > because if we have fewer slots available than `max_batch`, here we were > delaying the call to io_submit(). > > > > > Perhaps the following can be used: > > > > int64_t max_batch = s->aio_context->aio_max_batch ?: DEFAULT_MAX_BATCH; > > max_batch = MIN_NON_ZERO(MAX_EVENTS - s->io_q.in_flight + s->io_q.in_queue, max_batch); > > > > Since we will compare `in_queue` with `max_batch`, should we remove it from > this expression? > > I mean: > > int64_t max_batch = s->aio_context->aio_max_batch ?: DEFAULT_MAX_BATCH; > max_batch = MIN_NON_ZERO(MAX_EVENTS - s->io_q.in_flight, max_batch); > > then as it is in this patch: > > s->io_q.in_queue++; > if (!s->io_q.blocked && > (!s->io_q.plugged || > s->io_q.in_queue >= max_batch)) { > ioq_submit(s); > } Good. Stefan
diff --git a/block/linux-aio.c b/block/linux-aio.c index 3c0527c2bf..8a7bb136fc 100644 --- a/block/linux-aio.c +++ b/block/linux-aio.c @@ -28,6 +28,9 @@ */ #define MAX_EVENTS 1024 +/* Maximum number of requests in a batch. (default value) */ +#define DEFAULT_MAX_BATCH 32 + struct qemu_laiocb { Coroutine *co; LinuxAioState *ctx; @@ -351,6 +354,7 @@ static int laio_do_submit(int fd, struct qemu_laiocb *laiocb, off_t offset, LinuxAioState *s = laiocb->ctx; struct iocb *iocbs = &laiocb->iocb; QEMUIOVector *qiov = laiocb->qiov; + int64_t max_batch = s->aio_context->aio_max_batch ?: DEFAULT_MAX_BATCH; switch (type) { case QEMU_AIO_WRITE: @@ -371,7 +375,7 @@ static int laio_do_submit(int fd, struct qemu_laiocb *laiocb, off_t offset, s->io_q.in_queue++; if (!s->io_q.blocked && (!s->io_q.plugged || - s->io_q.in_flight + s->io_q.in_queue >= MAX_EVENTS)) { + s->io_q.in_queue >= max_batch)) { ioq_submit(s); }
When there are multiple queues attached to the same AIO context, some requests may experience high latency, since in the worst case the AIO engine queue is only flushed when it is full (MAX_EVENTS) or there are no more queues plugged. Commit 2558cb8dd4 ("linux-aio: increasing MAX_EVENTS to a larger hardcoded value") changed MAX_EVENTS from 128 to 1024, to increase the number of in-flight requests. But this change also increased the potential maximum batch to 1024 elements. When there is a single queue attached to the AIO context, the issue is mitigated from laio_io_unplug() that will flush the queue every time is invoked since there can't be others queue plugged. Let's use the new `aio-max-batch` IOThread parameter to mitigate this issue, limiting the number of requests in a batch. We also define a default value (32): this value is obtained running some benchmarks and it represents a good tradeoff between the latency increase while a request is queued and the cost of the io_submit(2) system call. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- block/linux-aio.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)