Re: [PATCH v5 1/4] linux-aio: use LinuxAioState from the running thread

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Wed, Mar 08, 2023 at 12:42:11PM +0100, Kevin Wolf wrote:
> Am 07.03.2023 um 15:18 hat Stefan Hajnoczi geschrieben:
> > On Tue, Mar 07, 2023 at 09:48:51AM +0100, Kevin Wolf wrote:
> > > Am 01.03.2023 um 17:16 hat Stefan Hajnoczi geschrieben:
> > > > On Fri, Feb 03, 2023 at 08:17:28AM -0500, Emanuele Giuseppe Esposito wrote:
> > > > > Remove usage of aio_context_acquire by always submitting asynchronous
> > > > > AIO to the current thread's LinuxAioState.
> > > > > 
> > > > > In order to prevent mistakes from the caller side, avoid passing LinuxAioState
> > > > > in laio_io_{plug/unplug} and laio_co_submit, and document the functions
> > > > > to make clear that they work in the current thread's AioContext.
> > > > > 
> > > > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > > > > ---
> > > > >  include/block/aio.h               |  4 ----
> > > > >  include/block/raw-aio.h           | 18 ++++++++++++------
> > > > >  include/sysemu/block-backend-io.h |  6 ++++++
> > > > >  block/file-posix.c                | 10 +++-------
> > > > >  block/linux-aio.c                 | 29 +++++++++++++++++------------
> > > > >  5 files changed, 38 insertions(+), 29 deletions(-)
> > > > > 
> > > > > diff --git a/include/block/aio.h b/include/block/aio.h
> > > > > index 8fba6a3584..b6b396cfcb 100644
> > > > > --- a/include/block/aio.h
> > > > > +++ b/include/block/aio.h
> > > > > @@ -208,10 +208,6 @@ struct AioContext {
> > > > >      struct ThreadPool *thread_pool;
> > > > >  
> > > > >  #ifdef CONFIG_LINUX_AIO
> > > > > -    /*
> > > > > -     * State for native Linux AIO.  Uses aio_context_acquire/release for
> > > > > -     * locking.
> > > > > -     */
> > > > >      struct LinuxAioState *linux_aio;
> > > > >  #endif
> > > > >  #ifdef CONFIG_LINUX_IO_URING
> > > > > diff --git a/include/block/raw-aio.h b/include/block/raw-aio.h
> > > > > index f8cda9df91..db614472e6 100644
> > > > > --- a/include/block/raw-aio.h
> > > > > +++ b/include/block/raw-aio.h
> > > > > @@ -49,14 +49,20 @@
> > > > >  typedef struct LinuxAioState LinuxAioState;
> > > > >  LinuxAioState *laio_init(Error **errp);
> > > > >  void laio_cleanup(LinuxAioState *s);
> > > > > -int coroutine_fn laio_co_submit(BlockDriverState *bs, LinuxAioState *s, int fd,
> > > > > -                                uint64_t offset, QEMUIOVector *qiov, int type,
> > > > > -                                uint64_t dev_max_batch);
> > > > > +
> > > > > +/* laio_co_submit: submit I/O requests in the thread's current AioContext. */
> > > > > +int coroutine_fn laio_co_submit(int fd, uint64_t offset, QEMUIOVector *qiov,
> > > > > +                                int type, uint64_t dev_max_batch);
> > > > > +
> > > > >  void laio_detach_aio_context(LinuxAioState *s, AioContext *old_context);
> > > > >  void laio_attach_aio_context(LinuxAioState *s, AioContext *new_context);
> > > > > -void laio_io_plug(BlockDriverState *bs, LinuxAioState *s);
> > > > > -void laio_io_unplug(BlockDriverState *bs, LinuxAioState *s,
> > > > > -                    uint64_t dev_max_batch);
> > > > > +
> > > > > +/*
> > > > > + * laio_io_plug/unplug work in the thread's current AioContext, therefore the
> > > > > + * caller must ensure that they are paired in the same IOThread.
> > > > > + */
> > > > > +void laio_io_plug(void);
> > > > > +void laio_io_unplug(uint64_t dev_max_batch);
> > > > >  #endif
> > > > >  /* io_uring.c - Linux io_uring implementation */
> > > > >  #ifdef CONFIG_LINUX_IO_URING
> > > > > diff --git a/include/sysemu/block-backend-io.h b/include/sysemu/block-backend-io.h
> > > > > index 031a27ba10..d41698ccc5 100644
> > > > > --- a/include/sysemu/block-backend-io.h
> > > > > +++ b/include/sysemu/block-backend-io.h
> > > > > @@ -74,8 +74,14 @@ void blk_iostatus_set_err(BlockBackend *blk, int error);
> > > > >  int blk_get_max_iov(BlockBackend *blk);
> > > > >  int blk_get_max_hw_iov(BlockBackend *blk);
> > > > >  
> > > > > +/*
> > > > > + * blk_io_plug/unplug are thread-local operations. This means that multiple
> > > > > + * IOThreads can simultaneously call plug/unplug, but the caller must ensure
> > > > > + * that each unplug() is called in the same IOThread of the matching plug().
> > > > > + */
> > > > >  void blk_io_plug(BlockBackend *blk);
> > > > >  void blk_io_unplug(BlockBackend *blk);
> > > > > +
> > > > >  AioContext *blk_get_aio_context(BlockBackend *blk);
> > > > >  BlockAcctStats *blk_get_stats(BlockBackend *blk);
> > > > >  void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk,
> > > > > diff --git a/block/file-posix.c b/block/file-posix.c
> > > > > index fa227d9d14..fa99d1c25a 100644
> > > > > --- a/block/file-posix.c
> > > > > +++ b/block/file-posix.c
> > > > > @@ -2095,10 +2095,8 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset,
> > > > >  #endif
> > > > >  #ifdef CONFIG_LINUX_AIO
> > > > >      } else if (s->use_linux_aio) {
> > > > > -        LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
> > > > >          assert(qiov->size == bytes);
> > > > > -        return laio_co_submit(bs, aio, s->fd, offset, qiov, type,
> > > > > -                              s->aio_max_batch);
> > > > > +        return laio_co_submit(s->fd, offset, qiov, type, s->aio_max_batch);
> > > > 
> > > > I'm having second thoughts here. This is correct in an IOThread today,
> > > > but the main loop thread case concerns me:
> > > > 
> > > > This patch changes behavior when the main loop or vCPU thread submits
> > > > I/O. Before, the IOThread's LinuxAioState would be used. Now the main
> > > > loop's LinuxAioState will be used instead and aio callbacks will be
> > > > invoked in the main loop thread instead of the IOThread.
> > > 
> > > You mean we have a device that has a separate iothread, but a request is
> > > submitted from the main thread? This isn't even allowed today; if a node
> > > is in an iothread, all I/O must be submitted from that iothread. Do you
> > > know any code that does submit I/O from the main thread instead?
> > 
> > I think you're right. My mental model was outdated. Both the coroutine
> > and non-coroutine code paths schedule coroutines in the AioContext.
> > 
> > However, I think this patch series is still risky because it could
> > reveal latent bugs. Let's merge it in the next development cycle (soft
> > freeze is today!) to avoid destabilizing 8.0.
> 
> Makes sense, I've already started a block-next anyway.
> 
> So is this an R-b or A-b or nothing for now?

I'm happy with it and I've read the code:

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

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