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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 8094 bytes --]

On 12.10.20 19:43, Andrey Shinkevich wrote:
> This patch completes the series with the COR-filter insertion for
> block-stream operations. Adding the filter makes it possible for copied
> regions to be discarded in backing files during the block-stream job,
> what will reduce the disk overuse.
> The COR-filter insertion incurs changes in the iotests case
> 245:test_block_stream_4 that reopens the backing chain during a
> block-stream job. There are changes in the iotests #030 as well.
> The iotests case 030:test_stream_parallel was deleted due to multiple
> conflicts between the concurrent job operations over the same backing
> chain. The base backing node for one job is the top node for another
> job. It may change due to the filter node inserted into the backing
> chain while both jobs are running. Another issue is that the parts of
> the backing chain are being frozen by the running job and may not be
> changed by the concurrent job when needed. The concept of the parallel
> jobs with common nodes is considered vital no more.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>  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.

> diff --git a/block/stream.c b/block/stream.c
> index d3e1812..93564db 100644
> --- a/block/stream.c
> +++ b/block/stream.c

[...]

> @@ -94,13 +94,14 @@ static void stream_clean(Job *job)
>  {
>      StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>      BlockJob *bjob = &s->common;
> -    BlockDriverState *bs = blk_bs(bjob->blk);
> +
> +    bdrv_cor_filter_drop(s->cor_filter_bs);
>  
>      /* Reopen the image back in read-only mode if necessary */
>      if (s->bs_read_only) {
>          /* Give up write permissions before making it read-only */
>          blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);

Perhaps it would be good to do so even before the filter is dropped.  I
don’t know whether moving bjob->blk from cor_filter_bs to target_bs
might cause problems otherwise.

> -        bdrv_reopen_set_read_only(bs, true, NULL);
> +        bdrv_reopen_set_read_only(s->target_bs, true, NULL);
>      }
>  }

[...]

> @@ -262,17 +249,48 @@ 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,
> +    QDict *opts = qdict_new();

Declaration should be done at the start of the block.

> +
> +    qdict_put_str(opts, "driver", "copy-on-read");
> +    qdict_put_str(opts, "file", bdrv_get_node_name(bs));
> +    if (base_overlay) {

@base_overlay is always non-NULL, this condition should check @base, I
think.

> +        /* Pass the base_overlay rather than base */
> +        qdict_put_str(opts, "base", base_overlay->node_name);
> +    }
> +    if (filter_node_name) {
> +        qdict_put_str(opts, "node-name", filter_node_name);
> +    }
> +
> +    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.

> +        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.)

>                           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

[...]

> diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
> index e60c832..940e85a 100755
> --- a/tests/qemu-iotests/245
> +++ b/tests/qemu-iotests/245
> @@ -899,17 +899,26 @@ class TestBlockdevReopen(iotests.QMPTestCase):
>          # make hd1 read-only and block-stream requires it to be read-write
>          # (Which error message appears depends on whether the stream job is
>          # already done with copying at this point.)

Hm.  Let’s look at the set of messages below... [1]

> -        self.reopen(opts, {},
> +        # As the COR-filter node is inserted into the backing chain with the
> +        # 'block-stream' operation, we move the options to their proper nodes.
> +        opts = hd_opts(1)

Oh, so this patch changes it so that only the subtree below hd1 is
reopened, and we don’t have to deal with the filter options.  Got it.
(I think.)

> +        opts['backing'] = hd_opts(2)
> +        opts['backing']['backing'] = None
> +        self.reopen(opts, {'read-only': True},
>              ["Can't set node 'hd1' to r/o with copy-on-read enabled",

[1]

This isn’t done anymore as of this patch.  So I don’t think this error
message can still appear.  Will some other message appear in its stead,
or is it always going to be the second one?

>               "Cannot make block node read-only, there is a writer on it"])
>  
>          # We can't remove hd2 while the stream job is ongoing
> -        opts['backing']['backing'] = None
> -        self.reopen(opts, {'backing.read-only': False}, "Cannot change 'backing' link from 'hd1' to 'hd2'")
> +        opts['backing'] = None
> +        self.reopen(opts, {'read-only': False},
> +                    "Cannot change 'backing' link from 'hd1' to 'hd2'")
>  
> -        # We can detach hd1 from hd0 because it doesn't affect the stream job
> +        # We can't detach hd1 from hd0 because there is the COR-filter implicit
> +        # node in between.
> +        opts = hd_opts(0)
>          opts['backing'] = None
> -        self.reopen(opts)
> +        self.reopen(opts, {},
> +                    "Cannot change backing link if 'hd0' has an implicit backing file")

Does “has an implicit backing file” mean that hd0 has an implicit node
(the COR filter) as its backing file?  And then reopening isn’t allowed
because the user supposedly doesn’t know about that implicit node?  If
so, makes sense.

Max

>  
>          self.vm.run_job('stream0', auto_finalize = False, auto_dismiss = True)
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]