Re: [PATCH v4 00/15] Apply COR-filter to the block-stream permanently

[Vladimir Sementsov-Ogievskiy]

12.05.2020 19:50, Andrey Shinkevich wrote:
> With this series, all the block-stream COR operations pass through
> the COR-filter. The patches 01-08/15 are taken from the series
> "block: Deal with filters" by Max Reitz, the full version of that
> can be found in the branches:
>
>        https://git.xanclic.moe/XanClic/qemu child-access-functions-v6
>        https://github.com/XanClic/qemu child-access-functions-v6
>
>        When running iotests, apply "char-socket: Fix race condition"
>        to avoid sporadic segmentation faults.
> v4:
>    01: Initialization of the COR-filter BDRVStateCOR member.

Hmm... but 01 doesn't touch COR-filter

> v3:
>    01: The COR filter insert/remove functions moved to block/copy-on-read.c
>        to be a part of API.
>    02: block/stream.c code refactoring.
>    03: The separate call to block_job_add_bdrv() is used to block operations
>        on the active node after the filter inserted and the job created.
>    04: The iotests case 030::test_overlapping_4 was modified to unbound
>        the block-stream job from the base node.
>    05: The COR driver functions preadv/pwritev replaced with their analogous
>        preadv/pwritev_part.

I assume, these changes are about your patches, which are 09-15, and Max's 
patches
are unchanged, right?

> v2:
>    01: No more skipping filters while checking for operation blockers.
>        However, we exclude filters between the bottom node and base
>        because we do not set the operation blockers for filters anymore.
>    02: As stated above, we do not set the operation blockers for filters
>        anymore. So, skip filters when we block operations for the target
>        node.
>    03: The comment added for the patch 4/7.
>    04: The QAPI target version changed to 5.1.
>    05: The 'filter-node-name' now excluded from using in the test #030.
>        If we need it no more in a final version of the series, the patch
>        5/7 may be removed.
>    06: The COR-filter included into the frozen chain of a block-stream job.
>        The 'above_base' node pointer is left because it is essential for
>        finding the base node in case of filters above.
>
>
> Andrey Shinkevich (7):
>    block: prepare block-stream for using COR-filter
>    copy-on-read: Support change filename functions
>    copy-on-read: Support preadv/pwritev_part functions
>    copy-on-read: add filter append/drop functions
>    qapi: add filter-node-name to block-stream
>    iotests: prepare 245 for using filter in block-stream
>    block: apply COR-filter to block-stream jobs
>
> Max Reitz (8):
>    block: Mark commit and mirror as filter drivers

this is for commit

>    copy-on-read: Support compressed writes

for stream

>    block: Add child access functions

I do think, that for these series we need only filtered child and nothing more

>    block: Add chain helper functions
>    block: Include filters when freezing backing chain
>    block: Use CAFs in block status functions
>    commit: Deal with filters when blocking intermediate nodes
>    block: Use CAFs when working with backing chains

So, fix stream, commit and some thing used in it.


Hi Max! Could you take a brief look and say, could we proceed in this way, 
taking part of your old series? How much it conflicts with your plans?

Let me clarify. This all is needed, as we have old proposed feature (and 
patches): discarding blocks from intermediate images during block-stream. It 
helps to save disk space during stream process. And the correct way to get 
access to intermediate nodes (to be able to discard from them) is to append a 
filter. Firstly we proposed our own filter, but that was proposed on list to 
use existing COR filter for stream and it seemed a correct way. So we are 
trying to insert this COR filter.

And the problem with it that without your series it breaks iotest 30, which 
does different magic with parallel stream and commit on the same backing chain.

So, it was my proposal to extract something from your series, to make this test 
work. And the result is here. I thought that the necessary part of your series 
for stream/commit is smaller.. But still, 8 patches is not too much. The 
feature for stream is being postponed already for more than a year due to this 
trouble. We need to proceed somehow. And the feature is useful.


--
Best regards,
Vladimir