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

[Stefan Hajnoczi <stefanha@redhat.com>]

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

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

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