diff mbox series

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

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

Commit Message

Greg Kurz Oct. 23, 2020, 3:01 p.m. UTC
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(+)

Comments

Kevin Wolf Oct. 27, 2020, 11:16 a.m. UTC | #1
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
Stefan Hajnoczi Oct. 27, 2020, 1:54 p.m. UTC | #2
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.
Greg Kurz Oct. 27, 2020, 3:24 p.m. UTC | #3
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
Kevin Wolf Oct. 27, 2020, 3:41 p.m. UTC | #4
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 mbox series

Patch

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);