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

[Kevin Wolf]

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()?

(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).

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

Kevin