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

[Andrey Shinkevich]

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