| Message ID | 160346526998.272601.9045392804399803158.stgit@bahia.lan |
|---|---|
| State | Accepted |
| Commit | 1a6d3bd229d429879a85a9105fb84cae049d083c |
| Headers | show |
| Series | [v2] block: End quiescent sections when a BDS is deleted | expand |
Am 23.10.2020 um 17:01 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> Thanks, applied to the block branch. Kevin
On Fri, Oct 23, 2020 at 05:01:10PM +0200, Greg Kurz wrote: > +/** > + * 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_drain_all_end_quiesce(BlockDriverState *bs); This function is only called from block.c. Can it be moved to the private block_int.h header? The code is not clear on whether bdrv_drain_all_end_quiesce() is an API that others can use or an internal helper function that must only be called by bdrv_close(). I came to the conclusion that the latter is true after reviewing the patch. Please update the bdrv_drain_all_end_quiesce() doc comment to clarify that this function is an internal helper for bdrv_close() - no one else needs to worry about it.
On Tue, 27 Oct 2020 13:54:04 +0000 Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Fri, Oct 23, 2020 at 05:01:10PM +0200, Greg Kurz wrote: > > +/** > > + * 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_drain_all_end_quiesce(BlockDriverState *bs); > > This function is only called from block.c. Can it be moved to the > private block_int.h header? > Ha, I wasn't aware of block_int.h... It seems to be a very good idea. > The code is not clear on whether bdrv_drain_all_end_quiesce() is an API > that others can use or an internal helper function that must only be > called by bdrv_close(). I came to the conclusion that the latter is true > after reviewing the patch. > Yes it is. > Please update the bdrv_drain_all_end_quiesce() doc comment to clarify > that this function is an internal helper for bdrv_close() - no one else > needs to worry about it. I'll do that. Thanks for the suggestions Stefan. Cheers, -- Greg
Am 27.10.2020 um 16:24 hat Greg Kurz geschrieben: > On Tue, 27 Oct 2020 13:54:04 +0000 > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > On Fri, Oct 23, 2020 at 05:01:10PM +0200, Greg Kurz wrote: > > > +/** > > > + * 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_drain_all_end_quiesce(BlockDriverState *bs); > > > > This function is only called from block.c. Can it be moved to the > > private block_int.h header? > > > > Ha, I wasn't aware of block_int.h... It seems to be a very good idea. > > > The code is not clear on whether bdrv_drain_all_end_quiesce() is an API > > that others can use or an internal helper function that must only be > > called by bdrv_close(). I came to the conclusion that the latter is true > > after reviewing the patch. > > > > Yes it is. > > > Please update the bdrv_drain_all_end_quiesce() doc comment to clarify > > that this function is an internal helper for bdrv_close() - no one else > > needs to worry about it. > > I'll do that. > > Thanks for the suggestions Stefan. I already sent a pull request, so if you're going to change something, please make it a follow-up patch rather than a new patch version. Kevin
diff --git a/block.c b/block.c index 430edf79bb10..ee5b28a9798d 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_drain_all_end_quiesce(bs); + } } void bdrv_close_all(void) diff --git a/block/io.c b/block/io.c index 54f0968aee27..c3a345a911e6 100644 --- a/block/io.c +++ b/block/io.c @@ -633,6 +633,19 @@ void bdrv_drain_all_begin(void) } } +void bdrv_drain_all_end_quiesce(BlockDriverState *bs) +{ + int drained_end_counter = 0; + + g_assert(bs->quiesce_counter > 0); + g_assert(!bs->refcnt); + + 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..809987017631 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_drain_all_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); bdrv_unref(bs_b); blk_unref(blk_b);
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> --- v2: - use g_assert() in core code (checkpatch) - rename helper to bdrv_drain_all_end_quiesce() (Kevin) --- block.c | 9 +++++++++ block/io.c | 13 +++++++++++++ include/block/block.h | 6 ++++++ tests/test-bdrv-drain.c | 1 + 4 files changed, 29 insertions(+)