Re: [PATCH v7 12/13] block: Add bdrv_co_move_to_aio_context()

[Stefan Hajnoczi]

On Fri, Sep 25, 2020 at 06:00:51PM +0200, Kevin Wolf wrote:
> Am 15.09.2020 um 16:31 hat Stefan Hajnoczi geschrieben:
> > On Wed, Sep 09, 2020 at 05:11:48PM +0200, Kevin Wolf wrote:
> > > Add a function to move the current coroutine to the AioContext of a
> > > given BlockDriverState.
> > > 
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  include/block/block.h |  6 ++++++
> > >  block.c               | 10 ++++++++++
> > >  2 files changed, 16 insertions(+)
> > > 
> > > diff --git a/include/block/block.h b/include/block/block.h
> > > index 981ab5b314..80ab322f11 100644
> > > --- a/include/block/block.h
> > > +++ b/include/block/block.h
> > > @@ -626,6 +626,12 @@ bool bdrv_debug_is_suspended(BlockDriverState *bs, 
> > > const char *tag);
> > >   */
> > >  AioContext *bdrv_get_aio_context(BlockDriverState *bs);
> > >  
> > > +/**
> > > + * Move the current coroutine to the AioContext of @bs and return the old
> > > + * AioContext of the coroutine.
> > > + */
> > > +AioContext *coroutine_fn bdrv_co_move_to_aio_context(BlockDriverState 
> > > *bs);
> > 
> > I'm not sure this function handles all cases:
> > 1. Being called without the BQL (i.e. from an IOThread).
> > 2. Being called while a device stops using its IOThread.
> > 
> > The races that come to mind are fetching the AioContext for bs and then
> > scheduling a BH. The BH is executed later on by the event loop. There
> > might be cases where the AioContext for bs is updated before the BH
> > runs.

The scenario I'm thinking about is where bs' AioContext changes while we
are trying to move there.

There is a window of time between fetching bs' AioContext, scheduling a
BH in our old AioContext (not in bs' AioContext), and then scheduling
the coroutine into the AioContext we previously fetched for bs.

Is it possible for the AioContext value we stashed to be outdated by the
time we use it?

I think the answer is that it's safe to use this function from the main
loop thread under the BQL. That way nothing else will change bs'
AioContext while we're running. But it's probably not safe to use this
function from an arbitrary IOThread (without the BQL).

I think this limitation is okay but it needs to be documented. Maybe an
assertion can verify that it holds.

Stefan

signature.asc
Description: PGP signature