Re: [Qemu-devel] [Qemu-block] [PATCH RFC 1/1] Stream block job involves copy-on-read filter

[Vladimir Sementsov-Ogievskiy]

11.02.2019 17:07, Alberto Garcia wrote:
> On Fri 08 Feb 2019 04:29:48 PM CET, Andrey Shinkevich wrote:
>> On 08/02/2019 16:13, Alberto Garcia wrote:
>>> On Wed 23 Jan 2019 12:54:24 PM CET, Andrey Shinkevich wrote:
>>>> +static BlockDriverState *insert_filter(BlockDriverState *bs, Error **errp)
>>>> +{
>>>> +    BlockDriverState *cor_filter_bs;
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    cor_filter_bs = create_filter_node(bs, errp);
>>>> +    if (cor_filter_bs == NULL) {
>>>> +        error_prepend(errp, "Could not create filter node: ");
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    bdrv_set_aio_context(cor_filter_bs, bdrv_get_aio_context(bs));
>>>> +
>>>> +    bdrv_drained_begin(bs);
>>>> +    bdrv_replace_node(bs, cor_filter_bs, &local_err);
>>>> +    bdrv_drained_end(bs);
>>>
>>> I think this was already discussed in the previous version of this
>>> patch: if you insert a copy-on-read filter here then all guest reads
>>> will copy the data from the backing chain, but you don't want to copy
>>> anything below the 'base' node, so the copy-on-read filter needs a
>>> 'base' parameter.
>>>
>> Before that, we still have the unresolved issue with getting the
>> filter as the 'base' input parameter. That happens sometimes because
>> the base is being searched by the file name, which is the same to the
>> one of the filter, rather than by the node name (!)
> 
> I'm not sure if I understand. block-stream has 'base' and 'base-node'
> parameters, and in the first case you would convert the file name to a
> node name.
> 
> stream_start() gets the BlockDriverState, not the file name, so why
> can't you get the node name from there and pass it to the copy-on-read
> filter?
> 
> Berto
> 

The problem is in  the concept of "base" node. The code written in manner
that base is not changed during block job. However, job don't own base and
there is no guarantee that it will not change during the job. Moreover,
when we create a filter above top node, starting several stream jobs to
nearby regions of nodes in common backing chain it leads exatly to this
situation: during the job an unrelated filter may be inserted between
our bottom-intermediate-node and base-node. Or, contra-wise, we can start
with base on some filter, which is removed during our job.

So, my suggestion is:
1. calculate bottome-intermediate-node as soon as possible, at the very
    beginning of qmp-command handling, before any context switch.
2. don't keep link to base-node in job, and operate relatively to
    bottom-intermediate-node
3. and this leads to implementation of block_status() function which will
    take bottom-node parameter instead of base, and may be other similar
    things.


-- 
Best regards,
Vladimir