Re: [Qemu-devel] [PATCH v6 27/42] commit: Deal with filters

[Vladimir Sementsov-Ogievskiy]

09.08.2019 19:13, Max Reitz wrote:
> This includes some permission limiting (for example, we only need to
> take the RESIZE permission if the base is smaller than the top).
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>   block/block-backend.c | 16 +++++---
>   block/commit.c        | 96 +++++++++++++++++++++++++++++++------------
>   blockdev.c            |  6 ++-
>   3 files changed, 85 insertions(+), 33 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index c13c5c83b0..0bc592d023 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -2180,11 +2180,17 @@ int blk_commit_all(void)
>           AioContext *aio_context = blk_get_aio_context(blk);
>   
>           aio_context_acquire(aio_context);
> -        if (blk_is_inserted(blk) && blk->root->bs->backing) {
> -            int ret = bdrv_commit(blk->root->bs);
> -            if (ret < 0) {
> -                aio_context_release(aio_context);
> -                return ret;
> +        if (blk_is_inserted(blk)) {
> +            BlockDriverState *non_filter;
> +
> +            /* Legacy function, so skip implicit filters */
> +            non_filter = bdrv_skip_implicit_filters(blk->root->bs);
> +            if (bdrv_filtered_cow_child(non_filter)) {
> +                int ret = bdrv_commit(non_filter);
> +                if (ret < 0) {
> +                    aio_context_release(aio_context);
> +                    return ret;
> +                }
>               }

and if non_filter is explicit filter we just skip it. I think we'd better return
error in this case. For example, just drop if (bdrv_filtered_cow_child) and get
ENOTSUP from bdrv_commit in this case.

And with at least this fixed I'm OK with this patch:

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

However some comments below:

>           }
>           aio_context_release(aio_context);
> diff --git a/block/commit.c b/block/commit.c
> index 5a7672c7c7..40d1c8eeac 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -37,6 +37,7 @@ typedef struct CommitBlockJob {
>       BlockBackend *top;
>       BlockBackend *base;
>       BlockDriverState *base_bs;
> +    BlockDriverState *above_base;

you called it base_overlay in mirror, seems better to keep same naming

>       BlockdevOnError on_error;
>       bool base_read_only;
>       bool chain_frozen;
> @@ -110,7 +111,7 @@ static void commit_abort(Job *job)
>        * XXX Can (or should) we somehow keep 'consistent read' blocked even
>        * after the failed/cancelled commit job is gone? If we already wrote
>        * something to base, the intermediate images aren't valid any more. */
> -    bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs),
> +    bdrv_replace_node(s->commit_top_bs, s->commit_top_bs->backing->bs,
>                         &error_abort);
>   
>       bdrv_unref(s->commit_top_bs);
> @@ -174,7 +175,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
>               break;
>           }
>           /* Copy if allocated above the base */
> -        ret = bdrv_is_allocated_above(blk_bs(s->top), blk_bs(s->base), false,
> +        ret = bdrv_is_allocated_above(blk_bs(s->top), s->above_base, true,
>                                         offset, COMMIT_BUFFER_SIZE, &n);
>           copy = (ret == 1);
>           trace_commit_one_iteration(s, offset, n, ret);
> @@ -267,15 +268,35 @@ void commit_start(const char *job_id, BlockDriverState 
> *bs,
>       CommitBlockJob *s;
>       BlockDriverState *iter;
>       BlockDriverState *commit_top_bs = NULL;
> +    BlockDriverState *filtered_base;
>       Error *local_err = NULL;
> +    int64_t base_size, top_size;
> +    uint64_t perms, iter_shared_perms;
>       int ret;
>   
>       assert(top != bs);
> -    if (top == base) {
> +    if (bdrv_skip_rw_filters(top) == bdrv_skip_rw_filters(base)) {
>           error_setg(errp, "Invalid files for merge: top and base are the 
> same");
>           return;
>       }
>   
> +    base_size = bdrv_getlength(base);
> +    if (base_size < 0) {
> +        error_setg_errno(errp, -base_size, "Could not inquire base image 
> size");
> +        return;
> +    }
> +
> +    top_size = bdrv_getlength(top);
> +    if (top_size < 0) {
> +        error_setg_errno(errp, -top_size, "Could not inquire top image 
> size");
> +        return;
> +    }
> +
> +    perms = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE;
> +    if (base_size < top_size) {
> +        perms |= BLK_PERM_RESIZE;
> +    }
> +
>       s = block_job_create(job_id, &commit_job_driver, NULL, bs, 0, 
> BLK_PERM_ALL,
>                            speed, creation_flags, NULL, NULL, errp);
>       if (!s) {
> @@ -315,17 +336,43 @@ void commit_start(const char *job_id, BlockDriverState 
> *bs,
>   
>       s->commit_top_bs = commit_top_bs;
>   
> -    /* Block all nodes between top and base, because they will
> -     * disappear from the chain after this operation. */
> -    assert(bdrv_chain_contains(top, base));
> -    for (iter = top; iter != base; iter = backing_bs(iter)) {
> -        /* XXX BLK_PERM_WRITE needs to be allowed so we don't block ourselves
> -         * at s->base (if writes are blocked for a node, they are also 
> blocked
> -         * for its backing file). The other options would be a second filter
> -         * driver above s->base. */

This code part is absolutely equal to corresponding in block/mirror.c.. It 
would be great
to put it into a function and reuse. However its not about these series.

> +    /*
> +     * Block all nodes between top and base, because they will
> +     * disappear from the chain after this operation.
> +     * Note that this assumes that the user is fine with removing all
> +     * nodes (including R/W filters) between top and base.  Assuring
> +     * this is the responsibility of the interface (i.e. whoever calls
> +     * commit_start()).
> +     */
> +    s->above_base = bdrv_find_overlay(top, base);
> +    assert(s->above_base);
> +
> +    /*
> +     * The topmost node with
> +     * bdrv_skip_rw_filters(filtered_base) == bdrv_skip_rw_filters(base)
> +     */
> +    filtered_base = bdrv_filtered_cow_bs(s->above_base);
> +    assert(bdrv_skip_rw_filters(filtered_base) == 
> bdrv_skip_rw_filters(base));
> +
> +    /*
> +     * XXX BLK_PERM_WRITE needs to be allowed so we don't block ourselves
> +     * at s->base (if writes are blocked for a node, they are also blocked
> +     * for its backing file). The other options would be a second filter
> +     * driver above s->base.
> +     */
> +    iter_shared_perms = BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE;
> +
> +    for (iter = top; iter != base; iter = bdrv_filtered_bs(iter)) {
> +        if (iter == filtered_base) {
> +            /*
> +             * From here on, all nodes are filters on the base.  This
> +             * allows us to share BLK_PERM_CONSISTENT_READ.
> +             */
> +            iter_shared_perms |= BLK_PERM_CONSISTENT_READ;
> +        }
> +
>           ret = block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
> -                                 BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE,
> -                                 errp);
> +                                 iter_shared_perms, errp);
>           if (ret < 0) {
>               goto fail;
>           }
> @@ -342,9 +389,7 @@ void commit_start(const char *job_id, BlockDriverState 
> *bs,
>       }
>   
>       s->base = blk_new(s->common.job.aio_context,
> -                      BLK_PERM_CONSISTENT_READ
> -                      | BLK_PERM_WRITE
> -                      | BLK_PERM_RESIZE,
> +                      perms,
>                         BLK_PERM_CONSISTENT_READ
>                         | BLK_PERM_GRAPH_MOD
>                         | BLK_PERM_WRITE_UNCHANGED);
> @@ -412,19 +457,22 @@ int bdrv_commit(BlockDriverState *bs)
>       if (!drv)
>           return -ENOMEDIUM;
>   
> -    if (!bs->backing) {
> +    backing_file_bs = bdrv_filtered_cow_bs(bs);

Hmm just note: if in future we'll have cow child which is not bs->backing, a 
lot of code will
fail, as we always assume that cow child is bs->backing. May be, this should be 
commented in
bdrv_filtered_cow_child implementation.

> +
> +    if (!backing_file_bs) {
>           return -ENOTSUP;
>       }
>   
>       if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT_SOURCE, NULL) ||
> -        bdrv_op_is_blocked(bs->backing->bs, BLOCK_OP_TYPE_COMMIT_TARGET, 
> NULL)) {
> +        bdrv_op_is_blocked(backing_file_bs, BLOCK_OP_TYPE_COMMIT_TARGET, 
> NULL))
> +    {
>           return -EBUSY;
>       }
>   
> -    ro = bs->backing->bs->read_only;
> +    ro = backing_file_bs->read_only;
>   
>       if (ro) {
> -        if (bdrv_reopen_set_read_only(bs->backing->bs, false, NULL)) {
> +        if (bdrv_reopen_set_read_only(backing_file_bs, false, NULL)) {
>               return -EACCES;
>           }
>       }
> @@ -440,8 +488,6 @@ int bdrv_commit(BlockDriverState *bs)
>       }
>   
>       /* Insert commit_top block node above backing, so we can write to it */
> -    backing_file_bs = backing_bs(bs);
> -
>       commit_top_bs = bdrv_new_open_driver(&bdrv_commit_top, NULL, 
> BDRV_O_RDWR,
>                                            &local_err);
>       if (commit_top_bs == NULL) {
> @@ -526,15 +572,13 @@ ro_cleanup:
>       qemu_vfree(buf);
>   
>       blk_unref(backing);
> -    if (backing_file_bs) {
> -        bdrv_set_backing_hd(bs, backing_file_bs, &error_abort);
> -    }
> +    bdrv_set_backing_hd(bs, backing_file_bs, &error_abort);

Preexisting, but we should not drop filter if we didn't added it (if we failed 
above filter
insertion). You increased amount of such cases. No damage still.

>       bdrv_unref(commit_top_bs);
>       blk_unref(src);
>   
>       if (ro) {
>           /* ignoring error return here */
> -        bdrv_reopen_set_read_only(bs->backing->bs, true, NULL);
> +        bdrv_reopen_set_read_only(backing_file_bs, true, NULL);
>       }
>   
>       return ret;
> diff --git a/blockdev.c b/blockdev.c
> index c6f79b4e0e..7bef41c0b0 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1094,7 +1094,7 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
>               return;
>           }
>   
> -        bs = blk_bs(blk);
> +        bs = bdrv_skip_implicit_filters(blk_bs(blk));
>           aio_context = bdrv_get_aio_context(bs);
>           aio_context_acquire(aio_context);
>   
> @@ -3454,7 +3454,9 @@ void qmp_block_commit(bool has_job_id, const char 
> *job_id, const char *device,
>   
>       assert(bdrv_get_aio_context(base_bs) == aio_context);
>   
> -    for (iter = top_bs; iter != backing_bs(base_bs); iter = 
> backing_bs(iter)) {
> +    for (iter = top_bs; iter != bdrv_filtered_bs(base_bs);
> +         iter = bdrv_filtered_bs(iter))
> +    {
>           if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_COMMIT_TARGET, errp)) {
>               goto out;
>           }
> 

-- 
Best regards,
Vladimir