Re: [PATCH v12 14/14] block: apply COR-filter to block-stream jobs

[Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>]

On 27.10.2020 21:24, Andrey Shinkevich wrote:
> 
> On 27.10.2020 20:57, Vladimir Sementsov-Ogievskiy wrote:
>> 27.10.2020 20:48, Andrey Shinkevich wrote:
>>>
>>> On 27.10.2020 19:13, Vladimir Sementsov-Ogievskiy wrote:
>>>> 22.10.2020 21:13, Andrey Shinkevich wrote:
>>>>> This patch completes the series with the COR-filter insertion for
>>>>> block-stream operations. Adding the filter makes it possible for 
>>>>> copied
>>>>> regions to be discarded in backing files during the block-stream job,
>>>>> what will reduce the disk overuse.
>>>>> The COR-filter insertion incurs changes in the iotests case
>>>>> 245:test_block_stream_4 that reopens the backing chain during a
>>>>> block-stream job. There are changes in the iotests #030 as well.
>>>>> The iotests case 030:test_stream_parallel was deleted due to multiple
>>>>> conflicts between the concurrent job operations over the same backing
>>>>> chain. The base backing node for one job is the top node for another
>>>>> job. It may change due to the filter node inserted into the backing
>>>>> chain while both jobs are running. Another issue is that the parts of
>>>>> the backing chain are being frozen by the running job and may not be
>>>>> changed by the concurrent job when needed. The concept of the parallel
>>>>> jobs with common nodes is considered vital no more.
>>>>>
>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>>> ---
>>>>>   block/stream.c             | 98 
>>>>> ++++++++++++++++++++++++++++++----------------
>>>>>   tests/qemu-iotests/030     | 51 +++---------------------
>>>>>   tests/qemu-iotests/030.out |  4 +-
>>>>>   tests/qemu-iotests/141.out |  2 +-
>>>>>   tests/qemu-iotests/245     | 22 +++++++----
>>>>>   5 files changed, 87 insertions(+), 90 deletions(-)
>>>>>
>>>>> diff --git a/block/stream.c b/block/stream.c
>>>
>>>
>>> [...]
>>>
>>>>> +    s = block_job_create(job_id, &stream_job_driver, NULL, 
>>>>> cor_filter_bs,
>>>>> +                         BLK_PERM_CONSISTENT_READ,
>>>>> +                         basic_flags | BLK_PERM_WRITE | 
>>>>> BLK_PERM_GRAPH_MOD,
>>>>
>>>> I think that BLK_PERM_GRAPH_MOD is something outdated. We have 
>>>> chain-feeze, what BLK_PERM_GRAPH_MOD adds to it? I don't know, and 
>>>> doubt that somebody knows.
>>>>
>>>
>>> That is true for the commit/mirror jobs also. If we agree to remove 
>>> the flag BLK_PERM_GRAPH_MOD from all these jobs, it will be made in a 
>>> separate series, won't it?
>>
>> Hmm. At least, let's not implement new logic based on 
>> BLK_PERM_GRAPH_MOD. In original code it's only block_job_create's 
>> perm, not in shared_perm, not somewhere else.. So, if we keep it, 
>> let's keep it as is: only in perm in block_job_create, not 
>> implementing additional perm/shared_perm logic.
>>
> 
> With @perm=0 in the block_job_add_bdrv(&s->common, "active node"...), it 
> won't.
> 
>>>
>>>>>                            speed, creation_flags, NULL, NULL, errp);
>>>>>       if (!s) {
>>>>>           goto fail;
>>>>>       }
>>>>> +    /*
>>>>> +     * Prevent concurrent jobs trying to modify the graph 
>>>>> structure here, we
>>>>> +     * already have our own plans. Also don't allow resize as the 
>>>>> image size is
>>>>> +     * queried only at the job start and then cached.
>>>>> +     */
>>>>> +    if (block_job_add_bdrv(&s->common, "active node", bs,
>>>>> +                           basic_flags | BLK_PERM_GRAPH_MOD,
>>>>
>>>> why not 0, like for other nodes? We don't use this BdrvChild at all, 
>>>> why to requre permissions?
>>>>
>>>
>>> Yes, '0' s right.
>>>
>>>>> +                           basic_flags | BLK_PERM_WRITE, 
>>>>> &error_abort)) {
>>>>> +        goto fail;
>>>>> +    }
>>>>> +
>>>>>       /* Block all intermediate nodes between bs and base, because 
>>>
>>>
>>> [...]
>>>
>>>>> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
>>>>> index dcb4b5d..0064590 100755
>>>>> --- a/tests/qemu-iotests/030
>>>>> +++ b/tests/qemu-iotests/030
>>>>> @@ -227,61 +227,20 @@ class TestParallelOps(iotests.QMPTestCase):
>>>>>           for img in self.imgs:
>>>>>               os.remove(img)
>>>>> -    # Test that it's possible to run several block-stream operations
>>>>> -    # in parallel in the same snapshot chain
>>>>> -    @unittest.skipIf(os.environ.get('QEMU_CHECK_BLOCK_AUTO'), 
>>>>> 'disabled in CI')
>>>>> -    def test_stream_parallel(self):
>>>>
>>>> Didn't we agree to add "bottom" paramter to qmp? Than this test-case 
>>>> can be rewritten using
>>>> node-names and new "bottom" stream argument.
>>>>

The QMP new "bottom" option is passed to the COR-driver. It is done 
withing the stream-job code. So, it works.

>>>
>>> I guess it will not help for the whole test. Particularly, there is 
>>> an issue with freezing the child link to COR-filter of the cuncurrent 
>>> job, then it fails to finish first.
>>
>> We should not have such frozen link, as our bottom node should be 
>> above COR-filter of concurrent job.
>>
>>
> 
> The bdrv_freeze_backing_chain(bs, above_base, errp) does that job. Max 
> insisted on keeping it.
> 
> Andrey

I have kept the test_stream_parallel() deleted in the coming v13 because 
it was agreed to make the above_base node frozen. With this, the test 
case can not pass. It is also true because the operations over the 
COR-filter node are blocked for the parallel jobs.

Andrey