Re: [PATCH v7 39/47] blockdev: Fix active commit choice

[Kevin Wolf]

Am 25.06.2020 um 17:22 hat Max Reitz geschrieben:
> We have to perform an active commit whenever the top node has a parent
> that has taken the WRITE permission on it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  blockdev.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 402f1d1df1..237fffbe53 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2589,6 +2589,7 @@ void qmp_block_commit(bool has_job_id, const char 
> *job_id, const char *device,
>      AioContext *aio_context;
>      Error *local_err = NULL;
>      int job_flags = JOB_DEFAULT;
> +    uint64_t top_perm, top_shared;
>  
>      if (!has_speed) {
>          speed = 0;
> @@ -2704,14 +2705,31 @@ void qmp_block_commit(bool has_job_id, const char 
> *job_id, const char *device,
>          goto out;
>      }
>  
> -    if (top_bs == bs) {
> +    /*
> +     * Active commit is required if and only if someone has taken a
> +     * WRITE permission on the top node.

...or if someone wants to take a WRITE permission while the job is
running.

Future intentions of the user is something that we can't know, so maybe
this should become an option in the future (not in this series, of
course).

>                                            Historically, we have always
> +     * used active commit for top nodes, so continue that practice.
> +     * (Active commit is never really wrong.)
> +     */

Changing the practice would break compatibility with clients that start
an active commit job and then attach it to a read-write device, so we
must continue the practice. I think the comment should be clearer about
this, it sounds more like "no reason, but why not".

This is even more problematic because the commit job doesn't unshare
BLK_PERM_WRITE yet, so it would lead to silent corruption rather than an
error.

> +    bdrv_get_cumulative_perm(top_bs, &top_perm, &top_shared);
> +    if (top_perm & BLK_PERM_WRITE ||
> +        bdrv_skip_filters(top_bs) == bdrv_skip_filters(bs))
> +    {
>          if (has_backing_file) {
>              error_setg(errp, "'backing-file' specified,"
>                               " but 'top' is the active layer");

Hm, this error message isn't accurate any more.

In fact, the implementation isn't consistent with the QAPI documentation
any more, because backing-file is only an error for the top level.

>              goto out;
>          }
> -        commit_active_start(has_job_id ? job_id : NULL, bs, base_bs,
> -                            job_flags, speed, on_error,
> +        if (!has_job_id) {
> +            /*
> +             * Emulate here what block_job_create() does, because it
> +             * is possible that @bs != @top_bs (the block job should
> +             * be named after @bs, even if @top_bs is the actual
> +             * source)
> +             */

Should it? Oh, yes, looks like it. block-commit is weird. :-)

> +            job_id = bdrv_get_device_name(bs);
> +        }
> +        commit_active_start(job_id, top_bs, base_bs, job_flags, speed, 
> on_error,
>                              filter_node_name, NULL, NULL, false, &local_err);
>      } else {
>          BlockDriverState *overlay_bs = bdrv_find_overlay(bs, top_bs);

Kevin