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

From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-block@nongnu.org
Cc: qemu-devel@nongnu.org, fam@euphon.net, stefanha@redhat.com,
	kwolf@redhat.com, mreitz@redhat.com, armbru@redhat.com,
	jsnow@redhat.com, eblake@redhat.com, den@openvz.org
Subject: Re: [PATCH v12 14/14] block: apply COR-filter to block-stream jobs
Date: Tue, 27 Oct 2020 21:24:24 +0300	[thread overview]
Message-ID: <e2547844-2820-11d8-749a-7c746536c6d1@virtuozzo.com> (raw)
In-Reply-To: <f8a956c7-c2dc-29c7-14b4-8e0d1b2ca9f8@virtuozzo.com>

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.
>>>
>>
>> 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