Re: [Qemu-devel] [PATCH 00/11] block: Deal with filters

[Max Reitz]

Let's pretend you didn't see this, as it breaks some iotests... *cough*
*cough*

(This is what I get for last-minute changes and rebases without proper
test running.  Yes, I'm ashamed.)

Max


On 2018-08-09 23:37, Max Reitz wrote:
> Note 1: This series depends on v10 of my “block: Fix some filename
> generation issues” series.
> 
> Based-on: <address@hidden>
> 
> Note 2: This is technically the first part of my active mirror followup.
> But just very technically.  I noticed that that followup started to
> consist of two parts, namely (A) fix filtery things in the block layer,
> and (B) fix active mirror.  So I decided to split it.  This is part A.
> Part B comes later.
> 
> 
> When we introduced filters, we did it a bit casually.  Sure, we talked a
> lot about them before, but that was mostly discussion about where
> implicit filters should be added to the graph (note that we currently
> only have two implicit filters, those being mirror and commit).  But in
> the end, we really just designated some drivers filters (Quorum,
> blkdebug, etc.) and added some specifically (throttle, COR), without
> really looking through the block layer to see where issues might occur.
> 
> It turns out vast areas of the block layer just don't know about filters
> and cannot really handle them.  Many cases will work in practice, in
> others, well, too bad, you cannot use some feature because some part
> deep inside the block layer looks at your filters and thinks they are
> format nodes.
> 
> This series sets out to correct a bit of that.  I lost my head many
> times and I'm sure this series is incomplete in many ways, but it really
> doesn't do any good if it sits on my disk any longer, it needs to go out
> now.
> 
> The most important patches of this series are patches 3 and 4.  These
> introduce functions to encapsulate bs->backing and bs->file accesses.
> Because sometimes, bs->backing means COW, sometimes it means filtered
> node.  And sometimes, bs->file means metadata storage, and sometimes it
> means filtered node.  With this functions, it's always clear what the
> caller wants, and it will always get what it wants.
> 
> Besides that, patch 3 introduces functions to skip filters which may be
> used by parts of the block layer that just don't care about them.
> 
> 
> Secondly, the restraints put on mirror's @replaces parameter are
> revisited and fixed.
> 
> 
> Thirdly, BDS.backing_file is changed to be constant.  I don't quite know
> why we modify it whenever we change a BDS's backing file, but that's
> definitely not quite right.  This fixes things like being able to
> perform a commit on a file (using relative filenames) in a directory
> that's not qemu's CWD.
> 
> 
> Finally, a number of tests are added.
> 
> 
> There are probably many things that are worthy of discussion, of which
> only some come to my head, e.g.:
> 
> - In which cases do we want to skip filters, in which cases do we want
>   to skip implicit filters?
>   My approach was to basically never skip explicitly added filters,
>   except when it's about finding a file in some tree (e.g. in a backing
>   chain).  Maybe there are cases where you think we should skip even
>   explicitly added filters.
> 
> - I made interesting decisions like “When you mirror from a node, we
>   should indeed mirror from that node, but when replacing it, we should
>   skip leave all implicit filters on top intact.”  You may disagree with
>   that.
>   (My reasoning here is that users aren't supposed to know about
>   implicit filters, and therefore, they should not intend to remove
>   them.  Also, mirror accepts only root nodes as the source, so you
>   cannot really specify the node below the implicit filters.  But you
>   can use @replaces to drop the implicit filters, if you know they are
>   there.)
> 
> 
> Max Reitz (11):
>   block: Mark commit and mirror as filter drivers
>   blockdev: Check @replaces in blockdev_mirror_common
>   block: Filtered children access functions
>   block: Storage child access function
>   block: Fix check_to_replace_node()
>   iotests: Add tests for mirror @replaces loops
>   block: Leave BDS.backing_file constant
>   iotests: Add filter commit test cases
>   iotests: Add filter mirror test cases
>   iotests: Add test for commit in sub directory
>   iotests: Test committing to overridden backing
> 
>  qapi/block-core.json           |   4 +
>  include/block/block.h          |   2 +
>  include/block/block_int.h      |  53 +++++-
>  block.c                        | 338 ++++++++++++++++++++++++++++-----
>  block/backup.c                 |   8 +-
>  block/block-backend.c          |  16 +-
>  block/commit.c                 |  38 ++--
>  block/io.c                     |  47 +++--
>  block/mirror.c                 |  39 ++--
>  block/qapi.c                   |  38 ++--
>  block/snapshot.c               |  40 ++--
>  block/stream.c                 |  15 +-
>  blockdev.c                     | 167 ++++++++++++----
>  migration/block-dirty-bitmap.c |   4 +-
>  nbd/server.c                   |   8 +-
>  qemu-img.c                     |  24 ++-
>  tests/qemu-iotests/020         |  36 ++++
>  tests/qemu-iotests/020.out     |  10 +
>  tests/qemu-iotests/040         | 191 +++++++++++++++++++
>  tests/qemu-iotests/040.out     |   4 +-
>  tests/qemu-iotests/041         | 270 +++++++++++++++++++++++++-
>  tests/qemu-iotests/041.out     |   4 +-
>  tests/qemu-iotests/191.out     |   1 -
>  23 files changed, 1133 insertions(+), 224 deletions(-)
>

signature.asc
Description: OpenPGP digital signature