Re: [PATCH v6 4/4] block: apply COR-filter to block-stream jobs

From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-block@nongnu.org
Cc: kwolf@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org,
	den@openvz.org, mreitz@redhat.com, jsnow@redhat.com
Subject: Re: [PATCH v6 4/4] block: apply COR-filter to block-stream jobs
Date: Mon, 24 Aug 2020 11:49:37 +0300	[thread overview]
Message-ID: <3d71e88f-7769-717b-8f68-a2bba52ee388@virtuozzo.com> (raw)
In-Reply-To: <b1153a54-96c8-3ddc-e4ee-42c613d02924@virtuozzo.com>

[-- Attachment #1: Type: text/plain, Size: 9937 bytes --]

On 24.08.2020 11:20, Vladimir Sementsov-Ogievskiy wrote:
> 23.08.2020 22:28, Andrey Shinkevich wrote:
>> On 19.08.2020 13:46, Vladimir Sementsov-Ogievskiy wrote:
>>> 19.08.2020 00:24, Andrey Shinkevich wrote:
>>>> The patch completes the series with the COR-filter insertion to any
>>>> block-stream operation. It also makes changes to the iotests 030.
>>>> The test case 'test_stream_parallel' was deleted due to multiple
>>>> errors.
>>>
>> ...
>>>
>>>>
>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>> ---
>>>>   block/stream.c             | 76 
>>>> ++++++++++++++++++++++++++++++++--------------
>>>>   tests/qemu-iotests/030     | 50 +++---------------------------
>>>>   tests/qemu-iotests/030.out |  4 +--
>>>>   3 files changed, 61 insertions(+), 69 deletions(-)
>>>>
>>>> diff --git a/block/stream.c b/block/stream.c
>>>> index 8bf6b6d..0b11979 100644
>>>> --- a/block/stream.c
>>>> +++ b/block/stream.c
>>>> @@ -19,6 +19,7 @@
>>>>   #include "qapi/qmp/qerror.h"
>>>>   #include "qemu/ratelimit.h"
>>>>   #include "sysemu/block-backend.h"
>>>> +#include "block/copy-on-read.h"
>>>>     enum {
>>>>       /*
>>>> @@ -33,8 +34,11 @@ typedef struct StreamBlockJob {
>>>>       BlockJob common;
>>>>       BlockDriverState *base_overlay; /* COW overlay (stream from 
>>>> this) */
>>>>       BlockDriverState *above_base;   /* Node directly above the 
>>>> base */
>>>> +    BlockDriverState *cor_filter_bs;
>>>> +    BlockDriverState *target_bs;
>>>>       BlockdevOnError on_error;
>>>>       char *backing_file_str;
>>>> +    char *base_fmt;
>>>>       bool bs_read_only;
>>>>       bool chain_frozen;
>>>>   } StreamBlockJob;
>>>> @@ -53,34 +57,26 @@ static void stream_abort(Job *job)
>>>>       StreamBlockJob *s = container_of(job, StreamBlockJob, 
>>>> common.job);
>>>>         if (s->chain_frozen) {
>>>> -        BlockJob *bjob = &s->common;
>>>> -        bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), 
>>>> s->above_base);
>>>> +        bdrv_unfreeze_backing_chain(s->cor_filter_bs, s->above_base);
>>>>       }
>>>>   }
>>>>     static int stream_prepare(Job *job)
>>>>   {
>>>>       StreamBlockJob *s = container_of(job, StreamBlockJob, 
>>>> common.job);
>>>> -    BlockJob *bjob = &s->common;
>>>> -    BlockDriverState *bs = blk_bs(bjob->blk);
>>>> +    BlockDriverState *bs = s->target_bs;
>>>>       BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
>>>>       BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
>>>>       Error *local_err = NULL;
>>>>       int ret = 0;
>>>>   -    bdrv_unfreeze_backing_chain(bs, s->above_base);
>>>> +    bdrv_unfreeze_backing_chain(s->cor_filter_bs, s->above_base);
>>>>       s->chain_frozen = false;
>>>>         if (bdrv_cow_child(unfiltered_bs)) {
>>>> -        const char *base_id = NULL, *base_fmt = NULL;
>>>> -        if (base) {
>>>> -            base_id = s->backing_file_str;
>>>> -            if (base->drv) {
>>>> -                base_fmt = base->drv->format_name;
>>>> -            }
>>>> -        }
>>>>           bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
>>>> -        ret = bdrv_change_backing_file(unfiltered_bs, base_id, 
>>>> base_fmt);
>>>> +        ret = bdrv_change_backing_file(unfiltered_bs, 
>>>> s->backing_file_str,
>>>> +                                       s->base_fmt);
>>>>           if (local_err) {
>>>>               error_report_err(local_err);
>>>>               return -EPERM;
>>>> @@ -94,7 +90,9 @@ static void stream_clean(Job *job)
>>>>   {
>>>>       StreamBlockJob *s = container_of(job, StreamBlockJob, 
>>>> common.job);
>>>>       BlockJob *bjob = &s->common;
>>>> -    BlockDriverState *bs = blk_bs(bjob->blk);
>>>> +    BlockDriverState *bs = s->target_bs;
>>>> +
>>>> +    bdrv_cor_filter_drop(s->cor_filter_bs);
>>>>         /* Reopen the image back in read-only mode if necessary */
>>>>       if (s->bs_read_only) {
>>>> @@ -104,13 +102,14 @@ static void stream_clean(Job *job)
>>>>       }
>>>>         g_free(s->backing_file_str);
>>>> +    g_free(s->base_fmt);
>>>>   }
>>>>     static int coroutine_fn stream_run(Job *job, Error **errp)
>>>>   {
>>>>       StreamBlockJob *s = container_of(job, StreamBlockJob, 
>>>> common.job);
>>>>       BlockBackend *blk = s->common.blk;
>>>> -    BlockDriverState *bs = blk_bs(blk);
>>>> +    BlockDriverState *bs = s->target_bs;
>>>>       BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
>>>>       bool enable_cor = !bdrv_cow_child(s->base_overlay);
>>>>       int64_t len;
>>>> @@ -231,6 +230,12 @@ void stream_start(const char *job_id, 
>>>> BlockDriverState *bs,
>>>>       int basic_flags = BLK_PERM_CONSISTENT_READ | 
>>>> BLK_PERM_WRITE_UNCHANGED;
>>>>       BlockDriverState *base_overlay = bdrv_find_overlay(bs, base);
>>>>       BlockDriverState *above_base;
>>>> +    BlockDriverState *cor_filter_bs = NULL;
>>>> +    char *base_fmt = NULL;
>>>> +
>>>> +    if (base && base->drv) {
>>>> +        base_fmt = g_strdup(base->drv->format_name);
>>>> +    }
>>>>         if (!base_overlay) {
>>>>           error_setg(errp, "'%s' is not in the backing chain of '%s'",
>>>> @@ -264,17 +269,36 @@ void stream_start(const char *job_id, 
>>>> BlockDriverState *bs,
>>>>           }
>>>>       }
>>>>   -    /* 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,
>>>> -                         basic_flags | BLK_PERM_WRITE,
>>>> +    cor_filter_bs = bdrv_cor_filter_append(bs, filter_node_name, 
>>>> errp);
>>>> +    if (cor_filter_bs == NULL) {
>>>> +        goto fail;
>>>> +    }
>>>> +
>>>> +    if (bdrv_freeze_backing_chain(cor_filter_bs, bs, errp) < 0) {
>>>> +        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,
>>>>                            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,
>>>> +                           basic_flags | BLK_PERM_GRAPH_MOD,
>>>> +                           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
>>>> @@ -294,6 +318,9 @@ void stream_start(const char *job_id, 
>>>> BlockDriverState *bs,
>>>>         s->base_overlay = base_overlay;
>>>>       s->above_base = above_base;
>>>> +    s->cor_filter_bs = cor_filter_bs;
>>>> +    s->target_bs = bs;
>>>> +    s->base_fmt = base_fmt;
>>>
>>> it's wrong to keep base_fmt during the job, as base may change
>>
>>
>> So, the format can differ from that of the backing_file_str given as 
>> the input parameter of the stream_start()...
>>
>> ...while the backing_file_str path is written to the QCOW2 header on 
>> a disk.
>>
>
> Or better, let's try to revert c624b015bf14f and freeze base again.

The reversion won't help as the patch "stream: Deal with filters" did 
that work already. We have to freeze the base again. I guess it will 
discard the concept of the 'base_overlay' and the  'above_base'  
introduced by Max in the series "block: Deal withfilters".

Andrey

>
>>
>>
>>>
>>>>       s->backing_file_str = g_strdup(backing_file_str);
>>>>       s->bs_read_only = bs_read_only;
>>>>       s->chain_frozen = true;
>>>> @@ -307,5 +334,10 @@ fail:
>>>>       if (bs_read_only) {
>>>>           bdrv_reopen_set_read_only(bs, true, NULL);
>>>>       }
>>>> -    bdrv_unfreeze_backing_chain(bs, above_base);
>>>> +    if (cor_filter_bs) {
>>>> +        bdrv_unfreeze_backing_chain(cor_filter_bs, above_base);
>>>> +        bdrv_cor_filter_drop(cor_filter_bs);
>>>> +    } else {
>>>> +        bdrv_unfreeze_backing_chain(bs, above_base);
>>>> +    }
>>>>   }
>>>> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
>>>> index 1cdd7e2..fec9d89 100755
>>>> --- a/tests/qemu-iotests/030
>>>> +++ b/tests/qemu-iotests/030
>>>> @@ -221,60 +221,20 @@ class TestParallelOps(iotests.QMPTestCase):
>>>>           for img in self.imgs:
>>>>               os.remove(img)
>> ...
>
>

[-- Attachment #2: Type: text/html, Size: 21934 bytes --]