Re: [Qemu-block] [PATCH 2/2] block-stream: include base into job node list

[Vladimir Sementsov-Ogievskiy]

On 19.03.2019 19:44, Andrey Shinkevich wrote:
> 
> 
> On 18/03/2019 17:42, Alberto Garcia wrote:
>> On Tue 26 Feb 2019 05:39:41 PM CET, Andrey Shinkevich wrote:
>>> On 26/02/2019 16:33, Alberto Garcia wrote:
>>>> On Mon 25 Feb 2019 05:39:14 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> --- a/block/stream.c
>>>>>> +++ b/block/stream.c
>>>>>> @@ -259,6 +259,15 @@ void stream_start(const char *job_id, 
>>>>>> BlockDriverState *bs,
>>>>>>                                 &error_abort);
>>>>>>          }
>>>>>>      
>>>>>> +    if (base) {
>>>>>> +        /*
>>>>>> +         * The base node should not disappear during the job.
>>>>>> +         */
>>>>>> +        block_job_add_bdrv(&s->common, "base", base, 0,
>>>>>> +                           BLK_PERM_ALL & ~BLK_PERM_GRAPH_MOD,
>>>>>> +                           &error_abort);
>>>>>> +    }
>>>>>> +
>>>>>>          s->base = base;
>>>>>>          s->backing_file_str = g_strdup(backing_file_str);
>>>>>>          s->bs_read_only = bs_read_only;
>>>>>>
>>>>>
>>>>>
>>>>> Hmm, intersting, is BLK_PERM_GRAPH_MOD a good way to lock relations in
>>>>> node graph?
>>>>>
>>>>> bdrv_replace_node don't check this permission. So, I don't understand,
>>>>> how this permission works.. Graph modification operation in difference
>>>>> with read or write are not done through BdrvChild at all.
>>>>>
>>>>> Are there any ideas or work started for some another mechanism of
>>>>> "freezing" a subgraph during an operation?
>>>>
>>>> See this discussion:
>>>>
>>>>       https://lists.gnu.org/archive/html/qemu-block/2018-11/msg00387.html
>>>>
>>>> And these patches (currently under review):
>>>>
>>>>       https://lists.gnu.org/archive/html/qemu-block/2019-01/msg00622.html
>>>
>>> The bdrv_freeze_backing_chain() excludes the base BlockDriverState.
>>> And we would like to protect the base as well when running the
>>> block-stream.
>>   
>> I was just looking into this again. The bdrv_freeze_backing_chain() (now
>> in master) freezes all links between bs and base, both included, so base
>> cannot go away anymore.
>>
>> Is block_job_add_bdrv() still necessary in this scenario?
>>
>> Unless I'm wrong I think that what we can do now is to start getting rid
>> of the op blockers, shouldn't it be possible to get the same
>> functionality with the permission system plus the BdrvChild.frozen
>> attribute ?
>>
>> Berto
>> I have got rid of using the block_job_add_bdrv() for the 'base'.
> But, as for the "intermediate nodes", I will want to add the
> BLK_PERM_WRITE shared flag to them for the 'discard block' feature
> in future. So, I have to check if we can get 'write' permission for
> the block discard operation without invoking block_job_add_bdrv()
> for them...
> 
> Andrey
> 

I think Alberto mean only block_job_add_bdrv for base, and I agree that
we don't need it after we have frozen attribute.

--
Best regards,
Vladimir