[PATCH v11 13/13] block: apply COR-filter to block-stream jobs

[Andrey Shinkevich]

On 15.10.2020 20:16, Andrey Shinkevich wrote:
> On 14.10.2020 19:24, Max Reitz wrote:
>> On 12.10.20 19:43, Andrey Shinkevich wrote:
> 
> [...]
> 
>>> ---
>>>   block/stream.c             | 93 
>>> +++++++++++++++++++++++++++++-----------------
>>>   tests/qemu-iotests/030     | 51 +++----------------------
>>>   tests/qemu-iotests/030.out |  4 +-
>>>   tests/qemu-iotests/141.out |  2 +-
>>>   tests/qemu-iotests/245     | 19 +++++++---
>>>   5 files changed, 81 insertions(+), 88 deletions(-)
>>
>> Looks like stream_run() could be a bit streamlined now (the allocation
>> checking should be unnecessary, unconditionally calling
>> stream_populate() should be sufficient), but not necessary now.
>>
> 
> That is what I had kept in my mind when I tackled this patch. But there 
> is an underwater reef to streamline. Namely, how the block-stream job 
> gets known about a long unallocated tail to exit the loop earlier in the 
> stream_run(). Shall we return the '-EOF' or another error code from the 
> cor_co_preadv_part() to be handled by the stream_run()? Any other 
> suggestions, if any, will be appreciated.
> 
>>> diff --git a/block/stream.c b/block/stream.c
>>> index d3e1812..93564db 100644
>>> --- a/block/stream.c
>>> +++ b/block/stream.c
>>
>> [...]
> 
>>> +
>>> +    cor_filter_bs = bdrv_cor_filter_append(bs, opts, BDRV_O_RDWR, 
>>> errp);
>>> +    if (cor_filter_bs == NULL) {
>>> +        goto fail;
>>> +    }
>>> +
>>> +    if (bdrv_freeze_backing_chain(cor_filter_bs, bs, errp) < 0) {
>>
>> Is there a reason why we can’t combine this with the
>> bdrv_free_backing_chain() from bs down to above_base?  I mean, the
>> effect should be the same, just asking.
>>
> 
> The bdrv_freeze_backing_chain(bs, above_base, errp) is called before the 
> bdrv_reopen_set_read_only() to keep the backing chain safe during the 
> context switch. Then we will want to freeze the 'COR -> TOP BS' link as 
> well. Freezing/unfreezing parts is simlier to manage than doing that 
> with the whole chain.
> If we decide to invoke the bdrv_reopen_set_read_only() after freezing 
> the backing chain together with the COR-filter, we will not be able to 
> get the 'write' permission on the read-only node.
> 
> 
>>> +        bdrv_cor_filter_drop(cor_filter_bs);
>>> +        cor_filter_bs = NULL;
>>> +        goto fail;
>>> +    }
>>> +
>>> +    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,
>>
>> Not that I’m an expert on the GRAPH_MOD permission, but why is this
>> shared here but not below?  Shouldn’t it be the same in both cases?
>> (Same for taking it as a permission.)
>>
> 
> When we invoke the block_job_add_bdrv(&s->common, "active node", bs,..) 
> below (particularly, we need it to block the operations on the top node, 
> bdrv_op_block_all()), we ask for the GRAPH_MOD permission for the top 
> node. To allow that, the parent filter node should share that permission 
> for the underlying node. Otherwise, we get assertion failed in the 
> bdrv_check_update_perm() called from bdrv_replace_node() when we remove 
> the filter.
> 

I will add my comments above to the code.

Andrey


[...]