Re: [PULL 14/53] block: apply COR-filter to block-stream jobs

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

28.01.2021 21:38, Philippe Mathieu-Daudé wrote:
> Hi Andrey,
> 
> On 1/26/21 3:19 PM, Max Reitz wrote:
>> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>
>> This patch completes the series with the COR-filter applied to
>> block-stream operations.
>>
>> Adding the filter makes it possible in future implement discarding
>> copied regions in backing files during the block-stream job, to reduce
>> the disk overuse (we need control on permissions).
>>
>> Also, the filter now is smart enough to do copy-on-read with specified
>> base, so we have benefit on guest reads even when doing block-stream of
>> the part of the backing chain.
>>
>> Several iotests are slightly modified due to filter insertion.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Message-Id: <20201216061703.70908-14-vsementsov@virtuozzo.com>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/stream.c             | 105 ++++++++++++++++++++++---------------
>>   tests/qemu-iotests/030     |   8 +--
>>   tests/qemu-iotests/141.out |   2 +-
>>   tests/qemu-iotests/245     |  20 ++++---
>>   4 files changed, 80 insertions(+), 55 deletions(-)
>>
>> diff --git a/block/stream.c b/block/stream.c
> ...
>> @@ -228,7 +211,9 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>>       bool bs_read_only;
>>       int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
>>       BlockDriverState *base_overlay;
>> +    BlockDriverState *cor_filter_bs = NULL;
>>       BlockDriverState *above_base;
>> +    QDict *opts;
>>   
>>       assert(!(base && bottom));
>>       assert(!(backing_file_str && bottom));
>> @@ -266,30 +251,62 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>>           }
>>       }
>>   
>> -    if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
>> -        return;
>> -    }
>> -
>>       /* Make sure that the image is opened in read-write mode */
>>       bs_read_only = bdrv_is_read_only(bs);
>>       if (bs_read_only) {
>> -        if (bdrv_reopen_set_read_only(bs, false, errp) != 0) {
>> -            bs_read_only = false;
>> -            goto fail;
>> +        int ret;
>> +        /* Hold the chain during reopen */
>> +        if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
>> +            return;
>> +        }
>> +
>> +        ret = bdrv_reopen_set_read_only(bs, false, errp);
>> +
>> +        /* failure, or cor-filter will hold the chain */
>> +        bdrv_unfreeze_backing_chain(bs, above_base);
>> +
>> +        if (ret < 0) {
>> +            return;
>>           }
>>       }
>>   
>> -    /* Prevent concurrent jobs trying to modify the graph structure here, we
>> -     * already have our own plans. Also don't allow resize as the image size is
>> -     * queried only at the job start and then cached. */
>> -    s = block_job_create(job_id, &stream_job_driver, NULL, bs,
>> -                         basic_flags | BLK_PERM_GRAPH_MOD,
>> +    opts = qdict_new();
> 
> Coverity reported (CID 1445793) that this resource could be leaked...
> 
>> +
>> +    qdict_put_str(opts, "driver", "copy-on-read");
>> +    qdict_put_str(opts, "file", bdrv_get_node_name(bs));
>> +    /* Pass the base_overlay node name as 'bottom' to COR driver */
>> +    qdict_put_str(opts, "bottom", base_overlay->node_name);
>> +    if (filter_node_name) {
>> +        qdict_put_str(opts, "node-name", filter_node_name);
>> +    }
>> +
>> +    cor_filter_bs = bdrv_insert_node(bs, opts, BDRV_O_RDWR, errp);
>> +    if (!cor_filter_bs) {
> 
> ... probably here.
> 
> Should we call g_free(opts) here?


Actually, not..

bdrv_insert_node() calls bdrv_open() which eats options even on failure.

I see, CID already marked false-positive?

> 
>> +        goto fail;
>> +    }
>> +
>> +    if (!filter_node_name) {
>> +        cor_filter_bs->implicit = true;
>> +    }
>> +
>> +    s = block_job_create(job_id, &stream_job_driver, NULL, cor_filter_bs,
>> +                         BLK_PERM_CONSISTENT_READ,
>>                            basic_flags | BLK_PERM_WRITE,
>>                            speed, creation_flags, NULL, NULL, errp);
>>       if (!s) {
>>           goto fail;
>>       }
>>   
>> +    /*
>> +     * Prevent concurrent jobs trying to modify the graph structure here, we
>> +     * already have our own plans. Also don't allow resize as the image size is
>> +     * queried only at the job start and then cached.
>> +     */
>> +    if (block_job_add_bdrv(&s->common, "active node", bs, 0,
>> +                           basic_flags | BLK_PERM_WRITE, &error_abort)) {
>> +        goto fail;
>> +    }
>> +
>>       /* Block all intermediate nodes between bs and base, because they will
>>        * disappear from the chain after this operation. The streaming job reads
>>        * every block only once, assuming that it doesn't change, so forbid writes
>> @@ -310,9 +327,9 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>>       s->base_overlay = base_overlay;
>>       s->above_base = above_base;
>>       s->backing_file_str = g_strdup(backing_file_str);
>> +    s->cor_filter_bs = cor_filter_bs;
>>       s->target_bs = bs;
>>       s->bs_read_only = bs_read_only;
>> -    s->chain_frozen = true;
>>   
>>       s->on_error = on_error;
>>       trace_stream_start(bs, base, s);
>> @@ -320,8 +337,10 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>>       return;
>>   
>>   fail:
>> +    if (cor_filter_bs) {
>> +        bdrv_cor_filter_drop(cor_filter_bs);
>> +    }
>>       if (bs_read_only) {
>>           bdrv_reopen_set_read_only(bs, true, NULL);
>>       }
>> -    bdrv_unfreeze_backing_chain(bs, above_base);
>>   }
> ...
> 
> 


-- 
Best regards,
Vladimir