Re: [PATCH] block: End quiescent sections when a BDS is deleted

[Greg Kurz <groug@kaod.org>]

On Fri, 23 Oct 2020 16:18:05 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 23.10.2020 um 12:41 hat Greg Kurz geschrieben:
> > If a BDS gets deleted during blk_drain_all(), it might miss a
> > call to bdrv_do_drained_end(). This means missing a call to
> > aio_enable_external() and the AIO context remains disabled for
> > ever. This can cause a device to become irresponsive and to
> > disrupt the guest execution, ie. hang, loop forever or worse.
> > 
> > This scenario is quite easy to encounter with virtio-scsi
> > on POWER when punching multiple blockdev-create QMP commands
> > while the guest is booting and it is still running the SLOF
> > firmware. This happens because SLOF disables/re-enables PCI
> > devices multiple times via IO/MEM/MASTER bits of PCI_COMMAND
> > register after the initial probe/feature negotiation, as it
> > tends to work with a single device at a time at various stages
> > like probing and running block/network bootloaders without
> > doing a full reset in-between. This naturally generates many
> > dataplane stops and starts, and thus many drain sections that
> > can race with blockdev_create_run(). In the end, SLOF bails
> > out.
> > 
> > It is somehow reproducible on x86 but it requires to generate
> > articial dataplane start/stop activity with stop/cont QMP
> > commands. In this case, seabios ends up looping for ever,
> > waiting for the virtio-scsi device to send a response to
> > a command it never received.
> > 
> > Add a helper that pairs all previously called bdrv_do_drained_begin()
> > with a bdrv_do_drained_end() and call it from bdrv_close().
> > While at it, update the "/bdrv-drain/graph-change/drain_all"
> > test in test-bdrv-drain so that it can catch the issue.
> > 
> > BugId: https://bugzilla.redhat.com/show_bug.cgi?id=1874441
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  block.c                 |    9 +++++++++
> >  block/io.c              |   13 +++++++++++++
> >  include/block/block.h   |    6 ++++++
> >  tests/test-bdrv-drain.c |    1 +
> >  4 files changed, 29 insertions(+)
> > 
> > diff --git a/block.c b/block.c
> > index 430edf79bb10..ddcb36dd48a8 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -4458,6 +4458,15 @@ static void bdrv_close(BlockDriverState *bs)
> >      }
> >      QLIST_INIT(&bs->aio_notifiers);
> >      bdrv_drained_end(bs);
> > +
> > +    /*
> > +     * If we're still inside some bdrv_drain_all_begin()/end() sections, end
> > +     * them now since this BDS won't exist anymore when bdrv_drain_all_end()
> > +     * gets called.
> > +     */
> > +    if (bs->quiesce_counter) {
> > +        bdrv_drained_end_quiesce(bs);
> > +    }
> >  }
> >  
> >  void bdrv_close_all(void)
> > diff --git a/block/io.c b/block/io.c
> > index 54f0968aee27..8a0da06bbb14 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -633,6 +633,19 @@ void bdrv_drain_all_begin(void)
> >      }
> >  }
> >  
> > +void bdrv_drained_end_quiesce(BlockDriverState *bs)
> 
> I think the name should make clear that this is meant as a counterpart
> for bdrv_drain_all_begin(), so maybe bdrv_drain_all_end_quiesce()?
> 

Sure.

> (The function is not suitable for any other kinds of drain because the
> parameters it passes to bdrv_do_drained_end() are only the same as for
> bdrv_drain_all_begin().)
> 
> > +{
> > +    int drained_end_counter = 0;
> > +
> > +    g_assert_cmpint(bs->quiesce_counter, >, 0);
> > +    g_assert_cmpint(bs->refcnt, ==, 0);
> 
> By the way, I didn't know about the problem with these macros either.
> 
> > +    while (bs->quiesce_counter) {
> > +        bdrv_do_drained_end(bs, false, NULL, true, &drained_end_counter);
> > +    }
> > +    BDRV_POLL_WHILE(bs, qatomic_read(&drained_end_counter) > 0);
> > +}
> > +
> >  void bdrv_drain_all_end(void)
> >  {
> >      BlockDriverState *bs = NULL;
> > diff --git a/include/block/block.h b/include/block/block.h
> > index d16c401cb44e..c0ce6e690081 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -779,6 +779,12 @@ void bdrv_drained_end(BlockDriverState *bs);
> >   */
> >  void bdrv_drained_end_no_poll(BlockDriverState *bs, int *drained_end_counter);
> >  
> > +/**
> > + * End all quiescent sections started by bdrv_drain_all_begin(). This is
> > + * only needed when deleting a BDS before bdrv_drain_all_end() is called.
> > + */
> > +void bdrv_drained_end_quiesce(BlockDriverState *bs);
> > +
> >  /**
> >   * End a quiescent section started by bdrv_subtree_drained_begin().
> >   */
> > diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
> > index 1595bbc92e9e..8a29e33e004a 100644
> > --- a/tests/test-bdrv-drain.c
> > +++ b/tests/test-bdrv-drain.c
> > @@ -594,6 +594,7 @@ static void test_graph_change_drain_all(void)
> >  
> >      g_assert_cmpint(bs_b->quiesce_counter, ==, 0);
> >      g_assert_cmpint(b_s->drain_count, ==, 0);
> > +    g_assert_cmpint(qemu_get_aio_context()->external_disable_cnt, ==, 0);
> 
> But here in the test case we should keep g_assert_cmpint() because it
> gives better error messages when it fails (and checkpatch doesn't warn
> about it in tests).
> 

Sure, it makes sense to keep them in tests. The limitation only applies
to the core code.

> Apart from the naming and checkpatch, this looks good to me.
> 

Cool, I'll send a v2 shortly.

> Kevin
>