Re: [Qemu-devel] [PATCH 36/54] commit: Use real permissions in commit block job

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> This is probably one of the most interesting conversions to the new
> op blocker system because a commit block job intentionally leaves some
> intermediate block nodes in the backing chain that aren't valid on their
> own any more; only the whole chain together results in a valid view.
> 
> In order to provide the 'consistent read' permission to the parents of
> the 'top' node of the commit job, a new filter block driver is inserted
> above 'top' which doesn't require 'consistent read' on its backing
> chain. Subsequently, the commit job can block 'consistent read' on all
> intermediate nodes without causing a conflict.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/commit.c | 108 
> ++++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 91 insertions(+), 17 deletions(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index b69586f..9336237 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -36,6 +36,7 @@ typedef struct CommitBlockJob {
>      BlockJob common;
>      RateLimit limit;
>      BlockDriverState *active;
> +    BlockDriverState *commit_top_bs;
>      BlockBackend *top;
>      BlockBackend *base;
>      BlockdevOnError on_error;
> @@ -83,12 +84,19 @@ static void commit_complete(BlockJob *job, void *opaque)
>      BlockDriverState *active = s->active;
>      BlockDriverState *top = blk_bs(s->top);
>      BlockDriverState *base = blk_bs(s->base);
> -    BlockDriverState *overlay_bs = bdrv_find_overlay(active, top);
> +    BlockDriverState *overlay_bs = bdrv_find_overlay(active, 
> s->commit_top_bs);
>      int ret = data->ret;
> +    bool remove_commit_top_bs = false;
>  
>      if (!block_job_is_cancelled(&s->common) && ret == 0) {
>          /* success */
> -        ret = bdrv_drop_intermediate(active, top, base, s->backing_file_str);
> +        ret = bdrv_drop_intermediate(active, s->commit_top_bs, base,
> +                                     s->backing_file_str);
> +    } else if (overlay_bs) {
> +        /* 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. 
> */
> +        remove_commit_top_bs = true;
>      }
>  
>      /* restore base open flags here if appropriate (e.g., change the base 
> back
> @@ -105,6 +113,13 @@ static void commit_complete(BlockJob *job, void *opaque)
>      blk_unref(s->base);
>      block_job_completed(&s->common, ret);
>      g_free(data);
> +
> +    /* If bdrv_drop_intermediate() didn't already do that, remove the commit
> +     * filter driver from the backing chain. Do this as the final step so 
> that
> +     * the 'consistent read' permission can be granted.  */
> +    if (remove_commit_top_bs) {
> +        bdrv_set_backing_hd(overlay_bs, top);
> +    }
>  }
>  
>  static void coroutine_fn commit_run(void *opaque)
> @@ -208,6 +223,34 @@ static const BlockJobDriver commit_job_driver = {
>      .start         = commit_run,
>  };
>  
> +static int coroutine_fn bdrv_commit_top_preadv(BlockDriverState *bs,
> +    uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
> +{
> +    return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);
> +}
> +
> +static void bdrv_commit_top_close(BlockDriverState *bs)
> +{
> +}
> +
> +static void bdrv_commit_top_child_perm(BlockDriverState *bs, BdrvChild *c,
> +                                       const BdrvChildRole *role,
> +                                       uint64_t perm, uint64_t shared,
> +                                       uint64_t *nperm, uint64_t *nshared)
> +{
> +    *nperm = 0;
> +    *nshared = BLK_PERM_ALL;
> +}
> +
> +/* Dummy node that provides consistent read to its users without requiring it
> + * from its backing file and that allows writes on the backing file chain. */
> +static BlockDriver bdrv_commit_top = {
> +    .format_name        = "commit_top",
> +    .bdrv_co_preadv     = bdrv_commit_top_preadv,
> +    .bdrv_close         = bdrv_commit_top_close,
> +    .bdrv_child_perm    = bdrv_commit_top_child_perm,
> +};
> +
>  void commit_start(const char *job_id, BlockDriverState *bs,
>                    BlockDriverState *base, BlockDriverState *top, int64_t 
> speed,
>                    BlockdevOnError on_error, const char *backing_file_str,
> @@ -219,6 +262,7 @@ void commit_start(const char *job_id, BlockDriverState 
> *bs,
>      int orig_base_flags;
>      BlockDriverState *iter;
>      BlockDriverState *overlay_bs;
> +    BlockDriverState *commit_top_bs = NULL;
>      Error *local_err = NULL;
>      int ret;
>  
> @@ -235,7 +279,6 @@ void commit_start(const char *job_id, BlockDriverState 
> *bs,
>          return;
>      }
>  
> -    /* FIXME Use real permissions */
>      s = block_job_create(job_id, &commit_job_driver, bs, 0, BLK_PERM_ALL,
>                           speed, BLOCK_JOB_DEFAULT, NULL, NULL, errp);
>      if (!s) {
> @@ -262,34 +305,62 @@ void commit_start(const char *job_id, BlockDriverState 
> *bs,
>          }
>      }
>  
> +    /* Insert commit_top block node above top, so we can block consistent 
> read
> +     * on the backing chain below it */
> +    commit_top_bs = bdrv_new_open_driver(&bdrv_commit_top, NULL, BDRV_O_RDWR,

Why RDWR when the driver only allows reads anyway?

> +                                         errp);
> +    if (commit_top_bs == NULL) {
> +        goto fail;
> +    }
> +
> +    bdrv_set_backing_hd(commit_top_bs, top);
> +    bdrv_set_backing_hd(overlay_bs, commit_top_bs);
> +
> +    s->commit_top_bs = commit_top_bs;
> +    bdrv_unref(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 != backing_bs(base); iter = backing_bs(iter)) {
> -        /* FIXME Use real permissions */
> -        block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
> -                           BLK_PERM_ALL, &error_abort);
> +    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.

As far as I can see, the loop doesn't even touch base, though...?

>                         The other options would be a second filter driver 
> above
> +         * s->base. */
> +        ret = block_job_add_bdrv(&s->common, "intermediate node", iter, 0,

Don't we need CONSISTENT_READ at least for top?

> +                                 BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE,
> +                                 errp);
> +        if (ret < 0) {
> +            goto fail;
> +        }
>      }
> +
> +    ret = block_job_add_bdrv(&s->common, "base", base, 0, BLK_PERM_ALL, 
> errp);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
>      /* overlay_bs must be blocked because it needs to be modified to
> -     * update the backing image string, but if it's the root node then
> -     * don't block it again */
> -    if (bs != overlay_bs) {
> -        /* FIXME Use real permissions */
> -        block_job_add_bdrv(&s->common, "overlay of top", overlay_bs, 0,
> -                           BLK_PERM_ALL, &error_abort);
> +     * update the backing image string. */
> +    ret = block_job_add_bdrv(&s->common, "overlay of top", overlay_bs,
> +                             BLK_PERM_GRAPH_MOD, BLK_PERM_ALL, errp);
> +    if (ret < 0) {
> +        goto fail;
>      }
>  
> -    /* FIXME Use real permissions */
> -    s->base = blk_new(0, BLK_PERM_ALL);
> +    s->base = blk_new(BLK_PERM_CONSISTENT_READ

Do we actually need CONSISTENT_READ for the base?

Max

> +                      | BLK_PERM_WRITE
> +                      | BLK_PERM_RESIZE,
> +                      BLK_PERM_CONSISTENT_READ
> +                      | BLK_PERM_GRAPH_MOD
> +                      | BLK_PERM_WRITE_UNCHANGED);
>      ret = blk_insert_bs(s->base, base, errp);
>      if (ret < 0) {
>          goto fail;
>      }
>  
> -    /* FIXME Use real permissions */
> +    /* Required permissions are already taken with block_job_add_bdrv() */
>      s->top = blk_new(0, BLK_PERM_ALL);
> -    ret = blk_insert_bs(s->top, top, errp);
> +    blk_insert_bs(s->top, top, errp);
>      if (ret < 0) {
>          goto fail;
>      }
> @@ -314,6 +385,9 @@ fail:
>      if (s->top) {
>          blk_unref(s->top);
>      }
> +    if (commit_top_bs) {
> +        bdrv_set_backing_hd(overlay_bs, top);
> +    }
>      block_job_unref(&s->common);
>  }
>  
>

signature.asc
Description: OpenPGP digital signature