On Fri, 23 Oct 2020 16:18:05 +0200 Kevin Wolf <kw...@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 <gr...@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 >