diff mbox series

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

Message ID 20200909151149.490589-13-kwolf@redhat.com
State New
Headers show
Series monitor: Optionally run handlers in coroutines | expand

Commit Message

Kevin Wolf Sept. 9, 2020, 3:11 p.m. UTC
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(+)

Comments

Stefan Hajnoczi Sept. 15, 2020, 2:31 p.m. UTC | #1
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.

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.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Kevin Wolf Sept. 25, 2020, 4 p.m. UTC | #2
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
Stefan Hajnoczi Sept. 28, 2020, 8:59 a.m. UTC | #3
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
Kevin Wolf Sept. 28, 2020, 10:21 a.m. UTC | #4
Am 28.09.2020 um 10:59 hat Stefan Hajnoczi geschrieben:
> 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).

It's probably the safest to treat it as such. The first part of it (the
window between fetching bs->aio_context and using it) is actually also
true for this ubiquitous sequence:

    AioContext *ctx = bdrv_get_aio_context(bs);
    aio_context_acquire(ctx);

I never really thought about this, but this is only safe in the main
thread. Most of its users are of course monitor command handlers, which
always run in the main thread.

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

Yes, why not.

Maybe we should actually change the interface into a pair of
bdrv_co_enter/leave() functions that also increase bs->in_flight so that
the whole section will complete before the AioContext of bs changes
(changing the AioContext while the handle coroutine has yielded and will
continue to run in the old context would be bad).

block_resize is safe without it, but it might be better to introduce
patterns that will be safe without being extra careful in each command.

Kevin
diff mbox series

Patch

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);
+
 /**
  * Transfer control to @co in the aio context of @bs
  */
diff --git a/block.c b/block.c
index 9538af4884..81403e00d1 100644
--- a/block.c
+++ b/block.c
@@ -6372,6 +6372,16 @@  AioContext *bdrv_get_aio_context(BlockDriverState *bs)
     return bs ? bs->aio_context : qemu_get_aio_context();
 }
 
+AioContext *coroutine_fn bdrv_co_move_to_aio_context(BlockDriverState *bs)
+{
+    Coroutine *self = qemu_coroutine_self();
+    AioContext *old_ctx = qemu_coroutine_get_aio_context(self);
+    AioContext *new_ctx = bdrv_get_aio_context(bs);
+
+    aio_co_reschedule_self(new_ctx);
+    return old_ctx;
+}
+
 void bdrv_coroutine_enter(BlockDriverState *bs, Coroutine *co)
 {
     aio_co_enter(bdrv_get_aio_context(bs), co);