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

[Alberto Garcia]

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