Re: [PATCH v11 13/13] block: apply COR-filter to block-stream jobs
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
[...]