Re: [PATCH v2] linux-aio: add IO_CMD_FDSYNC command support

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3816 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]