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

[Kevin Wolf <kwolf@redhat.com>]

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

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.

Updating the AioContext of a node drains the BDS first, so it would
execute any BHs still pending in the old AioContext. So this part looks
safe to me.

I'm not sure what you mean with the BQL part. I don't think I'm
accessing anything protected by the BQL?

> I didn't investigate these cases but wanted to mention them in case you
> want to add doc comments about when this function can be used or if
> you'd like to verify them yourself.

One thing that I'm not completely sure about (but that isn't really
related to this specific patch) is AioContext changes later in the
process when the actual command handler has yielded. We may have to be
careful to prevent those for coroutine based QMP commands in the block
layer.

By sheer luck, qmp_block_resize() creates a new BlockBackend that has
blk->allow_aio_context_change = false. So we're actually good in the
only command I'm converting. Phew.

Kevin

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