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

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> Hi,
> 
> 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 is one reason why this series is needed.  Over time (since v1), a
> second reason has made its way in:
> 
> bs->file is not necessarily the place where a node’s data is stored.
> qcow2 now has external data files, and currently there is no way for the
> general block layer to know that the data is not stored in bs->file.
> Right now, I do not think that has any real consequences (all functions
> that need access to the actual data storage file should only do so as a
> fallback if the driver does not provide some functionality, but qcow2
> should provide it all), but it still shows that we need some way to let
> the general block layer know about such data files.  (Also, I will need
> this for v1 of my “Inquire images’ rotational info” series.)
> 
> I won’t go on and on about this series now, I think the patches pretty
> much speak for themselves now.  If the cover letter gets too long,
> nobody reads it anyway (see previous versions).
> 
> 
> *** This series depends on some others. ***
> 
> Dependencies:
> - [PATCH 0/4] block: Keep track of parent quiescing
> - [PATCH 0/2] vl: Drain before (block) job cancel when quitting
> - [PATCH v2 0/2] blockdev: Overlays are not snapshots
> 
> Based-on: <address@hidden>
> Based-on: <address@hidden>
> Based-on: <address@hidden>

Could you please export a branch?

> 
> 
> v5:
> - Split the huge patches 2 and 3 from the previous version into many
>    smaller patches to maintain the potential reviewers’ sanity [Vladimir]

Thank you! In spite of frightening amount of patches, reviewing became a lot
simpler.

> 
> - Added support for compressed writes to the COR and throttle filter
>    drivers to demonstrate how that looks, because the backup job needs to
>    deal with filters that have such support
> 
> - Added differentiation between bdrv_storage_child(),
>    bdrv_primary_child(), and bdrv_metadata_child()
> 
> - A whole lot of things Vladimir has noted
> 
> - Made the block jobs really work with filters.  In case of commit and
>    stream, this now means that filters go away if they are between top
>    and base.  I think that’s OK because it’s the user’s choice to include
>    filters or not.  (They can move the filters around if they prefer a
>    different result.)
>    - This changes the “Add filter commit test cases” from checking that
>      most things do not work to checking that they do
> 
> - Added the “blockdev: Fix active commit choice” patch because it turned
>    out this became necessary after I allowed committing through and with
>    filters.
> 
> 
> Max Reitz (42):
>    block: Mark commit and mirror as filter drivers
>    copy-on-read: Support compressed writes
>    throttle: Support compressed writes
>    block: Add child access functions
>    block: Add chain helper functions
>    qcow2: Implement .bdrv_storage_child()
>    block: *filtered_cow_child() for *has_zero_init()
>    block: bdrv_set_backing_hd() is about bs->backing
>    block: Include filters when freezing backing chain
>    block: Use CAF in bdrv_is_encrypted()
>    block: Add bdrv_supports_compressed_writes()
>    block: Use bdrv_filtered_rw* where obvious
>    block: Use CAFs in block status functions
>    block: Use CAFs when working with backing chains
>    block: Re-evaluate backing file handling in reopen
>    block: Use child access functions when flushing
>    block: Use CAFs in bdrv_refresh_limits()
>    block: Use CAFs in bdrv_refresh_filename()
>    block: Use CAF in bdrv_co_rw_vmstate()
>    block/snapshot: Fall back to storage child
>    block: Use CAFs for debug breakpoints
>    block: Use CAFs in bdrv_get_allocated_file_size()
>    blockdev: Use CAF in external_snapshot_prepare()
>    block: Use child access functions for QAPI queries
>    mirror: Deal with filters
>    backup: Deal with filters
>    commit: Deal with filters
>    stream: Deal with filters
>    nbd: Use CAF when looking for dirty bitmap
>    qemu-img: Use child access functions
>    block: Drop backing_bs()
>    block: Make bdrv_get_cumulative_perm() public
>    blockdev: Fix active commit choice
>    block: Inline bdrv_co_block_status_from_*()
>    block: Fix check_to_replace_node()
>    iotests: Add tests for mirror @replaces loops
>    block: Leave BDS.backing_file constant
>    iotests: Let complete_and_wait() work with commit
>    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     | 109 ++++---
>   block.c                       | 523 +++++++++++++++++++++++++++++-----
>   block/backup.c                |   9 +-
>   block/blkdebug.c              |   7 +-
>   block/blklogwrites.c          |   1 -
>   block/block-backend.c         |  16 +-
>   block/commit.c                | 100 +++++--
>   block/copy-on-read.c          |  13 +-
>   block/io.c                    | 115 ++++----
>   block/mirror.c                | 113 ++++++--
>   block/qapi.c                  |  42 +--
>   block/qcow2.c                 |   9 +
>   block/snapshot.c              |  74 +++--
>   block/stream.c                |  23 +-
>   block/throttle.c              |  11 +-
>   blockdev.c                    | 139 +++++++--
>   nbd/server.c                  |   6 +-
>   qemu-img.c                    |  36 +--
>   tests/qemu-iotests/020        |  36 +++
>   tests/qemu-iotests/020.out    |  10 +
>   tests/qemu-iotests/040        | 238 ++++++++++++++++
>   tests/qemu-iotests/040.out    |   4 +-
>   tests/qemu-iotests/041        | 270 +++++++++++++++++-
>   tests/qemu-iotests/041.out    |   4 +-
>   tests/qemu-iotests/184.out    |   7 +-
>   tests/qemu-iotests/191.out    |   1 -
>   tests/qemu-iotests/204.out    |   1 +
>   tests/qemu-iotests/228        |   6 +-
>   tests/qemu-iotests/228.out    |   6 +-
>   tests/qemu-iotests/245        |   4 +-
>   tests/qemu-iotests/iotests.py |  10 +-
>   33 files changed, 1610 insertions(+), 339 deletions(-)
> 


-- 
Best regards,
Vladimir