| Message ID | 20240311054333.1264776-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] linux-aio: add IO_CMD_FDSYNC command support | expand |
On Mon, Mar 11, 2024 at 11:13:33AM +0530, Prasad Pandit wrote: > 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> > --- > block/file-posix.c | 12 ++++++++++++ > block/linux-aio.c | 5 ++++- > 2 files changed, 16 insertions(+), 1 deletion(-) > > v2: if IO_CMD_FDSYNC is not supported by the kernel, > fallback on thread-pool flush. > -> https://lists.nongnu.org/archive/html/qemu-devel/2024-03/msg01986.html > > diff --git a/block/file-posix.c b/block/file-posix.c > index 35684f7e21..4f2195d01d 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -2599,6 +2599,18 @@ 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)) { > + ret = laio_co_submit(s->fd, 0, NULL, QEMU_AIO_FLUSH, 0); > + if (ret >= 0) { > + /* > + * if AIO_FLUSH is supported return > + * else fallback on thread-pool flush. > + */ > + return ret; > + } Falling back every time on an older host kernel might be a noticeable performance regression. That can be avoided with a variable that keeps track of whether -EINVAL was seen before and skips Linux AIO in that case. However, it appears that popular distributions starting from Debian 10, Ubuntu 20.04, Fedora 27, CentOS 8, and OpenSUSE Leap 15.5 have the necessary minimum Linux 4.18 kernel: https://repology.org/project/linux/versions Fallback should be very rare, so I don't think it needs to be optimized: Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > + } > #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), > -- > 2.44.0 >
Am 11.03.2024 um 14:09 hat Stefan Hajnoczi geschrieben: > On Mon, Mar 11, 2024 at 11:13:33AM +0530, Prasad Pandit wrote: > > 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> > > --- > > block/file-posix.c | 12 ++++++++++++ > > block/linux-aio.c | 5 ++++- > > 2 files changed, 16 insertions(+), 1 deletion(-) > > > > v2: if IO_CMD_FDSYNC is not supported by the kernel, > > fallback on thread-pool flush. > > -> https://lists.nongnu.org/archive/html/qemu-devel/2024-03/msg01986.html > > > > diff --git a/block/file-posix.c b/block/file-posix.c > > index 35684f7e21..4f2195d01d 100644 > > --- a/block/file-posix.c > > +++ b/block/file-posix.c > > @@ -2599,6 +2599,18 @@ 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)) { > > + ret = laio_co_submit(s->fd, 0, NULL, QEMU_AIO_FLUSH, 0); > > + if (ret >= 0) { > > + /* > > + * if AIO_FLUSH is supported return > > + * else fallback on thread-pool flush. > > + */ > > + return ret; > > + } > > Falling back every time on an older host kernel might be a noticeable > performance regression. That can be avoided with a variable that keeps > track of whether -EINVAL was seen before and skips Linux AIO in that > case. > > However, it appears that popular distributions starting from Debian 10, > Ubuntu 20.04, Fedora 27, CentOS 8, and OpenSUSE Leap 15.5 have the > necessary minimum Linux 4.18 kernel: > https://repology.org/project/linux/versions > > Fallback should be very rare, so I don't think it needs to be optimized: > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> We might need this approach for a different reason: This is an io_submit() error, so while we retry the flush with the fallback path, other requests in the same batch may incorrectly return errors. This probably explains the errors Prasad saw in the guest when the kernel doesn't have support for flush in Linux AIO. So in order to avoid this, we'll probably have to send one flush just to probe (while making sure that no other request is pending - maybe immediately when opening the image?) and then remember whether it worked. Or we'd have to change the error handling around io_submit(), so that we don't always fail the first request in the batch, but first fail any flushes and only then the rest of the requests. Kevin
On Mon, Mar 11, 2024 at 04:40:05PM +0100, Kevin Wolf wrote: > Am 11.03.2024 um 14:09 hat Stefan Hajnoczi geschrieben: > > On Mon, Mar 11, 2024 at 11:13:33AM +0530, Prasad Pandit wrote: > > > 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> > > > --- > > > block/file-posix.c | 12 ++++++++++++ > > > block/linux-aio.c | 5 ++++- > > > 2 files changed, 16 insertions(+), 1 deletion(-) > > > > > > v2: if IO_CMD_FDSYNC is not supported by the kernel, > > > fallback on thread-pool flush. > > > -> https://lists.nongnu.org/archive/html/qemu-devel/2024-03/msg01986.html > > > > > > diff --git a/block/file-posix.c b/block/file-posix.c > > > index 35684f7e21..4f2195d01d 100644 > > > --- a/block/file-posix.c > > > +++ b/block/file-posix.c > > > @@ -2599,6 +2599,18 @@ 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)) { > > > + ret = laio_co_submit(s->fd, 0, NULL, QEMU_AIO_FLUSH, 0); > > > + if (ret >= 0) { > > > + /* > > > + * if AIO_FLUSH is supported return > > > + * else fallback on thread-pool flush. > > > + */ > > > + return ret; > > > + } > > > > Falling back every time on an older host kernel might be a noticeable > > performance regression. That can be avoided with a variable that keeps > > track of whether -EINVAL was seen before and skips Linux AIO in that > > case. > > > > However, it appears that popular distributions starting from Debian 10, > > Ubuntu 20.04, Fedora 27, CentOS 8, and OpenSUSE Leap 15.5 have the > > necessary minimum Linux 4.18 kernel: > > https://repology.org/project/linux/versions > > > > Fallback should be very rare, so I don't think it needs to be optimized: > > > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > We might need this approach for a different reason: This is an > io_submit() error, so while we retry the flush with the fallback path, > other requests in the same batch may incorrectly return errors. This > probably explains the errors Prasad saw in the guest when the kernel > doesn't have support for flush in Linux AIO. > > So in order to avoid this, we'll probably have to send one flush just to > probe (while making sure that no other request is pending - maybe > immediately when opening the image?) and then remember whether it > worked. > > Or we'd have to change the error handling around io_submit(), so that we > don't always fail the first request in the batch, but first fail any > flushes and only then the rest of the requests. I don't see the behavior you are describing in the code. My interpretation of ioq_submit() is that only the flush request fails. Other queued requests (before and after the flush) are submitted successfully: static void ioq_submit(LinuxAioState *s) { int ret, len; struct qemu_laiocb *aiocb; struct iocb *iocbs[MAX_EVENTS]; QSIMPLEQ_HEAD(, qemu_laiocb) completed; do { if (s->io_q.in_flight >= MAX_EVENTS) { break; } len = 0; QSIMPLEQ_FOREACH(aiocb, &s->io_q.pending, next) { iocbs[len++] = &aiocb->iocb; if (s->io_q.in_flight + len >= MAX_EVENTS) { break; } } ret = io_submit(s->ctx, len, iocbs); if (ret == -EAGAIN) { break; } 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; } s->io_q.in_flight += ret; s->io_q.in_queue -= ret; aiocb = container_of(iocbs[ret - 1], struct qemu_laiocb, iocb); QSIMPLEQ_SPLIT_AFTER(&s->io_q.pending, aiocb, next, &completed); } while (ret == len && !QSIMPLEQ_EMPTY(&s->io_q.pending)); Have I missed something? Thanks, Stefan
Am 11.03.2024 um 20:36 hat Stefan Hajnoczi geschrieben: > On Mon, Mar 11, 2024 at 04:40:05PM +0100, Kevin Wolf wrote: > > Am 11.03.2024 um 14:09 hat Stefan Hajnoczi geschrieben: > > > On Mon, Mar 11, 2024 at 11:13:33AM +0530, Prasad Pandit wrote: > > > > 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> > > > > --- > > > > block/file-posix.c | 12 ++++++++++++ > > > > block/linux-aio.c | 5 ++++- > > > > 2 files changed, 16 insertions(+), 1 deletion(-) > > > > > > > > v2: if IO_CMD_FDSYNC is not supported by the kernel, > > > > fallback on thread-pool flush. > > > > -> https://lists.nongnu.org/archive/html/qemu-devel/2024-03/msg01986.html > > > > > > > > diff --git a/block/file-posix.c b/block/file-posix.c > > > > index 35684f7e21..4f2195d01d 100644 > > > > --- a/block/file-posix.c > > > > +++ b/block/file-posix.c > > > > @@ -2599,6 +2599,18 @@ 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)) { > > > > + ret = laio_co_submit(s->fd, 0, NULL, QEMU_AIO_FLUSH, 0); > > > > + if (ret >= 0) { > > > > + /* > > > > + * if AIO_FLUSH is supported return > > > > + * else fallback on thread-pool flush. > > > > + */ > > > > + return ret; > > > > + } > > > > > > Falling back every time on an older host kernel might be a noticeable > > > performance regression. That can be avoided with a variable that keeps > > > track of whether -EINVAL was seen before and skips Linux AIO in that > > > case. > > > > > > However, it appears that popular distributions starting from Debian 10, > > > Ubuntu 20.04, Fedora 27, CentOS 8, and OpenSUSE Leap 15.5 have the > > > necessary minimum Linux 4.18 kernel: > > > https://repology.org/project/linux/versions > > > > > > Fallback should be very rare, so I don't think it needs to be optimized: > > > > > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > > > We might need this approach for a different reason: This is an > > io_submit() error, so while we retry the flush with the fallback path, > > other requests in the same batch may incorrectly return errors. This > > probably explains the errors Prasad saw in the guest when the kernel > > doesn't have support for flush in Linux AIO. > > > > So in order to avoid this, we'll probably have to send one flush just to > > probe (while making sure that no other request is pending - maybe > > immediately when opening the image?) and then remember whether it > > worked. > > > > Or we'd have to change the error handling around io_submit(), so that we > > don't always fail the first request in the batch, but first fail any > > flushes and only then the rest of the requests. > > I don't see the behavior you are describing in the code. My > interpretation of ioq_submit() is that only the flush request fails. > Other queued requests (before and after the flush) are submitted > successfully: > [...] You're right. I missed that io_submit() returns failure only if the first request in the queue is invalid, and returns a "short submission" for errors in later entries. Kevin
Hello, On Tue, 12 Mar 2024 at 15:15, Kevin Wolf <kwolf@redhat.com> wrote: > Am 11.03.2024 um 20:36 hat Stefan Hajnoczi geschrieben: > > > > That can be avoided with a variable that keeps track of whether -EINVAL was seen before and skips Linux AIO in that > > > > case. > > > > > > > > Fallback should be very rare, so I don't think it needs to be optimized: > You're right. I missed that io_submit() returns failure only if the > first request in the queue is invalid, and returns a "short submission" > for errors in later entries. === +bool laio_has_fdsync(int fd) +{ + AioContext *ctx = qemu_get_current_aio_context(); + struct qemu_laiocb cb = { + .co = qemu_coroutine_self(), + .ctx = aio_get_linux_aio(ctx), + }; + struct iocb *iocbs[] = {&cb.iocb, NULL}; + + /* check if host kernel supports IO_CMD_FDSYNC */ + io_prep_fdsync(&cb.iocb, fd); + int ret = io_submit(cb.ctx->ctx, 1, iocbs); + + return ret != -EINVAL; +} === To confirm: * Do we need a revised patch V3? I'm testing one with the above function to check if IO_CMD_FDSYNC is supported. If it returns true we call laio_co_submit(..., QEMU_AIO_FLUSH, ), else fallback to thread-pool. Thank you. --- - Prasad
On Tue, Mar 12, 2024 at 07:07:04PM +0530, Prasad Pandit wrote: > Hello, > > On Tue, 12 Mar 2024 at 15:15, Kevin Wolf <kwolf@redhat.com> wrote: > > Am 11.03.2024 um 20:36 hat Stefan Hajnoczi geschrieben: > > > > > That can be avoided with a variable that keeps track of whether -EINVAL was seen before and skips Linux AIO in that > > > > > case. > > > > > > > > > > Fallback should be very rare, so I don't think it needs to be optimized: > > You're right. I missed that io_submit() returns failure only if the > > first request in the queue is invalid, and returns a "short submission" > > for errors in later entries. > > === > +bool laio_has_fdsync(int fd) > +{ > + AioContext *ctx = qemu_get_current_aio_context(); > + struct qemu_laiocb cb = { > + .co = qemu_coroutine_self(), > + .ctx = aio_get_linux_aio(ctx), > + }; > + struct iocb *iocbs[] = {&cb.iocb, NULL}; > + > + /* check if host kernel supports IO_CMD_FDSYNC */ > + io_prep_fdsync(&cb.iocb, fd); > + int ret = io_submit(cb.ctx->ctx, 1, iocbs); > + > + return ret != -EINVAL; > +} > === > > To confirm: > * Do we need a revised patch V3? I'm testing one with the above > function to check if IO_CMD_FDSYNC is supported. If it returns true we > call laio_co_submit(..., QEMU_AIO_FLUSH, ), else fallback to > thread-pool. If you are already developing and testing it then I think a v3 with laio_has_fdsync() would be great. Thanks, Stefan
diff --git a/block/file-posix.c b/block/file-posix.c index 35684f7e21..4f2195d01d 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -2599,6 +2599,18 @@ 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)) { + ret = laio_co_submit(s->fd, 0, NULL, QEMU_AIO_FLUSH, 0); + if (ret >= 0) { + /* + * if AIO_FLUSH is supported return + * else fallback on thread-pool flush. + */ + return ret; + } + } #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),