Re: [PATCH v7 35/47] commit: Deal with filters

[Kevin Wolf]

Am 25.06.2020 um 17:22 hat Max Reitz geschrieben:
> 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 <mreitz@redhat.com>
> ---
>  block/block-backend.c          |  9 +++-
>  block/commit.c                 | 96 +++++++++++++++++++++++++---------
>  block/monitor/block-hmp-cmds.c |  2 +-
>  blockdev.c                     |  4 +-
>  4 files changed, 81 insertions(+), 30 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 6936b25c83..7f2c7dbccc 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -2271,8 +2271,13 @@ 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);

The old code didn't try to commit nodes that don't have a backing file.

> +        if (blk_is_inserted(blk)) {
> +            BlockDriverState *non_filter;
> +            int ret;
> +
> +            /* Legacy function, so skip implicit filters */
> +            non_filter = bdrv_skip_implicit_filters(blk->root->bs);
> +            ret = bdrv_commit(non_filter);

The new one tries unconditionally. For nodes without a backing file,
bdrv_commit() will return -ENOTSUP, so the whole function fails.

(First real bug at patch 35. I almost thought I wouldn't find any!)

>              if (ret < 0) {
>                  aio_context_release(aio_context);
>                  return ret;
> diff --git a/block/commit.c b/block/commit.c
> index 7732d02dfe..4122b6736d 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -37,6 +37,7 @@ typedef struct CommitBlockJob {
>      BlockBackend *top;
>      BlockBackend *base;
>      BlockDriverState *base_bs;
> +    BlockDriverState *base_overlay;
>      BlockdevOnError on_error;
>      bool base_read_only;
>      bool chain_frozen;

Hm, again this mysterious base_overlay. I know that stream introduced it
to avoid freezing the link to base, but commit doesn't seem to do that.

Is it to avoid using the block status of filter drivers between
base_overlay and base? If so, I guess that goes back to the question I
raised earlier in this series: What is the block status supposed to tell
for filter nodes?

But anyway, in contrast to mirror, commit actually freezes the chain
between commit_top_bs and base, so it should be safe at least.

> @@ -89,7 +90,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);
> @@ -153,7 +154,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->base_overlay, true,
>                                        offset, COMMIT_BUFFER_SIZE, &n);
>          copy = (ret == 1);
>          trace_commit_one_iteration(s, offset, n, ret);
> @@ -253,15 +254,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_filters(top) == bdrv_skip_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;
> +    }

base_perms would indicate which permissions these are (particularly
because it's not the next thing that requires permissions, but only used
further down the function).

>      s = block_job_create(job_id, &commit_job_driver, NULL, bs, 0, 
> BLK_PERM_ALL,
>                           speed, creation_flags, NULL, NULL, errp);
>      if (!s) {
> @@ -301,17 +322,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. */
> +    /*
> +     * 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->base_overlay = bdrv_find_overlay(top, base);
> +    assert(s->base_overlay);
> +
> +    /*
> +     * The topmost node with
> +     * bdrv_skip_filters(filtered_base) == bdrv_skip_filters(base)
> +     */
> +    filtered_base = bdrv_cow_bs(s->base_overlay);
> +    assert(bdrv_skip_filters(filtered_base) == bdrv_skip_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_filter_or_cow_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;
>          }
> @@ -328,9 +375,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);
> @@ -398,19 +443,22 @@ int bdrv_commit(BlockDriverState *bs)
>      if (!drv)
>          return -ENOMEDIUM;
>  
> -    if (!bs->backing) {
> +    backing_file_bs = bdrv_cow_bs(bs);
> +
> +    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;
>          }
>      }
> @@ -428,8 +476,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) {
> @@ -515,15 +561,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);

This means that bdrv_set_backing_hd() is now called to undo a change
that hasn't even been made yet. This fails (with &error_abort) if the
backing chain is frozen.

On the other hand, the other bdrv_set_backing_hd() calls in the same
function would fail the same way.

>      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;

Kevin