qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Qemu-block] [PATCH v2 0/9] block: Delay poll when ending drained sectio


From: Max Reitz
Subject: [Qemu-block] [PATCH v2 0/9] block: Delay poll when ending drained sections
Date: Wed, 19 Jun 2019 17:25:54 +0200

Hi,

This is v2 to “block: Keep track of parent quiescing”.

Please read this cover letter, because I’m very unsure about the design
in this series and I’d appreciate some comments.

As Kevin wrote in his reply to that series, the actual problem is that
bdrv_drain_invoke() polls on every node whenever ending a drain.  This
may cause graph changes, which is actually forbidden.

To solve that problem, this series makes the drain code construct a list
of undrain operations that have been initiated, and then polls all of
them on the root level once graph changes are acceptable.

Note that I don’t like this list concept very much, so I’m open to
alternatives.

Furthermore, all BdrvChildRoles with BDS parents have a broken
.drained_end() implementation.  The documentation clearly states that
this function is not allowed to poll, but it does.  So this series
changes it to a variant (using the new code) that does not poll.

There is a catch, which may actually be a problem, I don’t know: The new
variant of that .drained_end() does not poll at all, never.  As
described above, now every bdrv_drain_invoke() returns an object that
describes when it will be done and which can thus be polled for.  These
objects are just discarded when using BdrvChildRole.drained_end().  That
does not feel quite right.  It would probably be more correct to let
BdrvChildRole.drained_end() return these objects so the top level
bdrv_drained_end() can poll for their completion.

I decided not to do this, for two reasons:
(1) Doing so would spill the “list of objects to poll for” design to
    places outside of block/io.c.  I don’t like the design very much as
    it is, but I can live with it as long as it’s constrained to the
    core drain code in block/io.c.
    This is made worse by the fact that currently, those objects are of
    type BdrvCoDrainData.  But it shouldn’t be a problem to add a new
    type that is externally visible (we only need the AioContext and
    whether bdrv_drain_invoke_entry() is done).

(2) It seems to work as it is.

The alternative would be to add the same GSList ** parameter to
BdrvChildRole.drained_end() that I added in the core drain code in patch
2, and then let the .drained_end() implementation fill that with objects
to poll for.  (Which would be accomplished by adding a frontend to
bdrv_do_drained_end() that lets bdrv_child_cb_drained_poll() pass the
parameter through.)

Opinions?


And then we have bdrv_replace_child_noperm(), which actually wants a
polling BdrvChildRole.drained_end().  So this series adds
BdrvChildRole.drained_end_unquiesce(), which takes that role (if there
is any polling to do).

Note that if I implemented the alternative described above
(.drained_end() gets said GSList ** parameter), a
.drained_end_unquiesce() wouldn’t be necessary.
bdrv_parent_drained_end_single() could just poll the list returned by
.drained_end() by itself.


Finally, patches 1, 8, and 9 are unmodified from v1.


git backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/9:[----] [--] 'block: Introduce BdrvChild.parent_quiesce_counter'
002/9:[down] 'block: Add @data_objs to bdrv_drain_invoke()'
003/9:[down] 'block: Add bdrv_poll_drain_data_objs()'
004/9:[down] 'block: Move polling out of bdrv_drain_invoke()'
005/9:[down] 'block: Add @poll to bdrv_parent_drained_end_single()'
006/9:[down] 'block: Add bdrv_drained_end_no_poll()'
007/9:[down] 'block: Fix BDS children's .drained_end()'
008/9:[----] [--] 'iotests: Add @has_quit to vm.shutdown()'
009/9:[----] [--] 'iotests: Test commit with a filter on the chain'


Max Reitz (9):
  block: Introduce BdrvChild.parent_quiesce_counter
  block: Add @data_objs to bdrv_drain_invoke()
  block: Add bdrv_poll_drain_data_objs()
  block: Move polling out of bdrv_drain_invoke()
  block: Add @poll to bdrv_parent_drained_end_single()
  block: Add bdrv_drained_end_no_poll()
  block: Fix BDS children's .drained_end()
  iotests: Add @has_quit to vm.shutdown()
  iotests: Test commit with a filter on the chain

 include/block/block.h      |  22 +++++-
 include/block/block_int.h  |  23 ++++++
 block.c                    |  24 +++---
 block/io.c                 | 155 ++++++++++++++++++++++++++++++-------
 python/qemu/__init__.py    |   5 +-
 tests/qemu-iotests/040     |  40 +++++++++-
 tests/qemu-iotests/040.out |   4 +-
 tests/qemu-iotests/255     |   2 +-
 8 files changed, 231 insertions(+), 44 deletions(-)

-- 
2.21.0




reply via email to

[Prev in Thread] Current Thread [Next in Thread]