[v4,8/8] linux-aio: share one LinuxAioState within an AioContext

This has better performance because it executes fewer system calls
and does not use a bottom half per disk.

Originally proposed by Ming Lei.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 async.c                            |  23 +++++++
 block/linux-aio.c                  |   3 +
 block/raw-posix.c                  | 119 +++++--------------------------------
 block/raw-win32.c                  |   2 +-
 include/block/aio.h                |  13 ++++
 {block => include/block}/raw-aio.h |   0
 6 files changed, 54 insertions(+), 106 deletions(-)
 rename {block => include/block}/raw-aio.h (100%)

On Thu, Apr 07, 2016 at 06:33:36PM +0200, Paolo Bonzini wrote:
> This has better performance because it executes fewer system calls
> and does not use a bottom half per disk.

Each aio_context_t is initialized for 128 in-flight requests in
laio_init().

Will it be possible to hit the limit now that all drives share the same
aio_context_t?

Stefan

On 19/04/2016 11:09, Stefan Hajnoczi wrote:
>> > This has better performance because it executes fewer system calls
>> > and does not use a bottom half per disk.
> Each aio_context_t is initialized for 128 in-flight requests in
> laio_init().
> 
> Will it be possible to hit the limit now that all drives share the same
> aio_context_t?

It was also possible before, because the virtqueue can be bigger than
128 items; that's why there is logic to submit I/O requests after an
io_get_events.  As usual when the answer seems trivial, am I
misunderstanding your question?

Thanks,

Paolo

On Mon, May 09, 2016 at 06:31:44PM +0200, Paolo Bonzini wrote:
> On 19/04/2016 11:09, Stefan Hajnoczi wrote:
> >> > This has better performance because it executes fewer system calls
> >> > and does not use a bottom half per disk.
> > Each aio_context_t is initialized for 128 in-flight requests in
> > laio_init().
> > 
> > Will it be possible to hit the limit now that all drives share the same
> > aio_context_t?
> 
> It was also possible before, because the virtqueue can be bigger than
> 128 items; that's why there is logic to submit I/O requests after an
> io_get_events.  As usual when the answer seems trivial, am I
> misunderstanding your question?

I'm concerned about a performance regression rather than correctness.

But looking at linux-aio.c there *is* a correctness problem:

  static void ioq_submit(struct qemu_laio_state *s)
  {
      int ret, len;
      struct qemu_laiocb *aiocb;
      struct iocb *iocbs[MAX_QUEUED_IO];
      QSIMPLEQ_HEAD(, qemu_laiocb) completed;

      do {
          len = 0;
          QSIMPLEQ_FOREACH(aiocb, &s->io_q.pending, next) {
              iocbs[len++] = &aiocb->iocb;
              if (len == MAX_QUEUED_IO) {
                  break;
              }
          }

          ret = io_submit(s->ctx, len, iocbs);
          if (ret == -EAGAIN) {
              break;
          }
          if (ret < 0) {
              abort();
          }

          s->io_q.n -= 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));
      s->io_q.blocked = (s->io_q.n > 0);
  }

io_submit() may have submitted some of the requests when -EAGAIN is
returned.  QEMU gets no indication of which requests were submitted.  It
may be possible to dig around in the s->ctx rings to find out or we need
to keep track of the number of in-flight requests so we can prevent ever
hitting EAGAIN.

ioq_submit() pretends that no requests were submitted on -EAGAIN and
will submit them again next time.  This could result in double
completions.

Regarding performance, I'm thinking about a guest with 8 disks (queue
depth 32).  The worst case is when the guest submits 32 requests at once
but the Linux AIO event limit has already been reached.  Then the disk
is starved until other disks' requests complete.

Stefan

Am 10.05.2016 um 11:30 hat Stefan Hajnoczi geschrieben:
> On Mon, May 09, 2016 at 06:31:44PM +0200, Paolo Bonzini wrote:
> > On 19/04/2016 11:09, Stefan Hajnoczi wrote:
> > >> > This has better performance because it executes fewer system calls
> > >> > and does not use a bottom half per disk.
> > > Each aio_context_t is initialized for 128 in-flight requests in
> > > laio_init().
> > > 
> > > Will it be possible to hit the limit now that all drives share the same
> > > aio_context_t?
> > 
> > It was also possible before, because the virtqueue can be bigger than
> > 128 items; that's why there is logic to submit I/O requests after an
> > io_get_events.  As usual when the answer seems trivial, am I
> > misunderstanding your question?
> 
> I'm concerned about a performance regression rather than correctness.
> 
> But looking at linux-aio.c there *is* a correctness problem:
> 
>   static void ioq_submit(struct qemu_laio_state *s)
>   {
>       int ret, len;
>       struct qemu_laiocb *aiocb;
>       struct iocb *iocbs[MAX_QUEUED_IO];
>       QSIMPLEQ_HEAD(, qemu_laiocb) completed;
> 
>       do {
>           len = 0;
>           QSIMPLEQ_FOREACH(aiocb, &s->io_q.pending, next) {
>               iocbs[len++] = &aiocb->iocb;
>               if (len == MAX_QUEUED_IO) {
>                   break;
>               }
>           }
> 
>           ret = io_submit(s->ctx, len, iocbs);
>           if (ret == -EAGAIN) {
>               break;
>           }
>           if (ret < 0) {
>               abort();
>           }
> 
>           s->io_q.n -= 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));
>       s->io_q.blocked = (s->io_q.n > 0);
>   }
> 
> io_submit() may have submitted some of the requests when -EAGAIN is
> returned.  QEMU gets no indication of which requests were submitted.

My understanding (which is based on the manpage rather than code) is
that -EAGAIN is only returned if no request could be submitted. In other
cases, the number of submitted requests is returned (similar to how
short reads work).

> It may be possible to dig around in the s->ctx rings to find out or we
> need to keep track of the number of in-flight requests so we can
> prevent ever hitting EAGAIN.
> 
> ioq_submit() pretends that no requests were submitted on -EAGAIN and
> will submit them again next time.  This could result in double
> completions.

Did you check in the code that this can happen?

> Regarding performance, I'm thinking about a guest with 8 disks (queue
> depth 32).  The worst case is when the guest submits 32 requests at once
> but the Linux AIO event limit has already been reached.  Then the disk
> is starved until other disks' requests complete.

Sounds like a valid concern.

Kevin

On 10/05/2016 11:40, Kevin Wolf wrote:
> > Regarding performance, I'm thinking about a guest with 8 disks (queue
> > depth 32).  The worst case is when the guest submits 32 requests at once
> > but the Linux AIO event limit has already been reached.  Then the disk
> > is starved until other disks' requests complete.
>
> Sounds like a valid concern.

Oh, so you're concerned about the non-dataplane case.  My suspicion is
that, with such a number of outstanding I/O, you probably have bad
performance unless you use dataplane.

Also, aio=threads has had a 64 I/O limit for years and we've never heard
about problems with stuck I/O.  But I agree that it's one area to keep
an eye on.

Paolo

On Tue, May 10, 2016 at 11:40:33AM +0200, Kevin Wolf wrote:
> Am 10.05.2016 um 11:30 hat Stefan Hajnoczi geschrieben:
> > On Mon, May 09, 2016 at 06:31:44PM +0200, Paolo Bonzini wrote:
> > > On 19/04/2016 11:09, Stefan Hajnoczi wrote:
> > > >> > This has better performance because it executes fewer system calls
> > > >> > and does not use a bottom half per disk.
> > > > Each aio_context_t is initialized for 128 in-flight requests in
> > > > laio_init().
> > > > 
> > > > Will it be possible to hit the limit now that all drives share the same
> > > > aio_context_t?
> > > 
> > > It was also possible before, because the virtqueue can be bigger than
> > > 128 items; that's why there is logic to submit I/O requests after an
> > > io_get_events.  As usual when the answer seems trivial, am I
> > > misunderstanding your question?
> > 
> > I'm concerned about a performance regression rather than correctness.
> > 
> > But looking at linux-aio.c there *is* a correctness problem:
> > 
> >   static void ioq_submit(struct qemu_laio_state *s)
> >   {
> >       int ret, len;
> >       struct qemu_laiocb *aiocb;
> >       struct iocb *iocbs[MAX_QUEUED_IO];
> >       QSIMPLEQ_HEAD(, qemu_laiocb) completed;
> > 
> >       do {
> >           len = 0;
> >           QSIMPLEQ_FOREACH(aiocb, &s->io_q.pending, next) {
> >               iocbs[len++] = &aiocb->iocb;
> >               if (len == MAX_QUEUED_IO) {
> >                   break;
> >               }
> >           }
> > 
> >           ret = io_submit(s->ctx, len, iocbs);
> >           if (ret == -EAGAIN) {
> >               break;
> >           }
> >           if (ret < 0) {
> >               abort();
> >           }
> > 
> >           s->io_q.n -= 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));
> >       s->io_q.blocked = (s->io_q.n > 0);
> >   }
> > 
> > io_submit() may have submitted some of the requests when -EAGAIN is
> > returned.  QEMU gets no indication of which requests were submitted.
> 
> My understanding (which is based on the manpage rather than code) is
> that -EAGAIN is only returned if no request could be submitted. In other
> cases, the number of submitted requests is returned (similar to how
> short reads work).

I misread the code:

  /*
   * AKPM: should this return a partial result if some of the IOs were
   * successfully submitted?
   */
  for (i=0; i<nr; i++) {
          struct iocb __user *user_iocb;
          struct iocb tmp;

          if (unlikely(__get_user(user_iocb, iocbpp + i))) {
                  ret = -EFAULT;
                  break;
          }

          if (unlikely(copy_from_user(&tmp, user_iocb, sizeof(tmp)))) {
                  ret = -EFAULT;
                  break;
          }

          ret = io_submit_one(ctx, user_iocb, &tmp, compat);
          if (ret)
                  break;
  }
  blk_finish_plug(&plug);

  percpu_ref_put(&ctx->users);
  return i ? i : ret;

You are right that it will return the number of submitted requests (and
no errno) if a failure occurs partway through.

So the "bug" I found does not exist.

Stefan

On Tue, May 10, 2016 at 12:32:27PM +0200, Paolo Bonzini wrote:
> On 10/05/2016 11:40, Kevin Wolf wrote:
> > > Regarding performance, I'm thinking about a guest with 8 disks (queue
> > > depth 32).  The worst case is when the guest submits 32 requests at once
> > > but the Linux AIO event limit has already been reached.  Then the disk
> > > is starved until other disks' requests complete.
> >
> > Sounds like a valid concern.
> 
> Oh, so you're concerned about the non-dataplane case.  My suspicion is
> that, with such a number of outstanding I/O, you probably have bad
> performance unless you use dataplane.
> 
> Also, aio=threads has had a 64 I/O limit for years and we've never heard
> about problems with stuck I/O.  But I agree that it's one area to keep
> an eye on.

Good point.  In that case I don't feel so bad if we merge this.

Stefan

On Thu, Apr 07, 2016 at 06:33:36PM +0200, Paolo Bonzini wrote:
> This has better performance because it executes fewer system calls
> and does not use a bottom half per disk.
> 
> Originally proposed by Ming Lei.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  async.c                            |  23 +++++++
>  block/linux-aio.c                  |   3 +
>  block/raw-posix.c                  | 119 +++++--------------------------------
>  block/raw-win32.c                  |   2 +-
>  include/block/aio.h                |  13 ++++
>  {block => include/block}/raw-aio.h |   0
>  6 files changed, 54 insertions(+), 106 deletions(-)
>  rename {block => include/block}/raw-aio.h (100%)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>