Re: [Qemu-block] [PATCH 2/5] commit: Support multiple roots above top node

[Eric Blake]

On 09/25/2017 07:28 AM, Kevin Wolf wrote:
> This changes the commit block job to support operation in a graph where
> there is more than a single active layer that references the top node.
> 
> This involves inserting the commit filter node not only on the path
> between the given active node and the top node, but between the top node
> and all of its parents.
> 
> On completion, bdrv_drop_intermediate() must consider all parents for
> updating the backing file link. These parents may be backing files
> themselves and such read-only; reopen them temporarily if necessary.

s/such/as such/

> Previously this was achieved by the bdrv_reopen() calls in the commit
> block job that made overlay_bs read-write for the whole duration of the
> block job, even though write access is only needed on completion.

Do we know, at the start of the operation, enough to reject attempts
that will eventually fail because we cannot obtain write access at
completion, without having to wait for all the intermediate work before
the completion phase is reached?  If not, that might be a bit of a
regression (where a command used to fail up front, we now waste a lot of
effort, and possibly modify the backing chain irreversably, before
finding out we still fail because we lack write access).

> 
> Now that we consider all parents, overlay_bs is meaningless. It is left
> in place in this commit, but we'll remove it soon.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block.c        | 68 
> ++++++++++++++++++++++++++++++++++------------------------
>  block/commit.c |  2 +-
>  2 files changed, 41 insertions(+), 29 deletions(-)
>  
>      /* success - we can delete the intermediate states, and link top->base */
> -    if (new_top_bs->backing->role->update_filename) {
> -        backing_file_str = backing_file_str ? backing_file_str : 
> base->filename;
> -        ret = new_top_bs->backing->role->update_filename(new_top_bs->backing,
> -                                                         base, 
> backing_file_str,
> -                                                         &local_err);
> -        if (ret < 0) {
> -            bdrv_set_backing_hd(new_top_bs, top, &error_abort);
> +    backing_file_str = backing_file_str ? backing_file_str : base->filename;
> +
> +    QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
> +        /* Check whether we are allowed to switch c from top to base */
> +        GSList *ignore_children = g_slist_prepend(NULL, c);
> +        bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
> +                               ignore_children, &local_err);
> +        if (local_err) {
> +            ret = -EPERM;
> +            error_report_err(local_err);
>              goto exit;
>          }
> -    }
> +        g_slist_free(ignore_children);
>  

And it looks like you DO check up front that permissions are sufficient
for later on.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature