| Message ID | 20240307114744.1135711-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | linux-aio: add IO_CMD_FDSYNC command support | expand |
Am 07.03.2024 um 12:47 hat Prasad Pandit geschrieben: > From: Prasad Pandit <pjp@fedoraproject.org> > > Libaio defines IO_CMD_FDSYNC command to sync all outstanding > asynchronous I/O operations, by flushing out file data to the > disk storage. > > Enable linux-aio to submit such aio request. This helps to > reduce latency induced via pthread_create calls by > thread-pool (aio=threads). > > Signed-off-by: Prasad Pandit <pjp@fedoraproject.org> Kernel support for this is "relatively" new, added in 2018. Should we fall back to the thread pool if we receive -EINVAL? Kevin
Hi, On Thu, 7 Mar 2024 at 19:21, Kevin Wolf <kwolf@redhat.com> wrote: > Kernel support for this is "relatively" new, added in 2018. Should we > fall back to the thread pool if we receive -EINVAL? laio_co_submit laio_do_submit ioq_submit io_submit * When an aio operation is not supported by the kernel, io_submit() call would return '-EINVAL', indicating that the specified (_FDSYNC) operation is invalid for the file descriptor. ie. an error (-EINVAL) needs to reach the 'laio_co_submit' routine above, to make its caller fall back on the thread-pool functionality as default. * Strangely the 'ioq_submit' routine above ignores the return value from io_submit and returns void. I think we need to change that to be able to fall back on the thread-pool functionality. I'll try to see if such a change works as expected. Please let me know if there's another way to fix this. Thank you. --- - Prasad
Am 07.03.2024 um 18:19 hat Prasad Pandit geschrieben: > Hi, > > On Thu, 7 Mar 2024 at 19:21, Kevin Wolf <kwolf@redhat.com> wrote: > > Kernel support for this is "relatively" new, added in 2018. Should we > > fall back to the thread pool if we receive -EINVAL? > > laio_co_submit > laio_do_submit > ioq_submit > io_submit > > * When an aio operation is not supported by the kernel, io_submit() > call would return '-EINVAL', indicating that the specified (_FDSYNC) > operation is invalid for the file descriptor. ie. an error (-EINVAL) > needs to reach the 'laio_co_submit' routine above, to make its caller > fall back on the thread-pool functionality as default. Hm... This might make it more challenging because then not only one specific request fails, but the whole submission batch. Do we know if it can include unrelated requests? > * Strangely the 'ioq_submit' routine above ignores the return value > from io_submit and returns void. I think we need to change that to be > able to fall back on the thread-pool functionality. I'll try to see if > such a change works as expected. Please let me know if there's another > way to fix this. It passes the return value to the request: if (ret < 0) { /* Fail the first request, retry the rest */ aiocb = QSIMPLEQ_FIRST(&s->io_q.pending); QSIMPLEQ_REMOVE_HEAD(&s->io_q.pending, next); s->io_q.in_queue--; aiocb->ret = ret; qemu_laio_process_completion(aiocb); continue; } laio_co_submit() then fetches it from laiocb.ret and returns it to its caller (in this case, your new code). Kevin
Hello Kevin, On Fri, 8 Mar 2024 at 14:35, Kevin Wolf <kwolf@redhat.com> wrote: > Hm... This might make it more challenging because then not only one > specific request fails, but the whole submission batch. * Yes exactly. * I've updated yesterday's patch to return an error (-EINVAL) from ioq_submit to laio_co_submit and fallback on thread-pool as you said. It seems to be working fine when the kernel supports an AIO_FDSYNC call. I'm trying to test it against the Fedora-26 kernel, which was < 4.13.0, and did not support the AIO_FDSYNC call. > Do we know if it can include unrelated requests? * Unrelated requests? > It passes the return value to the request: > > if (ret < 0) { > /* Fail the first request, retry the rest */ > aiocb = QSIMPLEQ_FIRST(&s->io_q.pending); > QSIMPLEQ_REMOVE_HEAD(&s->io_q.pending, next); > s->io_q.in_queue--; > aiocb->ret = ret; > qemu_laio_process_completion(aiocb); > continue; > } > > laio_co_submit() then fetches it from laiocb.ret and returns it to its > caller (in this case, your new code). * I did not get what's happening here. It looks like when 'io_submit' returns an error, ioq_submit plucks out the first 'laiocb' object from QSIMPLEQ and sets the error code (-EINVAL) in it, and continues to submit further requests in the batch. But what if 'io_submit' of those further requests also returns an error? ie. For each error code the first object in the queue is removed and error code is saved in it? But in laio_co_submit() it is only looking at the 'ret' value of one local laiocb object, not all in the batch. So which return code 'ret' is it really checking against -EINPROGRESS? * If a user submits DEFAULT_MAX_BATCH 32=aio requests, in laio_co_submit() there does not seem to be a way to see 'io_submit' return value for all of them, right? Should it check the io_submit status of them all? Thank you. --- - Prasad
Hello Kevin,
On Fri, 8 Mar 2024 at 17:38, Prasad Pandit <ppandit@redhat.com> wrote:
> I'm trying to test it against the Fedora-26 kernel, which was < 4.13.0, and did not support the AIO_FDSYNC call.
[PATCH v2] -> https://lists.nongnu.org/archive/html/qemu-devel/2024-03/msg02495.html
* I've sent v2 of this patch which checks the return value from
'laio_co_submit' function and returns if it is >= zero(0).
* I tested this and previous version of this patch on host kernels
which support IO_CMD_FDSYNC and which don't.
1) When kernel supports IO_CMD_FDSYNC, everything works well. No issues.
2) When kernel does _not_ support IO_CMD_FDSYNC
- With [PATCH v1], guest does not boot, instead it opens a rescue shell
- With [PATCH v2], guest boots and seems to work fine. But
after some time guest kernel threads seem to hang and show traces like
===
INFO: task kworker/u2:0:9 blocked for more than 245 seconds.
INFO: task (tmpfiles):482 blocked for more than 123 seconds.
INFO: task (tmpfiles):482 blocked for more than 368 seconds.
INFO: task systemd-random-:477 blocked for more than 368 seconds.
[ 492.932383] Not tainted 6.6.7-100.fc37.x86_64 #1
[ 492.935404] "echo 0 >
/proc/sys/kernel/hung_task_timeout_secs" disables this message.
===
* I'm not yet sure how to fix this. I'd appreciate if you've suggestions.
Thank you.
---
- Prasad
diff --git a/block/file-posix.c b/block/file-posix.c index 35684f7e21..4ef6c6c9e5 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -2599,6 +2599,11 @@ static int coroutine_fn raw_co_flush_to_disk(BlockDriverState *bs) if (raw_check_linux_io_uring(s)) { return luring_co_submit(bs, s->fd, 0, NULL, QEMU_AIO_FLUSH); } +#endif +#ifdef CONFIG_LINUX_AIO + if (raw_check_linux_aio(s)) { + return laio_co_submit(s->fd, 0, NULL, QEMU_AIO_FLUSH, 0); + } #endif return raw_thread_pool_submit(handle_aiocb_flush, &acb); } diff --git a/block/linux-aio.c b/block/linux-aio.c index ec05d946f3..d940d029e3 100644 --- a/block/linux-aio.c +++ b/block/linux-aio.c @@ -384,6 +384,9 @@ static int laio_do_submit(int fd, struct qemu_laiocb *laiocb, off_t offset, case QEMU_AIO_READ: io_prep_preadv(iocbs, fd, qiov->iov, qiov->niov, offset); break; + case QEMU_AIO_FLUSH: + io_prep_fdsync(iocbs, fd); + break; /* Currently Linux kernel does not support other operations */ default: fprintf(stderr, "%s: invalid AIO request type 0x%x.\n", @@ -412,7 +415,7 @@ int coroutine_fn laio_co_submit(int fd, uint64_t offset, QEMUIOVector *qiov, AioContext *ctx = qemu_get_current_aio_context(); struct qemu_laiocb laiocb = { .co = qemu_coroutine_self(), - .nbytes = qiov->size, + .nbytes = qiov ? qiov->size : 0, .ctx = aio_get_linux_aio(ctx), .ret = -EINPROGRESS, .is_read = (type == QEMU_AIO_READ),